Fix client-v2: reject null into non-nullable Array in RowBinary writer#2940
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix client-v2: reject null into non-nullable Array in RowBinary writer#2940polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
Writing a Java null into a non-nullable Array(...) column via RowBinaryFormatWriter emitted two bytes (00 00) instead of one: writeValuePreamble special-cased Array and wrote a stray writeNonNull marker (0x00) on top of the array length that serializeArrayData(null) already writes (var-uint 0 = 0x00). The server read the extra byte as a phantom extra row (single-column inserts) or a column shift that failed the whole insert with CANNOT_READ_ALL_DATA (multi-column inserts). A non-nullable Array cannot represent a null, so it now throws IllegalArgumentException naming the column - like every other non-nullable type and the merged Enum fix (#2932) - in both the RowBinary and RowBinaryWithDefaults branches. Empty arrays still serialize as a single length byte, columns with a DDL default still use the default under RowBinaryWithDefaults, and Dynamic columns (which can hold a null as the implicit Nothing type) are left unchanged. Fixes: #2938
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Member
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 577e765. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a client-v2 RowBinary writer corruption bug where writing Java null into a non-nullable Array(...) column emitted an extra marker byte, causing phantom rows or column shifts on the server. The fix removes the Array null special-case so non-nullable arrays now reject null consistently (throwing IllegalArgumentException), while keeping Dynamic’s existing “null as Nothing” behavior intact.
Changes:
- Remove
Arrayspecial-casing inRowBinaryFormatSerializer.writeValuePreamble()sonullinto non-nullableArray(...)throws instead of emitting an extra byte. - Add integration tests covering: (1) null into non-nullable array throws for both
RowBinaryandRowBinaryWithDefaults, (2) valid arrays still round-trip without shifting trailing columns, (3) defaulted array columns underRowBinaryWithDefaultsstill use the default onnull. - Add/extend unit coverage pinning that
Dynamicwithout defaults serializesnullas the implicitNothingtag (single byte).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/RowBinaryFormatSerializer.java | Removes the Array null special-case so non-nullable arrays reject null; keeps Dynamic null behavior. |
| client-v2/src/test/java/com/clickhouse/client/datatypes/RowBinaryFormatWriterTest.java | Adds integration tests to prevent regression (throwing on invalid null arrays, preserving valid array framing, default fallback). |
| client-v2/src/test/java/com/clickhouse/client/internal/SerializerUtilsTests.java | Adds a unit test pinning Dynamic null serialization in plain RowBinary (no defaults). |
| CHANGELOG.md | Documents the bug fix and its user-visible impact (preventing silent corruption). |
chernser
requested changes
Jul 24, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
Add integration tests in DataTypeTests
Addresses review feedback on #2938: add integration tests in DataTypeTests for the null-into-non-nullable-Array RowBinary writer fix. These exercise the fix through the POJO insert path (client.insert -> POJOSerDe -> RowBinaryFormatSerializer.writeValuePreamble), a different entry point than the existing RowBinaryFormatWriterTest coverage: - null into a non-nullable Array now throws IllegalArgumentException, pinned in both the plain RowBinary branch (no defaults) and the RowBinaryWithDefaults branch (a sibling column carries a default); - valid empty/populated arrays still round-trip with the trailing column intact (no misframing); - a null into a non-nullable Array that HAS a DDL default still coerces to the default instead of throwing.
Collaborator
Author
|
@chernser thanks — added integration tests in They cover the null-into-non-nullable-
Verified both directions against a live server: all 5 invocations pass with the fix; reverting only the source fix fails the two "throws" cases for the right reasons ( |
|
Member
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 816f99c. Configure here.
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.
Description
Fixes #2938.
Writing a Java
nullinto a non-nullableArray(...)column viaRowBinaryFormatWriterserialized two bytes (00 00) where the correct RowBinary encoding of an empty array is a single byte (00).RowBinaryFormatSerializer.writeValuePreamblespecial-caseddataType == Array, writing a straywriteNonNullmarker (0x00) and returningtrue; the caller then invokedSerializerUtils.serializeArrayData(null), which writes the var-uint length0(another0x00). The server read the extra byte as a phantom extra row (single-column inserts) or as a column shift that failed the whole insert withCANNOT_READ_ALL_DATA(multi-column inserts) — silent data corruption, not an error.A non-nullable
Arraycannot represent anull, so it now throwsIllegalArgumentExceptionnaming the column — consistent with every other non-nullable type and the mergedEnumfix (#2932) — by removing theArrayspecial-case in both theRowBinaryandRowBinaryWithDefaultsbranches so it falls through to the existingthrow.Verified against ClickHouse
26.5.1.882: raw00→ one row[[]]; the client's00 00→ two rows[[],[]](single column) /CANNOT_READ_ALL_DATA(multi column).Changes
client-v2/.../data_formats/RowBinaryFormatSerializer.java— removed thedataType == Arrayspecial-case from both branches ofwriteValuePreamble;nullinto a non-nullableArraynow hits the existingIllegalArgumentException.Scope (deliberate boundaries)
Dynamicleft unchanged in both branches. ADynamiccolumn can legitimately hold anull(serialized as the implicitNothingtype — verified round-trip: the server storesNULL,dynamicType='None'), so throwing there would break a working feature. This is a genuine semantic distinction (anArraycannot representnull; aDynamiccan), not theArraycorruption.[]) unchanged — still a single length byte.RowBinaryWithDefaultsanullinto a defaultedArraystill uses the default (thehasDefaultpath runs before the type dispatch).Test
RowBinaryFormatWriterTest.writeNullIntoNonNullableArrayThrowsTest(@DataProvideroverRowBinaryandRowBinaryWithDefaults) — insertsnullintoarrin(id Int32, arr Array(Int32), tail Int32)viaRowBinaryFormatWriterand asserts a clearIllegalArgumentException. Fails onmain(serverCANNOT_READ_ALL_DATAforRowBinary; silent coercion to[]forRowBinaryWithDefaults), passes with the fix.RowBinaryFormatWriterTest.writeNonNullableArrayRoundTripsTest(both formats) — contrast: empty and populated arrays still round-trip, and the fixed-widthtailcolumn after the array keeps its value (a stray byte would shift it).RowBinaryFormatWriterTest.writeNullIntoDefaultedArrayUsesDefaultTest— contrast:nullinto a defaultedArrayunderRowBinaryWithDefaultsstill uses the DDL default.SerializerUtilsTests.testDynamicNullWithoutDefaultsWritesNothingTag— contrast:nullinto a non-nullableDynamic(plainRowBinary) still emits the singleNothingtag (unchanged).Full
client-v2RowBinaryFormatWriterTest(26 integration tests) and the serializer unit tests (49) pass; no existing tests were modified.changes_checklist.md
nullin a non-nullableArrayno longer silently corrupts the stream; the new failure path is covered by tests. Nullable arrays, empty arrays, defaulted columns, andDynamicare unaffected.IllegalArgumentExceptionmessage/type already used for every other non-nullable column, so caller/test expectations stay uniform.RowBinaryWithDefaultsvsRowBinary, defaulted column,Dynamic.Pre-PR validation gate
main, passes with fix)Arrayspecial-case)client-v2,@DataProvider, no issue refs in test code, field-in-middle serialization test)docs/features.mdchange needed (documents readers, not writer null-semantics)RowBinaryFormatWriter.commitRowandPOJOSerDeboth callwriteValuePreamble), proven end-to-end throughclient.insert(...)