Fix client-v2: serialize Nested columns in RowBinary writer#2936
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix client-v2: serialize Nested columns in RowBinary writer#2936polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
RowBinaryFormatWriter threw `UnsupportedOperationException: Unsupported data type: Nested` when inserting into a table whose schema keeps an un-flattened `Nested(...)` column (created with `flatten_nested = 0`), because SerializerUtils.serializeData had no case for the Nested type and fell through to serializePrimitiveData's default throw. A `Nested(f1 T1, ..., fN TN)` column has the same RowBinary layout as `Array(Tuple(T1, ..., TN))` - a var-uint element count followed by that many tuples - which is exactly how BinaryStreamReader.readNested reads it. serializeData now serializes it symmetrically. Fixes: #2477
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
8 tasks
Member
|
@cursor review |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes client-v2 RowBinary insert serialization for un-flattened Nested(...) columns (tables created with flatten_nested = 0), aligning the write path with the existing read-path behavior and ClickHouse’s RowBinary wire layout.
Changes:
- Add
Nestedhandling inSerializerUtils.serializeDataand implementNestedserialization asArray(Tuple(...))(var-uint row count + tuples). - Add unit round-trip coverage for
Nested(...)serialization (including nullable field alignment cases). - Add an integration test that reproduces the reported failure through
RowBinaryFormatWriter+client.insert, and updateCHANGELOG.md.
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/internal/SerializerUtils.java | Adds Nested case and serializes it with the correct RowBinary layout (equivalent to Array(Tuple(...))). |
| client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java | Adds deterministic unit round-trip tests covering Nested(...), including nullable-in-middle alignment. |
| client-v2/src/test/java/com/clickhouse/client/datatypes/RowBinaryFormatWriterTest.java | Adds integration coverage for flatten_nested = 0 tables using an un-flattened Nested(...) column. |
| CHANGELOG.md | Documents the client-v2 bug fix and links the issue. |
…nd-trip Adds an Object[][] "matrix array" case to the Nested RowBinary round-trip DataProvider, exercising convertArrayValueToList's array branch and serializeTupleData's array branch (the existing cases used List shapes only). Documents that array-shaped Nested values round-trip correctly.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e0f4674. Configure here.
| private static void serializeNestedTypeData(OutputStream stream, Object value, ClickHouseColumn column) throws IOException { | ||
| if (value == null) { | ||
| writeVarInt(stream, 0); | ||
| return; |
There was a problem hiding this comment.
Null Nested rejected unlike Array
Medium Severity
RowBinaryFormatWriter still rejects a Java null for a non-nullable Nested column in writeValuePreamble, while non-nullable Array columns are allowed through and serialize as an empty array. The new serializeNestedTypeData writes a zero element count for null, but that path is never reached on insert.
Reviewed by Cursor Bugbot for commit e0f4674. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Thanks — I checked this empirically against a live server (ClickHouse 26.5) and I don't think we should let a Java null through for a non-nullable Nested column, because the Array path it would mirror is itself byte-incorrect.
Here is exactly what writeValuePreamble + the serializer emit for a non-nullable column given a Java null:
| value | bytes the client emits |
|---|---|
Array(Int32) + null |
00 00 |
Nested(a Int32, b String) + null |
throws IllegalArgumentException: An attempt to write null into not nullable column |
Nested(a Int32, b String) + Arrays.asList() (empty) |
00 |
The Array case emits two bytes: writeValuePreamble writes writeNonNull (0x00) and then serializeArrayData(null) writes the var-uint length 0 (0x00). Sending those bytes to the server for a single Array(Int32) column does not insert one empty array — it inserts a phantom second row:
# one 0x00 -> 1 row
printf '\x00' | INSERT ... FORMAT RowBinary ; SELECT count(), groupArray(arr) -> 1 [[]]
# two 0x00 -> 2 rows (the extra preamble byte is read as a second empty-array row)
printf '\x00\x00' | INSERT ... FORMAT RowBinary ; SELECT count(), groupArray(arr) -> 2 [[],[]]
So Array doesn't correctly "serialize null as an empty array" — it silently corrupts the stream (and in a multi-column row that extra byte shifts every following column). Making Nested match Array would copy that bug.
The current Nested behavior — throwing for null into a non-nullable column — is the safe and consistent choice (it's what every non-Array/Dynamic type does) and it fails loudly instead of corrupting data. The correct way to write an empty Nested is an empty list, which round-trips correctly (00, a single empty row) and is covered by the unit test. The if (value == null) writeVarInt(0) guard in serializeNestedTypeData is a deliberate mirror of the identical guard in serializeArrayData and emits correct bytes if reached via direct/recursive serialization, so I've kept it.
I'm leaving this thread open. The pre-existing Array + null phantom-row issue is a separate behavior change unrelated to the Nested support in this PR, so I'm flagging it to be fixed on its own rather than folding it in here. If you'd prefer top-level null to mean "empty Nested", the correct fix would be a do-nothing preamble branch (emitting a single 00), not the Array branch — happy to add that, but it's a separate decision from the crash fix 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 #2477.
RowBinaryFormatWriter(client-v2) threwUnsupportedOperationException: Unsupported data type: Nestedwhen inserting into a table whose schema keeps an un-flattenedNested(...)column. ANestedcolumn stays typed asNested(...)(rather than being expanded into parallelArray(...)sub-columns) when the table is created withflatten_nested = 0, which is the case the reporter hit.SerializerUtils.serializeDatahad nocase Nested, so it fell through toserializePrimitiveData'sdefault:branch and threw.A
Nested(f1 T1, ..., fN TN)column has the sameRowBinarylayout asArray(Tuple(T1, ..., TN)): a var-uint element (row) count followed by that many tuples, each carrying the N field values in declaration order. I confirmed this byte-for-byte against a live ClickHouse 26.5 server —SELECT n FROM <nested table> FORMAT RowBinaryis identical to the equivalentArray(Tuple(...))literal. It is also exactly how the read path already interprets it (BinaryStreamReader.readNested). The writer now serializes it symmetrically.Changes
SerializerUtils.serializeData: addedcase Nested→ new private helperserializeNestedTypeData.serializeNestedTypeData: writes the var-uint element count, then serializes each element via the existingserializeTupleData(aNestedcolumn'sgetNestedColumns()are the tuple field columns). Anullvalue serializes as a zero-length array, matchingserializeArrayData. Nullable fields inside the tuple keep their null-marker byte via the existingserializeNestedDatapath, soNested(a UInt32, b Nullable(String))round-trips correctly.No public API change; no
docs/features.mdentry (it has no RowBinary-writer type list — theNestedmention there is the unrelated JDBC result-set type map). No existing tests modified.Test
SerializerUtilsTest, deterministic round-tripserializeData→BinaryStreamReader.readValue, via@DataProvider):Nested(a Int32, b String)(two rows);Nested(a Int32, b Nullable(String), c Float64)with a present and a null middle field plus a trailing fixed-widthFloat64(so a dropped null-marker byte misaligns the float and is caught); and an emptyNested.RowBinaryFormatWriterTest.writeNestedTypeTests, the reported entry point —client.insert+RowBinaryFormatWriter, thenSELECTback): aflatten_nested = 0table withn Nested(a UInt32, b Nullable(String)).client.insertpath with "Insert request failed", caused by theUnsupportedOperationException) and pass with it. Full module runs stay green (44 unit, 18 integration in the touched files); no existing test was changed.Pre-PR validation gate
main, passes on branch — unit and integration)case Nested; wire format =Array(Tuple(...)))serializeData, the single write funnel; proven via theRowBinaryFormatWriter/client.insertintegration test)CHANGELOG.mdupdated (client-v2 Bug Fixes)AGENTS.md/docs/changes_checklist.mdchanges_checklist.mdserializeNestedTypeDataisprivate, does one thing (serialize aNestedcolumn asArray(Tuple(...))), reuses existing helpers rather than duplicating them, and is covered by behavior tests.RowBinaryoutput.Note on "probably more unsupported types"
While auditing the writer against the reader I found one further genuine, reachable gap of the same kind:
SimpleAggregateFunction(func, T)is handled by the reader but not the writer. It is intentionally not included here to keep this PR focused on the reportedNestedbug — it has an extra subtlety worth its own change: when the underlying type isNullable(T)the null-marker lives on the inner column while the outer column is non-nullable, so both the serializer delegation andwriteValuePreambleneed to account for that. Happy to follow up with a dedicated PR. (DateTime32is not a gap — DESCRIBE normalizes it toDateTime, which is already handled.)