feat(client-v2, jdbc-v2): add QBit data type support#2939
Open
polyglotAI-bot wants to merge 4 commits into
Open
feat(client-v2, jdbc-v2): add QBit data type support#2939polyglotAI-bot wants to merge 4 commits into
polyglotAI-bot wants to merge 4 commits into
Conversation
QBit(element_type, dimension) is transmitted over RowBinary exactly like Array(element_type) - a var-int length followed by that many element values - so it is read and written as a Java array of its element type (float[] for BFloat16/Float32, double[] for Float64) reusing the existing array codec, while keeping dataType == QBit for metadata fidelity. In jdbc-v2 QBit maps to java.sql.Types.ARRAY and is exposed as a java.sql.Array. Element types: BFloat16, Float32, Float64. The bit-transposed on-disk layout is a server storage detail and is not exposed to the client. Implements: #2610
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
QBit(element_type, dimension) is a fixed-size vector: the server requires exactly `dimension` elements and rejects any other length over RowBinary with a late SERIALIZATION_ERROR (verified against server 26.5). The write path reused serializeArrayData, which wrote the actual array/list length without checking it, so a wrong-sized (including empty) vector was serialized like a normal Array and only failed on the server after a round-trip. Split the QBit case out of the Array fall-through into serializeQBitData, which validates the element count against column.getPrecision() (the dimension) and throws a clear IllegalArgumentException naming the column, expected dimension, and actual length before delegating to serializeArrayData. This mirrors the client-side length enforcement already applied to the other fixed-size type, FixedString(N). A correctly-sized QBit is still byte-for-byte identical to Array(element_type). Addresses Cursor Bugbot review on PR #2939.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
A non-null QBit value that is neither a Java array nor a List fell through serializeQBitData to serializeArrayData, which writes no bytes for such a value, desynchronizing the RowBinary stream so the following columns are misread or rejected by the server. Reject it up-front with a clear IllegalArgumentException, consistent with the existing fixed-dimension validation and the Geometry reject-unsupported path. Addresses Cursor Bugbot review on #2939.
Member
|
@cursor review |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for ClickHouse’s experimental QBit(element_type, dimension) vector type across the shared type parser (clickhouse-data), the client-v2 RowBinary reader/writer, and jdbc-v2 type mapping and array handling, with integration/unit test coverage and documentation updates.
Changes:
- Parse
QBit(E, N)inclickhouse-dataas a one-level “array-like” type (nested element column + dimension retained as precision). - Reuse existing
Array(element_type)RowBinary read/write paths forQBitinclient-v2, including parameter string rendering as an array literal. - Expose
QBitasTypes.ARRAYinjdbc-v2and ensure JDBCArraymetadata reports the element type (not the fullQBit(...)type).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/test/java/com/clickhouse/jdbc/JdbcDataTypeTests.java | Adds JDBC integration test for QBit mapping, PreparedStatement write, and ResultSet read/metadata. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/types/Array.java | Treats QBit as array-nested for base type/name reporting via getArrayNestedLevel(). |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcUtils.java | Maps ClickHouseDataType.QBit to JDBCType.ARRAY. |
| docs/features.md | Documents QBit wire contract/behavior and JDBC mapping expectations. |
| client-v2/src/test/java/com/clickhouse/client/datatypes/DataTypeTests.java | Adds client-v2 integration round-trip tests for QBit with Float32/Float64/BFloat16, plus DDL settings support in helper. |
| client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java | Adds serializer-level validation tests for QBit dimension and input type. |
| client-v2/src/main/java/com/clickhouse/client/api/internal/DataTypeConverter.java | Renders QBit parameters as array literals by reusing the Array path. |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java | Adds QBit serialization that validates fixed dimension and type, then delegates to array serialization. |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java | Reads QBit by reusing Array decoding. |
| clickhouse-data/src/test/java/com/clickhouse/data/ClickHouseColumnTest.java | Adds unit tests for QBit parsing (element type + dimension) and missing-dimension failure case. |
| clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseColumn.java | Adds QBit keyword parsing, nested element column setup, and dimension retention in precision. |
| CHANGELOG.md | Adds release note entry for QBit support and behavior. |
…eject null QBit, await DDL in test Review response on PR #2939: - NativeFormatReader: reject reading any column that is or contains a QBit (recursive check). The server transmits QBit in the Native format using its internal bit-transposed Tuple(FixedString(...)) layout, not the Array-like RowBinary representation this reader decodes, so the previous fall-through misread the bytes and desynchronized the block. Fail loud with a clear ClientException pointing to a RowBinary format instead of decoding garbage. - SerializerUtils.serializeQBitData: reject a null value. A fixed-dimension QBit cannot be null; a QBit nested in a container (Tuple/Map/Array) reaches this path via serializeNestedData without the top-level preamble, so without the guard the null was written as a zero-length vector, desynchronizing the stream. - DataTypeTests.writeReadVerify: await client.execute(tableDef) in the no-ddl branch (it was unawaited, racing the subsequent getTableSchema). - Tests: Native rejection (top-level + nested Map(String,QBit)); null QBit rejection (direct + nested-in-Tuple). docs/features.md + CHANGELOG updated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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 557768b. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
client-v2/src/main/java/com/clickhouse/client/api/data_formats/NativeFormatReader.java:116
- In NativeFormatReader's Array handling, the per-row offsets are read into
sizes, butreadArrayItem(...)is always invoked withsizes[0]. This makes every row read the same element count and can desynchronize the Native stream for multi-row results. In ClickHouse Native format, Array columns store cumulative offsets, so the per-row length should be computed as a difference between successive offsets.
if (column.isArray()) {
int[] sizes = new int[nRows];
for (int j = 0; j < nRows; j++) {
sizes[j] = Math.toIntExact(binaryStreamReader.readLongLE());
}
for (int j = 0; j < nRows; j++) {
values.add(binaryStreamReader.readArrayItem(column.getNestedColumns().get(0), sizes[0]));
Collaborator
Author
|
Thanks @mshustov for the re-review. Addressing the one low-confidence item Copilot surfaced (suppressed) on Verdict: it's a genuine bug, but it is pre-existing and unrelated to this QBit PR — so I'm scoping it out of this PR and tracking it for a separate fix. Not introduced here. The concern is real for multi-row Line 116 calls Suggested fix (per-row length = successive-offset difference): for (int j = 0; j < nRows; j++) {
int len = sizes[j] - (j == 0 ? 0 : sizes[j - 1]);
values.add(binaryStreamReader.readArrayItem(column.getNestedColumns().get(0), len));
}Since this is independent of the QBit feature and lives in the general Array read path, I'm keeping this PR focused on QBit (one concern per PR) and will handle the |
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.
Adds read/write support for the experimental
QBit(element_type, dimension)vector data type (ClickHouse25.10+) toclient-v2andjdbc-v2.QBitwas previously only an unimplemented type constant (#2646), and every data-type test skipped it with// TODO: add support.Supported element types:
BFloat16,Float32,Float64(the server rejects any other element type). Theallow_experimental_qbit_typeserver setting is required only to create aQBitcolumn — insert/select on an existing column do not need it.Design
The key finding (verified byte-for-byte against a live ClickHouse
26.5server): aQBit(element_type, dimension)value is transmitted overRowBinaryexactly likeArray(element_type)— a var-int length prefix followed by that many little-endian element values. The bit-transposedTuple(FixedString(...))layout referenced in the issue is a MergeTree on-disk storage detail, not the wire format; on the wire the server presents (and accepts) the logical vector as an array, and reports the column type string asQBit(Float32, 8).So
QBitis implemented as a one-level array of its element type, reusing the existing, well-tested array code paths, while keepingdataType == QBitfor metadata fidelity:clickhouse-dataClickHouseColumn): aQBitbranch parses(element_type, dimension), builds the nested element column, setsarrayLevel = 1/arrayBaseColumn, and keeps the dimension as the columnprecision.client-v2BinaryStreamReader):case QBitreuses theArrayread (readArray).client-v2SerializerUtils):case QBitreusesserializeArrayData.float[]forBFloat16/Float32,double[]forFloat64— identical toArray(element_type), through generic records and POJO binding.QBitmaps tojava.sql.Types.ARRAY;getObject/getArrayreturn ajava.sql.Arrayof the element type, andPreparedStatementbinding renders it via the array-literal path (DataTypeConverter).Entry points covered:
client-v2read + write (generic + POJO);jdbc-v2read + write; the shared type parser.Changes
clickhouse-data/.../ClickHouseColumn.java—KEYWORD_QBIT;QBitparse branch;QBitcase inupdate().client-v2/.../BinaryStreamReader.java—case QBit→ array read.client-v2/.../SerializerUtils.java—case QBit→ array write.client-v2/.../internal/DataTypeConverter.java—case QBit→ array-literal rendering (JDBC/SQL param path).jdbc-v2/.../internal/JdbcUtils.java—QBit→JDBCType.ARRAY.jdbc-v2/.../types/Array.java— resolve the array element column viagetArrayNestedLevel() > 0(wasisArray(), which isfalseforQBit) sogetBaseType()/getBaseTypeName()report the element type, not the wholeQBittype.docs/features.md,CHANGELOG.md— documentation.Test
ClickHouseColumnTest(unit,@DataProvider): parsingQBit(E, N)for all three element types (dataType, element column, dimension) + a negative missing-dimension case.DataTypeTests(client-v2 integration): full client write → server → read round-trip forFloat32/Float64/BFloat16. TheQBitcolumn sits mid-schema with a trailingInt32column, so a codec byte-misalignment would shift it and fail the assertion.JdbcDataTypeTests(jdbc-v2 integration):PreparedStatementwrite +ResultSetread; assertsgetColumnType == ARRAY,Array.getBaseType == FLOAT/getBaseTypeName == "Float32", and the element values.Regression: full
clickhouse-dataunit suite (1660) and the existingArrayintegration tests in both modules pass unedited.Notes
QBitnested insideVariant/Dynamic— the server support there is experimental and the pre-existing tests already skip those combinations; left out of scope.nullinto a non-nullableQBitcolumn throws a clear client-side error (rather than emitting an empty vector asArraydoes), which is intentional givenQBit's fixed dimension — an empty vector is never valid.Pre-PR validation gate
Array(element_type);QBit→ARRAYlikeGeometry/Point)docs/features.md(client-v2 + jdbc-v2) andCHANGELOG.mdupdatedAGENTS.md/docs/features.md/docs/changes_checklist.md