SEPURI-SAI-KRISHNA opened a new pull request, #28947:
URL: https://github.com/apache/flink/pull/28947
## What is the purpose of the change
`TritonTypeMapper#deserializeArrayFromJson` silently corrupts null elements
in array-typed
Triton responses. A JSON `null` inside an array is read back as `0`,
`false`, or the literal
four-character string `"null"`, with `isNullAt()` reporting `false` at that
position — so the
substituted value is indistinguishable from a real prediction downstream.
Two things combine to cause it:
1. Each branch builds a **primitive** backing array (`int[]`, `double[]`,
`boolean[]`, ...) and
fills it with Jackson's coercing accessors. `NullNode.asInt()` returns
`0`,
`asBoolean()` returns `false`, `asDouble()` returns `0.0`, and `asText()`
returns the string
`"null"`. No branch checks `isNull()`.
2. Even a check would not have been enough: `GenericArrayData#isNullAt` is
`return !isPrimitiveArray && ((Object[]) array)[pos] == null;`, so a
primitive-backed
`GenericArrayData` reports `isNullAt() == false` at every position **by
construction**. A null
cannot be represented in that shape at all.
Measured on unpatched master:
| Declared type | JSON payload | Read back as |
|---|---|---|
| `ARRAY<STRING>` | `["a", null, "b"]` | `isNullAt(1) == false`,
`getString(1) == "null"` (4 chars) |
| `ARRAY<INT>` | `[1, null, 3]` | `isNullAt(1) == false`, `getInt(1) == 0` |
| `ARRAY<DOUBLE>` | `[1.5, null]` | `getDouble(1) == 0.0` |
| `ARRAY<BOOLEAN>` | `[true, null]` | `getBoolean(1) == false` |
This is a correctness bug rather than a crash, which is what makes it worth
fixing: an inference
result of `0` or `false` looks like a legitimate model output, so the
corruption propagates
silently into user queries.
It is also an asymmetry within the same class. `serializeArrayToJsonArray`
already emits
`addNull()` for a null element, and the scalar path `deserializeFromJson`
already returns `null`
for a null node. Only the array deserialization path drops the information,
so a null does not
survive a serialize/deserialize round trip.
## Brief change log
- `deserializeArrayFromJson` scans for a null element first. When there is
none — the common case
— the existing primitive fast path runs completely unchanged.
- When a null is present, `deserializeNullableArrayFromJson` builds a boxed
`Object[]` and
delegates each element to the existing `deserializeFromJson`, which
already maps a null node to
a Java `null` and already covers every element type the primitive path
supports, including the
`FloatType` `isNumber()` special case.
- Nested array element types are rejected on the nullable path as well, so
the presence of a null
element cannot change which element types are accepted.
- A null against a `NOT NULL` element type is rejected with a clear message
rather than written
into an array whose declared type forbids it.
## Verifying this change
This change adds tests and can be verified as follows:
- `TritonTypeMapperTest#testDeserializeArrayWithNullStringElement`
- `TritonTypeMapperTest#testDeserializeArrayWithNullNumericElements`
- `TritonTypeMapperTest#testDeserializeArrayWithNullBooleanElement`
- `TritonTypeMapperTest#testNullElementSurvivesSerializeDeserializeRoundTrip`
- `TritonTypeMapperTest#testDeserializeArrayRejectsNullForNotNullElementType`
- `TritonTypeMapperTest#testDeserializeArrayWithoutNullsIsUnchanged`
Red/green verified against this exact commit:
- **Without** the production change: `Tests run: 14, Failures: 5` — the five
null cases fail,
reading back exactly the `0` / `false` / `"null"` values tabulated above.
- **With** it: `TritonTypeMapperTest` 14/14, and the whole
`flink-model-triton` module
`Tests run: 99, Failures: 0, Errors: 0, Skipped: 0`.
`testDeserializeArrayWithoutNullsIsUnchanged` passes in both directions by
design — it pins the
primitive fast path so a future change cannot quietly route null-free
payloads through the boxed
branch.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): **no**
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: **no**
- The serializers: **no**
- The runtime per-record code paths (performance sensitive): **yes,
marginally.** Null-free
payloads keep the existing primitive arrays and gain one `isNull()` pass
over a node list that
is iterated immediately afterwards. Payloads containing a null take a
boxed array, which is
the only shape that can represent the value correctly.
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing,
Kubernetes/Yarn, ZooKeeper: **no**
- The S3 file system connector: **no**
## Documentation
- Does this pull request introduce a new feature? **no**
- If yes, how is the feature documented? **not applicable**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
