Fix client-v2: write Nullable marker for nested Tuple/Map elements#2886
Open
polyglotAI-bot wants to merge 3 commits into
Open
Fix client-v2: write Nullable marker for nested Tuple/Map elements#2886polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
Conversation
…lements A Nullable element nested inside a Tuple or Map was serialized without its leading null-marker byte for present (non-null) values, corrupting the RowBinary stream so the server (and the client's own reader) mis-parsed the following bytes. Top-level columns get the marker from writeValuePreamble and serializeArrayData already wrote per-element markers, but serializeTupleData and serializeMapData called serializeData directly and skipped it. Add a serializeNestedData helper that writes the 0x00 (present) / 0x01 (null) marker for nested Nullable elements, and route Tuple elements and Map values through it. The array and top-level paths are unchanged. Fixes: #2721
|
Repository collaborators can run the JMH benchmark suite against this PR by commenting: Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%): Only one benchmark run per PR is active at a time — issuing a new |
There was a problem hiding this comment.
Pull request overview
Fixes client-v2 RowBinary serialization for nested Nullable(T) values inside Tuple elements and Map values by ensuring the required 0x00/0x01 null-marker byte is emitted for nested nullable values (without changing the top-level nullable path, which is handled by RowBinaryFormatSerializer.writeValuePreamble). This addresses misaligned reads/corruption reported in #2721 when nested present values previously omitted the 0x00 marker.
Changes:
- Add
serializeNestedData(...)helper to write the null-marker for nested nullable elements and delegate to existingserializeData(...). - Route tuple element serialization and map value serialization through
serializeNestedData(...)(map keys remain primitive/markerless). - Add round-trip regression tests covering tuple/map nullable cases and nested container composition.
Reviewed changes
Copilot reviewed 2 out of 2 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-nullable marker handling and applies it to tuple elements and map values during RowBinary serialization. |
client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java |
Adds round-trip unit tests to prevent regressions for nested Nullable in Tuple/Map (issue #2721). |
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
chernser
reviewed
Jun 22, 2026
chernser
requested changes
Jun 22, 2026
… feedback) Address review on #2886: squash the four nested-Nullable round-trip tests into a single TestNG @DataProvider-driven test, drop the issue-referencing/verbose test comments, and broaden coverage to all nested datatypes. The provider now exercises a present Nullable element of every byte-width class (String, FixedString, Int/UInt 8/16/32/64, Float32/64, Bool, UUID, Date, Decimal64, IPv4) nested in a Tuple, the same on the Map value path, both serializeTupleData branches (List and array input), null markers, container compositions including Array(Tuple(Nullable)) (how Nested is encoded), and non-nullable contrast cases that must keep serializing without a marker. A small normalize() helper compares round-tripped values structurally across the Tuple/Array/Map container representations the reader returns.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
chernser
requested changes
Jun 22, 2026
…ing Float64 Per review feedback: a single-column / leading-Nullable schema cannot detect a byte-misaligned nested-Nullable serialization, because nothing follows the dropped marker. Each present-Nullable data-provider row now puts the Nullable in the MIDDLE of the schema (Tuple(Int32, Nullable(X), Float64)) with a leading non-null column and a trailing fixed-width Float64, and the assertion compares the whole row — so a dropped not-null marker shifts the following bytes and the trailing Float64 reads a wrong value (or hits EOF), positionally detecting the fault. Also covers the Java-array serializeTupleData branch with a null element.
|
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 #2721 (upstream report: ClickHouse/flink-connector-clickhouse#65).
When
client-v2serializes aNullable(T)element nested inside aTupleorMaptoRowBinary, the leading null-marker byte was omitted for present (non-null) values. InRowBinaryaNullable(T)value is0x00+ value when present and0x01when null; without the0x00the server (and the client's ownBinaryStreamReader) mis-aligns and reads the value's first byte as the null flag, corrupting every following column.Root cause: the marker is the caller's responsibility. Top-level columns get it from
RowBinaryFormatSerializer.writeValuePreamble, andserializeArrayDataalready writes a per-element marker — butserializeTupleDataandserializeMapDatacalledSerializerUtils.serializeDatadirectly for their nested elements, skipping the marker. (Nullableonly wraps scalar types, so a nestedNullableelement always reachesserializeData'sdefault/primitive branch with no marker written.)Changes
SerializerUtils.serializeDatais unchanged for the top-level path (it must not write the marker there —writeValuePreamblealready does, so writing it here would double it).serializeNestedData(stream, value, column)helper that writes the0x00/0x01marker for a nestedNullableelement (and skips the value when null), then delegates toserializeData.serializeTupleData(both theListand array branches) and theserializeMapDatavalue through the helper. Map keys keep usingserializePrimitiveData(keys are neverNullable).serializeArrayDataand the top-level/primitive paths are left untouched (already correct).Test
New round-trip unit tests in
SerializerUtilsTest(SerializerUtils.serializeData→BinaryStreamReader.readValue, which reads the null flag uniformly, so a missing marker fails the round-trip):Tuple(Nullable(String), String)— present and null element (also pins the non-nullable sibling stays markerless).Map(String, Nullable(String))— present and null value (pins the non-nullable key stays markerless).Tuple(Nullable(Int32), Nullable(String))— fixed-width nullable element + all-null tuple.Tuple(String, Map(String, Nullable(String)))— composition through nested containers.Both originally-broken cases fail on
mainwithEOF End of stream reached before reading all data; all pass with the fix. The serialized bytes were verified byte-for-byte against a live ClickHouse server (26.5): e.g.('optional-value','value-2')::Tuple(Nullable(String),String)→00 0e …65 07 …32, andmap('k1','v1','k2',NULL)::Map(String,Nullable(String))→02 02 6b31 00 02 7631 02 6b32 01.Pre-PR validation gate
mainwith EOF, pass on branch)AGENTS.md/docs/ai-review.md/docs/changes_checklist.mdchanges_checklist.md walkthrough
serializeNestedDataisprivate, single-purpose, mirrors the existingserializeDatasignature, and has behavior-focused tests. No public API growth.Nullablepresent-value (which now matches the server). Verified the new value round-trips. Nodocs/features.mdchange: this is a correctness fix to already-supportedTuple/Map/Nullabletypes, not a new or changed user-visible feature.No central tracking issue exists for this bug; it was filed directly in
clickhouse-java.Note
Medium Risk
Touches RowBinary serialization for nested Tuple/Map values; wrong marker logic could still misalign bytes on insert, but scope is narrow and guarded by new round-trip tests.
Overview
Fixes RowBinary corruption when
Nullablescalars appear insideTupleorMapvalues: nested present values now get the required0x00null-marker (and nulls get0x01) before the payload, matching ClickHouse andBinaryStreamReader.Adds private
serializeNestedDatainSerializerUtilsfor that marker handling and switchesserializeTupleData(list and array paths) andserializeMapDatavalue serialization to use it; map keys stay onserializePrimitiveData. Top-levelserializeDatais unchanged soRowBinaryFormatSerializer.writeValuePreambleis not doubled.New
SerializerUtilsTestround-trips cover tuple/map nullable cases, fixed-width nullable tuple fields, and nested tuple+map composition (#2721).Reviewed by Cursor Bugbot for commit 0da71d8. Bugbot is set up for automated code reviews on this repo. Configure here.