Repository navigation
[client-v2] Reject a non-array value instead of writing no bytes for the column - #3194
Open
piyush15102003 wants to merge 1 commit into
Open
piyush15102003 wants to merge 1 commit into
piyush15102003 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3041.
What was wrong
SerializerUtils.serializeArrayDataguarded on "the value is aListor a Java array" and hadno
elsebranch, so a non-nullvalue of any other type returned having written nothing atall for the column — not even the var-int length
0.In
RowBinarythe bytes of the next column are then read as this column's length prefix and therest of the row is misframed. Over
RowBinarythat surfaces as a server-sideCode: 33 CANNOT_READ_ALL_DATA; overRowBinaryWithDefaultsthe insert can succeed and storecorrupted 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 typesRing/LineString/MultiPoint,Polygon/MultiLineString, andMultiPolygon. For the geo cases the unwrap isconditional (
value instanceof ClickHouseGeoXxxValue ? ... : value), so a wrong-typed value staysun-unwrapped and lands in the same silent skip.
The sibling container serializers already reject this:
Tuplethrows,Geometrythrows,Mapraises
ClassCastException, andQBitwas guarded for exactly this hazard in #2939.Arrayandthe geo types were the only container paths failing silently.
Change
One
elsebranch inserializeArrayData, which covers all six paths:The message follows
serializerVariant/serializerGeometryand names the value class and thecolumn 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 wouldbe misleading. Happy to change it if you would rather have the name.
nullstill writes the var-int length0, and arrays andLists serialize exactly as before.Testing
Added to
SerializerUtilsTest:@DataProvideroverArray(String),Array(UInt32),Array(Array(String))and the five geotypes with a wrong-typed value, each expecting
IllegalArgumentException;Stringafter it, assertingthe following column still reads back correctly — this is the drift the bug caused;
nullstill writes a single var-int0.SerializerUtilsTest, with this changeSerializerUtilsTest, with theelsebranch removedclient-v2unit suiteThe two positive tests (framing,
null) pass both with and without the fix, so they guard theunchanged behaviour rather than restating the bug.
The 5 pre-existing failures are 4 ×
CompressedBlockInputStreamTest(missingwin/amd64zstd-jninative, #3175) and
DataTypeConverterTest.testDateToString, which I confirmed fails identically onunmodified
mainin my timezone.Also updated
docs/features.mdand addedhistory/latest/3041.md, per the closeout checklist inAGENTS.md.