GH-3747: Preserve the final selected row when skipping pages - #3748
Open
sunchao wants to merge 1 commit into
Open
GH-3747: Preserve the final selected row when skipping pages#3748sunchao wants to merge 1 commit into
sunchao wants to merge 1 commit into
Conversation
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.
Why are the changes needed?
A Parquet file stores each column in separate data pages, and the page boundaries do not have to line up across columns. When column-index filtering rules out pages for a predicate, the record reader must advance every projected column to the same retained row positions. Otherwise it can assemble a record using values from different rows.
The current reader can lose that synchronization at the final selected row. For example, consider a row group with these two required INT32 columns:
predicatepayloadSuppose
predicatehas pages covering rows [0,1], [2,3], and [4], whilepayloadhas pages covering [0,1,2] and [3,4]. Filtering forpredicate = 0eliminates predicate page [2,3], so the reader needs positions 0, 1, and 4. It should return payloads [0, 10, 40]. With column-index filtering enabled, it instead returns [0, 10, 20]: the last payload comes from row 2, not row 4.The payload reader has already taken row 4 from the selected-position iterator, but has not yet reached that row. When it needs to move to the next page,
SynchronizingColumnReadermistakes the exhausted iterator for the end of the read and stops on the earlier page. Disabling column-index filtering gives the correct result for the same file.What changes were proposed in this PR?
Keep the synchronizing reader active while its final selected row is still pending. Iterator exhaustion is only sufficient to finish once the current target has been reached, or the reader has explicitly recorded that no target remains. This lets the existing page-advance and value-skipping logic reach the last selected row without changing how other rows are read.
The change is confined to that completion check, with regression coverage in the existing column-reader test class. It does not change public APIs, the Parquet format, or how pages are selected for filtering.
How was this PR tested?
Validated locally against Apache
masterat60175684378abff1ea001b6541ec38da42a2eff1, with Java 17 and Thrift 0.23.0. All four added regression tests fail before the fix with an earlier row's value and pass afterward. They cover optional and repeated columns, V1 and V2 pages, null or empty rows, and selections ending at different positions within a page. The three pre-existing tests in the class remain passing.The full
parquet-columnsuite passes 675 tests, and the existingTestColumnIndexFilteringfile-reader suite passes 24 tests. The column reactor's dependency tests and thespotless:checkformatting check also pass.An additional standalone check writes real files with the page layouts above and reads them through an ordinary
ParquetReaderpredicate, without supplying row positions. It reproduces the wrong payload with the old reader and returns the correct values with this fix, for both V1 and V2 pages. Aligned-page and final-contiguous-range controls remain correct. This check is separate from the committed unit tests.Closes #3747.