Repository navigation
[client-v2] Keep row buffers across Native blocks so rows at a block boundary are not lost - #3294
Open
piyush15102003 wants to merge 1 commit into
Open
piyush15102003 wants to merge 1 commit into
piyush15102003 wants to merge 1 commit into
Conversation
…boundary are not lost Closes ClickHouse#3151
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.
Summary
Reading a
Nativeresult that spans more than one block lost two rows at every block boundary: the last row of a block and the first row of the next were returned as all-nullrows, with the row count unchanged and no error.AbstractBinaryFormatReaderreads one row ahead. When the read-ahead starts a new block,NativeFormatReader.readBlock()callssetSchema(), which replaced both row buffers. The read-ahead then wrote the new block's first row into the oldnextRecordarray, andnext()wrapped the freshly allocated, emptycurrentRecord.Changes:
setSchema()allocatescurrentRecord/nextRecordonly when they arenullor the column count differs, so the first call (constructor, RowBinaryWithNamesAndTypes header) still allocates them.next()takes the record reference before the read-ahead, so the returned row does not depend on what the read-ahead does to the fields.Closes #3151
Checklist
NativeFormatReaderBlockBoundaryTest: 25 rows in 10-row blocks vianext()and via getters, and 1-row blocks). They check every value, not only the row count, and all three fail onmain(Lists differ at element [9]: 9 != null).history/latest/3151.md)Note
Medium Risk
Touches core Native/binary row iteration where incorrect behavior silently corrupted values; the fix is narrow and covered by new regression tests.
Overview
Fixes silent data loss when reading multi-block Native query results in client-v2: at every block boundary the last row of one block and the first row of the next were replaced by all-
nullrows while the row count stayed the same.The reader read-aheads one row; starting the next block re-publishes the schema via
setSchema(), which used to always allocate newcurrentRecord/nextRecordbuffers and leftnext()wrapping empty arrays after the read-ahead.setSchema()now reuses those buffers when the column count is unchanged, andnext()captures the row array reference beforereadNextRecord()so returned rows are not tied to buffer swaps during read-ahead.Adds
NativeFormatReaderBlockBoundaryTest(multi-block and 1-row blocks, vianext()and typed getters) and a changelog note for #3151.Reviewed by Cursor Bugbot for commit ed0e317. Bugbot is set up for automated code reviews on this repo. Configure here.