Fix client-v2 Native reader misreading multi-row Array columns - #2956
Open
polyglotAI-bot wants to merge 6 commits into
Open
Fix client-v2 Native reader misreading multi-row Array columns#2956polyglotAI-bot wants to merge 6 commits into
polyglotAI-bot wants to merge 6 commits into
Conversation
NativeFormatReader.readBlock() read an Array column's cumulative row offsets but then used the first row's offset as the element count for every row, truncating later rows and desyncing the columns that follow the array in the same block. Compute each row's length from the delta between consecutive offsets instead. Also make BinaryStreamReader.readArrayItem handle len == 0 (empty array rows) without reading a phantom element and indexing a zero-length array, mirroring the existing len == 0 guard in readArray. Fixes: #2955
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
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 682746c. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a client-v2 ClickHouse Native format decoding bug where Array(T) columns in multi-row blocks were misread when row array lengths differed, which could also desync the stream and corrupt subsequent columns.
Changes:
- Corrected Native
Array(T)per-row element counts by computing length deltas from cumulativeUInt64offsets. - Made
BinaryStreamReader.readArrayItem()safely handlelen == 0by returning an empty, correctly-typedArrayValuewithout reading any element bytes. - Added an integration test covering multi-row, non-uniform (including empty) arrays and updated
CHANGELOG.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/NativeFormatReader.java | Fixes per-row array length calculation from cumulative offsets to prevent truncation and stream desync. |
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/BinaryStreamReader.java | Adds an internal guard for empty arrays when readArrayItem() is called directly. |
| client-v2/src/test/java/com/clickhouse/client/query/QueryTests.java | Adds integration coverage for multi-row arrays (non-uniform + empty rows) and validates post-array column correctness. |
| CHANGELOG.md | Documents the Native multi-row array decoding fix and references the issue. |
…row-array-offsets # Conflicts: # CHANGELOG.md
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…row-array-offsets # Conflicts: # CHANGELOG.md
…row-array-offsets # Conflicts: # CHANGELOG.md
…row-array-offsets # Conflicts: # CHANGELOG.md
…row-array-offsets # Conflicts: # CHANGELOG.md
|
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 #2955.
In the ClickHouse Native format an
Array(T)column in a block is encoded as N cumulativeUInt64offsets followed by the flattened elements, so the element count for rowjisoffsets[j] - offsets[j-1](withoffsets[-1] = 0).NativeFormatReader.readBlock()read those offsets into a local array but then passedsizes[0](row 0's offset) as the element count for every row. For results with more than one row and non-uniform array lengths this truncated later rows and, because fewer elements than encoded were consumed, desynced the stream so the columns following the array in the same block were corrupted (typically surfacing asIllegalArgumentException: Non-empty typeName is required). It was accidentally correct only when every row had the same length, which is why existing single-row / uniform tests didn't catch it.Fixing the offset computation exposed a second issue on the same path: for an empty array row the reader now legitimately requests
len == 0, butBinaryStreamReader.readArrayItem(called directly by the Native reader) unconditionally read one element and wrote index 0 — reading a phantom value and throwingArrayIndexOutOfBoundsException.readArrayalready guardslen == 0before callingreadArrayItem; this adds the equivalent guard insidereadArrayItemso the direct Native-reader call is correct too. (Note the old code already crashed this way whenever the first row's array was empty.)Changes
client-v2/.../data_formats/NativeFormatReader.java— read the array column's cumulative offsets and derive each row's element count from the delta between consecutive offsets, instead of reusingsizes[0]for all rows.client-v2/.../data_formats/internal/BinaryStreamReader.java—readArrayItemreturns an empty, correctly-typedArrayValueforlen == 0(via the existingresolveArrayItemClass) instead of reading a phantom element.client-v2/.../query/QueryTests.java— addedtestReadingMultiRowArrays(TestNG@DataProvider).CHANGELOG.md— Bug Fixes entry.Test
testReadingMultiRowArraysqueries a multi-row result with the array column placed in the middle of the schema (id, arr, tag) and a fixed-widthtag Int32after it, so any stream desync shiftstag/subsequent rows and is detected. Whole rows are asserted, with expected values taken from the server. Data-provider cases:main(bug), passes with the fix.Verified: the Native non-uniform case fails on unpatched
main(desync exception; AIOOBE on the empty row once the offset fix alone is applied) and passes with both changes. Existing array/Native coverage stays green — 51 unit tests (BinaryStreamReaderTests,ClickHouseBinaryFormatReaderTest,ClickHouseBinaryFormatReaderNullableArrayTest) and 24 integration array tests (QueryTests,DataTypeTests).Pre-PR validation gate
main, passes on branch)len == 0)Client.newBinaryFormatReader→NativeFormatReader), pinned by an integration test through the real query path@DataProvider, mid-schema field with fixed-width trailing column, no issue refs inside tests)Notes / scope
Kept intentionally to the reported offset bug (one concern per PR). While fixing this I confirmed a separate, deeper pre-existing bug in the same file:
NativeFormatReaderreads array element sub-structures —Array(Nullable(T))null-maps and nestedArray(Array(T))inner-offset levels — with the RowBinary inline convention instead of Native's columnar layout, so those read incorrectly regardless of this change. That needs areadBlockelement-reading rework and is out of scope here; it is being tracked separately.