Fix client-v2: consume Nullable null marker when reading into a primitive POJO field - #2994
Open
polyglotAI-bot wants to merge 1 commit into
Open
Fix client-v2: consume Nullable null marker when reading into a primitive POJO field#2994polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
…tive POJO field The compiled POJO setter chose the box-free read path on `targetType.isPrimitive() && isReadToPrimitive(column.getDataType())` without considering nullability, so a Nullable(T) column bound to a primitive field went straight to a primitive read method and left the one-byte null marker - which is on the wire for every value of a nullable column regardless of the value - in the stream. That shifted the stream by one byte per row, decoding the nullable column and every column after it from the wrong offset without raising anything. The generated setter now always calls BinaryStreamReader#readNullMarkerForPrimitive, which consumes the marker of a nullable column and reports a value that is actually NULL with a NullValueException, since a primitive field cannot hold it. Nullability is resolved from the column being read rather than at compile time, so a result whose nullability differs from the registered schema is read correctly too. Fixes: #2993
|
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
|
@claude review this PR |
|
Claude finished @mshustov's task in 4m 48s —— View job Code Review
FindingsHigh
Medium
Low1. The UInt64 + primitive branch now partially improves stream state but the ClassCastException is still inevitable
2.
3.
Correctness analysisRoot cause fix: The compiled setter previously called Stack invariant: The bytecode generated by
Runtime vs. compile-time nullability: The column argument comes from
Error message format: TestsThe five new integration tests in
The cause-chain unwinding in Documentation
Human Review Instructions for Important ChangesThis change touches the compiled-setter serialization fast path in
Verdict
|
7 tasks
Collaborator
Author
|
Thanks for the review, @mshustov / @claude — I evaluated all three Low 1 — UInt64 + primitive branch asymmetryThe underlying defect in that branch is no longer un-tracked: the compiled setter there never emits a read call at all, so the On the specific safety question — does consuming the marker there leave the stream partially consumed in a way that surprises callers? No, and it is strictly better than the status quo:
Keeping the marker read in that branch is also deliberately forward-compatible: once #2997 lands, nullability in that branch is already handled correctly with no further change. Low 2 —
|
|
Claude finished @polyglotAI-bot's task in 1m 16s —— View job
Follow-upAll three responses check out against the code. Here is the verification for each, plus a note on the optional Javadoc offer. Low 1 — UInt64 branch Confirmed. Low 2 — Confirmed. Low 3 — Confirmed. The optional Javadoc offer (Low 3) Adding a one-line note like Human-review confirmation points
Merge-conflict note The heads-up about the trivial conflict with #2997 in No new findings. The PR is solid; the three Low items are satisfactorily addressed. |
7 tasks
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
2 participants
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 #2993.
On the compiled-POJO read fast path,
SerializerUtils.compilePOJOSetterselected the box-free path ontargetType.isPrimitive() && BinaryStreamReader.isReadToPrimitive(column.getDataType()). Neither half of thatcondition looks at nullability —
isReadToPrimitiveanswers for the data type, andInt64is readable to aprimitive whether or not the column is nullable — so the generated setter emitted a bare
BinaryStreamReader.readLongLE()and never consumed the one-byteNullablenull marker. That marker is on thewire for every value of a nullable column because the column is declared
Nullable, not because a value isnull, so the stream stayed shifted by one byte per row: the nullable column and every column after it decoded from
the wrong offset, and because the reader still consumed the right total number of bytes nothing was raised. The
caller silently got plausible-looking wrong numbers (or, further along, an
EOFException). The generated setter nowcalls the new
BinaryStreamReader#readNullMarkerForPrimitive, which consumes the marker for a nullable column and —since a primitive field cannot hold NULL — reports an actual NULL with a
NullValueException, matching how theexisting primitive accessors (
MapBackedRecord,AbstractBinaryFormatReader) report the same situation.Nullability is decided by the callee from the column being read, not at compile time from the registered schema.
POJOSerDekeys deserializers by table name or query, so a registered schema and the actual result can disagreeabout a column's nullability (e.g. registering a table schema whose column is
Nullable(Int64)and then reading aprojection that drops the wrapper). A compile-time decision would produce the same one-byte shift in the opposite
direction; resolving it from the column being read makes the fast path agree with
readValueby construction.Changes
client-v2SerializerUtils.compilePOJOSetter: both primitive branches now emit a call toBinaryStreamReader#readNullMarkerForPrimitivebefore the primitive read (new privatenullMarkerReaderForPrimitiveemitter;DUP+ column + target-type name, net-zero on the operand stack).client-v2BinaryStreamReader#readNullMarkerForPrimitive(ClickHouseColumn, String): consumes the null marker ofa nullable column, no-op otherwise, and throws
NullValueExceptionwhen the value is NULL.CHANGELOG.md,docs/features.md: documented the fix and the primitive-field/NULL behavior of POJO binding.Note on the second primitive branch (
UInt64+ primitive field): it also gets the marker read for consistency, butit has a separate, pre-existing defect — it emits no read call at all, so
CHECKCAST BigIntegeris applied tothe reader itself and any primitive field bound to a
UInt64column fails with aClassCastException(reproducedon
main, unrelated to nullability). That is intentionally left out of this PR; it will be reported and fixed on itsown.
Test
Five integration tests in
client-v2DataTypeTests(POJO read throughqueryAll, i.e. the live runtime path):testReadNullableColumnsIntoPrimitivePojoFields— oneNullable(T)column per type inisReadToPrimitive(Int8/UInt8/Int16/UInt16/Int32/UInt32/Int64/Float32/Float64/Bool/Enum8/Enum16)at boundary magnitudes, bound to primitive fields, with a non-nullable
Int32in front and a non-nullableInt64witness at the end, over 3 rows with the whole row asserted — so any dropped/extra byte shifts a later field or
loses a row and is detected. Fails on
mainwithEOFException.testReadNullableBFloat16IntoPrimitivePojoField—Nullable(BFloat16)(24.11+, skipped below that) into afloatfield. Onmainreturns-2.0and a corrupt trailing column.testReadNullValueIntoPrimitivePojoField— a real NULL into a primitive field reportsNullValueExceptionnaming the column and the target type. On
mainit desynchronizes into anEOFExceptioninstead.testReadNullValueIntoBoxedPojoField— contrast case: the boxed field path is untouched and still reads NULL(passes on
main).testReadNonNullableColumnIntoPrimitivePojoFieldRegisteredAsNullable— contrast case for the runtime-vs-schemanullability decision: a schema whose column is
Nullable(Int64)with a projection that is not nullable stilldecodes correctly (passes on
main).Existing tests are unchanged. Run in the module:
client-v2unit tests (1665 + 131 + 529, 0 failures) and theDataTypeTests(99),QueryTestsandInsertTestsintegration suites (126), all green against ClickHouse 26.5.docs/changes_checklist.mdBinaryStreamReader#readNullMarkerForPrimitive): name and parameters follow theneighbouring read helpers; it lives next to
isReadToPrimitive, the predicate that selects the path it serves;visibility is
publiconly because the generated setter class calls it by reflection-free bytecode (the class isin the
data_formats.internalpackage); nullability expectations are stated in the Javadoc; it is covered bybehavior-focused tests through the POJO read path.
previously produced corrupt rows — a NULL read into a primitive field is now reported instead of shifting the
stream. No public API is removed or changed, no config key or default is touched, and boxed POJO fields are
unaffected.
docs/features.mdis updated because the POJO-binding behavior is user-visible.serialization/validation changes.
Pre-PR validation gate
main, each for the desync reason)AGENTS.mdanddocs/changes_checklist.md;docs/features.mdandCHANGELOG.mdupdated