feat(client-v2): decode QBit in the Native format - #2983
Open
polyglotAI-bot wants to merge 4 commits into
Open
Conversation
Reading a plain top-level QBit(Float32|Float64|BFloat16, dimension) column over the Native format previously failed fast: the server transmits QBit there using its internal bit-plane-transposed Tuple(FixedString) layout rather than the Array-like encoding used over RowBinary, which the reader could not decode. Reconstruct the vector by reversing that transposition (the exact inverse of the server's SerializationQBit transpose): read the element_size bit-plane FixedString columns column-major and, for each row, scatter each plane's bits back to their element/bit positions. Values are materialized into the same float[]/double[] ArrayValue representation as the RowBinary path, so a QBit column round-trips identically through either format. Strided QBit(E, dim, stride), Nullable/LowCardinality-wrapped QBit, and QBit nested inside a container remain rejected with a clear error directing callers to a RowBinary format. Implements: #2610
chernser
reviewed
Jul 29, 2026
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds client-v2 support for reading plain top-level QBit(Float32|Float64|BFloat16, dimension) columns from ClickHouse’s Native output format by reversing the server’s bit-plane transposition, bringing Native read parity with the existing RowBinary QBit handling.
Changes:
- Implement Native-format
QBitblock decoding inBinaryStreamReaderand wire it intoNativeFormatReaderbehind a strict “decodable QBit” gate. - Add integration tests asserting
Native == RowBinaryparity for multiple element types, dimensions, and multi-row blocks; keep nested/unsupported variants rejected. - Update
docs/features.mdandCHANGELOG.mdto reflect Native read support and remaining limitations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java |
Adds Native QBit bit-plane transpose inversion to materialize per-row vectors. |
client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java |
Decodes top-level float QBit via the new reader method; rejects other QBit shapes early with an error. |
client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java |
Adds integration coverage for Native QBit reads (single/multi-row) and preserves nested rejection behavior. |
docs/features.md |
Documents Native read support for plain top-level QBit, plus unsupported variants. |
CHANGELOG.md |
Notes the new Native read capability and limitations in the unreleased section. |
Comment on lines
118
to
+122
| for (int j = 0; j < nRows; j++) { | ||
| values.add(binaryStreamReader.readArrayItem(column.getNestedColumns().get(0), sizes[0])); | ||
| } | ||
| } else { | ||
| values = new ArrayList<>(nRows); |
Collaborator
Author
There was a problem hiding this comment.
Thanks — but this sizes[0] per-row length is a pre-existing bug in the generic Array(T) Native decode path, not something this PR introduces: the flagged line is an unchanged context line. This PR chains the new QBit branch ahead of the existing else if (column.isArray()) block but does not modify its body.
It also doesn't affect QBit reads: a plain top-level QBit is handled by readQBitNative, and any other QBit is rejected, both before this Array branch is reached — so QBit never flows through sizes[0].
The actual fix (sizes[0] → per-row offsets[j] - offsets[j-1]) is already in flight in #2956, which owns the NativeFormatReader multi-row Array offset correction. Leaving it out of scope here to keep this PR to the QBit feature and avoid conflicting with #2956.
…ard plane-size overflow - Reject message + docs (features.md, CHANGELOG) now state that only a plain top-level QBit with a Float32/Float64/BFloat16 element type is decoded over Native; a QBit with any other element type is rejected alongside the strided/Nullable/LowCardinality/nested cases. The server constrains QBit element types to those three, so the non-float case is only reachable via the client's forward-compat lenient type parser. - Guard the per-plane byte count (nRows * ceil(dimension/8)) against int overflow in readQBitNative: compute it as a long and throw a clear ClientException instead of wrapping to a negative/short readNBytes length that would desynchronize the stream. - Add a unit test pinning the overflow guard.
chernser
requested changes
Jul 30, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
coverage of the new code is only 41% - add more test with different values, test whatever fail scenarios possible
there are too many verbose comments - shorten them. create separate document about Qbit encoding in Native and Rowbinary to shortly describe the "problem".
…add encoding doc - Add unit tests for BinaryStreamReader#readQBitNative covering all supported element types, single/two-byte bit planes, multi-row slicing, special float values (NaN/Inf/-0.0), and the unsupported-element-type and int-overflow guards, driven by Native-format QBit bytes captured from the server (readQBitNative new-code coverage 41% -> 100%). - Add NativeFormatReaderQBitTest covering the block-level decode-vs-reject routing (isNativeDecodableQBit / containsQBit) without a live server. - Move the bit-plane transpose math into docs/qbit-encoding.md and trim the inline comments in readQBitNative / NativeFormatReader to short pointers; link the doc from docs/features.md. Addresses @chernser review feedback on #2983.
Collaborator
Author
|
Thanks for the review, @chernser — addressed all three in f71744e: 1. New-code coverage. Root cause of the 41%: the QBit integration tests skip on the coverage build (it runs CH 25.3 via
Local jacoco now reports 2. Verbose comments. Trimmed the byte/bit-math comments in 3. Encoding doc. Added All QBit unit + integration tests are green locally. |
|
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
Implements #2610 (follow-up to #2939).
client-v2gainedQBitread/write overRowBinaryin #2939, but reading aQBitcolumn over theNativeoutput format still failed fast: overNativethe server transmitsQBitusing its internal bit-plane-transposedTuple(FixedString(...))layout, not theArray(element_type)-like encoding used overRowBinary, and the reader had no decoder for it.This PR decodes a plain top-level
QBit(Float32 | Float64 | BFloat16, dimension)column from theNativeformat, so such a column can now be read throughNativeand round-trips identically toRowBinary.Design
The
NativeQBitlayout was verified against the ClickHouse server source (SerializationQBit::transposeBits) and empirically against server26.5.1:QBitcolumn is the server's nestedTuple(FixedString(S) × element_size)serialized column-major (plane-major):element_sizebit planes (Float32→32,Float64→64,BFloat16→16), eachS = ceil(dimension/8)bytes per row, laid outplane0[all rows] plane1[all rows] ….pcarries float bit(element_size − 1 − p)of every element (most-significant plane first). Within a plane'sSbytes, elementjis at byte(bitIndex >> 3), bit(bitIndex & 7)wherebitIndex = (S*8 − 1) − (j ^ 7).BinaryStreamReader#readQBitColumnreverses that transposition per row and materializes the vector into the samefloat[]/double[]ArrayValue(via the sameconvertArraypath) that the RowBinaryreadQBitproduces — so aQBitread overNativeand overRowBinaryyields equal values through generic records, binary readers, and POJO binding.NativeFormatReaderdecodes the column when it is a plain top-level floatQBit, and otherwise preserves the existing fail-loud rejection.Kept out of scope (still rejected with a clear, actionable
ClientException, as before):QBit(E, dim, stride)— a master-only server feature (server26.5.1rejects the 3-arg type), so it cannot be exercised/verified here;Nullable/LowCardinality-wrappedQBit;QBitnested inside a container (Array/Tuple/Map).These remain readable over a
RowBinaryformat.Changes
client-v2BinaryStreamReader: newreadQBitColumn(column, nRows)— the block-level inverse bit-plane transpose decoder.client-v2NativeFormatReader.readBlock: decode a plain top-level floatQBit; newisNativeDecodableQBitgate; the rejection (andcontainsQBitjavadoc) narrowed to the strided/wrapped/nested variants with an accurate message.DataTypeTests): newtestQBitNativeFormatRead(@DataProvider:Float32/Float64/BFloat16; dims 3, 8, 10, 16 covering single-byte and two-byte / partially-filled planes; negative and fractional values; assertsNative == RowBinaryparity),testQBitNativeFormatMultiRow(column-major per-row slicing forS=1andS=2), andtestQBitNativeFormatRejectednarrowed/renamed totestQBitNativeFormatNestedRejected(top-levelQBitnow decodes; the nested case still rejects — the one existing assertion this feature intentionally changes).CHANGELOG.mdanddocs/features.md: updated theQBitentry to describeNativeread support and the remaining unsupported variants.Test
Run against ClickHouse
26.5.1via the integration harness:testQBitNativeFormatRead(6 cases ×Native+RowBinary),testQBitNativeFormatMultiRow,testQBitNativeFormatNestedRejected, plus the pre-existingtestQBit/testQBitInDynamicColumn— 10/10 green.DataTypeTestsintegration class (101/101) andBinaryStreamReaderTests+BinaryReaderBackedRecordTestunit tests (31/31) — no regression; no existing test weakened.The tests place the
QBitcolumn between a leadingInt64and a trailingInt32and assert the whole row, so a wrong byte count / desync is detected; expected values are derived from the input and cross-checked against the independently-correct RowBinary path.Docs / surface
0.11.0-rc1section as feat(client-v2, jdbc-v2): add QBit data type support #2939).docs/features.mdQBitentry updated.QBit)....data_formats.internalpackage (not tracked public API).Pre-PR validation gate
newBinaryFormatReader)client-v2build + focused tests green; no regressiondocs/features.md+CHANGELOG.mdupdated perAGENTS.md/changes_checklist.mdNo backport needed. No server-side change required.