sunchao opened a new pull request, #3748: URL: https://github.com/apache/parquet-java/pull/3748
## 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: | Physical row | `predicate` | `payload` | | --- | ---: | ---: | | 0 | 0 | 0 | | 1 | 0 | 10 | | 2 | 100 | 20 | | 3 | 100 | 30 | | 4 | 0 | 40 | Suppose `predicate` has pages covering rows **[0,1]**, **[2,3]**, and **[4]**, while `payload` has pages covering **[0,1,2]** and **[3,4]**. Filtering for `predicate = 0` eliminates 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, `SynchronizingColumnReader` mistakes 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 `master` at `60175684378abff1ea001b6541ec38da42a2eff1`, 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-column` suite passes **675 tests**, and the existing `TestColumnIndexFiltering` file-reader suite passes **24 tests**. The column reactor's dependency tests and the `spotless:check` formatting check also pass. An additional standalone check writes real files with the page layouts above and reads them through an ordinary `ParquetReader` predicate, 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
