Skip to content

[client-v2] Reject a non-array value instead of writing no bytes for the column - #3194

Open
piyush15102003 wants to merge 1 commit into
ClickHouse:mainfrom
piyush15102003:fix/array-serializer-rejects-wrong-type
Open

piyush15102003 wants to merge 1 commit into
ClickHouse:mainfrom
piyush15102003:fix/array-serializer-rejects-wrong-type

Conversation

@piyush15102003

Copy link
Copy Markdown
Contributor

Closes #3041.

What was wrong

SerializerUtils.serializeArrayData guarded on "the value is a List or a Java array" and had
no else branch, so a non-null value of any other type returned having written nothing at
all
for the column — not even the var-int length 0.

In RowBinary the bytes of the next column are then read as this column's length prefix and the
rest of the row is misframed. Over RowBinary that surfaces as a server-side
Code: 33 CANNOT_READ_ALL_DATA; over RowBinaryWithDefaults the insert can succeed and store
corrupted rows (the issue shows one committed row becoming four, with the other column values
lost).

Six dispatch paths reach it — Array, and the five array-backed geo types Ring/LineString/
MultiPoint, Polygon/MultiLineString, and MultiPolygon. For the geo cases the unwrap is
conditional (value instanceof ClickHouseGeoXxxValue ? ... : value), so a wrong-typed value stays
un-unwrapped and lands in the same silent skip.

The sibling container serializers already reject this: Tuple throws, Geometry throws, Map
raises ClassCastException, and QBit was guarded for exactly this hazard in #2939. Array and
the geo types were the only container paths failing silently.

Change

One else branch in serializeArrayData, which covers all six paths:

throw new IllegalArgumentException("Cannot write value of class " + value.getClass()
        + " into column with array type " + column.getOriginalTypeName());

The message follows serializerVariant/serializerGeometry and names the value class and the
column type rather than the column name. The issue asked for the column name, but three of the
six paths pass synthetic columns (georing, geopolygin, geomultipolygin), so naming them would
be misleading. Happy to change it if you would rather have the name.

null still writes the var-int length 0, and arrays and Lists serialize exactly as before.

Testing

Added to SerializerUtilsTest:

  • a @DataProvider over Array(String), Array(UInt32), Array(Array(String)) and the five geo
    types with a wrong-typed value, each expecting IllegalArgumentException;
  • a test that nothing is written to the stream before the rejection;
  • a framing test with the array column in the middle of the row and a String after it, asserting
    the following column still reads back correctly — this is the drift the bug caused;
  • a test that null still writes a single var-int 0.
Run Result
SerializerUtilsTest, with this change 124 pass, 0 fail
SerializerUtilsTest, with the else branch removed 9 fail — the 8 rejection rows and the no-partial-write test
Full client-v2 unit suite 5 failures, all pre-existing

The two positive tests (framing, null) pass both with and without the fix, so they guard the
unchanged behaviour rather than restating the bug.

The 5 pre-existing failures are 4 × CompressedBlockInputStreamTest (missing win/amd64 zstd-jni
native, #3175) and DataTypeConverterTest.testDateToString, which I confirmed fails identically on
unmodified main in my timezone.

Also updated docs/features.md and added history/latest/3041.md, per the closeout checklist in
AGENTS.md.

Closes ClickHouse#3041.

serializeArrayData guarded on "value is a List or an array" and had no
else branch, so a non-null value of any other type returned having
written nothing at all for the column - not even the var-int length 0.
In RowBinary the next column's bytes are then read as this column's
length prefix and the rest of the row is misframed: over RowBinary the
server reports Code: 33 CANNOT_READ_ALL_DATA, and over
RowBinaryWithDefaults the insert can succeed and store corrupted rows.

Six dispatch paths reach it - Array, and the five array-backed geo types
Ring/LineString/MultiPoint, Polygon/MultiLineString and MultiPolygon,
whose unwrap is conditional, so a wrong-typed value stays un-unwrapped
and lands in the same silent skip. One else branch covers all of them.

The sibling container serializers already reject this: Tuple throws,
Geometry throws, Map raises ClassCastException, and QBit was guarded
for exactly this hazard in ClickHouse#2939. Array and the geo types were the only
ones failing silently.

The message follows serializerVariant/serializerGeometry and names the
value class and the column type rather than the column name, because
the geo paths pass synthetic columns (georing, geopolygin) whose names
would be misleading.

null still writes the var-int length 0, and arrays and Lists serialize
exactly as before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[client-v2] serializeArrayData silently writes zero bytes for a wrong-typed Array/geo value and desynchronizes the RowBinary stream

1 participant