adriangb opened a new pull request, #11613: URL: https://github.com/apache/arrow-rs/pull/11613
# Which issue does this PR close? - Part of https://github.com/apache/arrow-rs/issues/11234. - Alternative to https://github.com/apache/arrow-rs/pull/11238. It follows the design that @alamb proposed in https://github.com/apache/arrow-rs/pull/11238#pullrequestreview-5539250391. The goal is to compare the two designs. Only one of the two PRs should merge. # Rationale for this change The user-facing change is the same as in https://github.com/apache/arrow-rs/pull/11238: with `RequestGranularity::Batch`, `try_decode` returns each batch as soon as its pages are pushed, and does not wait for the full row group. ```rust let mut decoder = ParquetPushDecoderBuilder::try_new_decoder(metadata)? .with_batch_size(100) .with_request_granularity(RequestGranularity::Batch) .build()?; // NeedsData asks for the pages of the first 100 rows only ``` This PR implements it without the new `Incremental` state and without a second page store. # What changes are included in this PR? Batch mode uses the existing states. After each batch, the state goes back to `WaitingOnData` for the pages of the next batch: ```diff Start Filters -> WaitingOnFilterData (full row group, unchanged) StartData - WaitingOnData -> reader for the full row group -> Finished + RowGroup mode: WaitingOnData -> reader for the full row group -> Finished + Batch mode: WaitingOnData { batches: Some(..) } + pages of the next batch pushed? decode one batch + more rows? -> WaitingOnData (pages of the next batch) + else -> Finished - Incremental(IncrementalRowGroup) (removed) ``` The output reader reads from the decoder's `PushBuffers`. Thus, the reader sees the pages that the caller pushes later: ```diff RowGroupReaderBuilder - buffers: PushBuffers + buffers: Arc<Mutex<PushBuffers>> // shared with ColumnChunkData::Shared -PageStore (removed) -IncrementalRowGroup (incremental.rs) (removed) ``` Compared to https://github.com/apache/arrow-rs/pull/11238: | | https://github.com/apache/arrow-rs/pull/11238 | This PR | |---|---|---| | Predicates | `batch_size` rows at a time, overlap with output | Full row group first, as in `RowGroup` mode | | Predicate cache | No (https://github.com/apache/arrow-rs/pull/11240 adds it) | Yes, the existing cache | | Configured `RowSelectionPolicy` | No (https://github.com/apache/arrow-rs/pull/11240 adds it) | Yes | | Bytes | Copied from `PushBuffers` into `PageStore` | One `PushBuffers` | | Lines in `src/` | about +1,500 | about +770 | Also, from the review comments: - `FetchGranularity` is now `RequestGranularity`. - The I/O and memory docs are on `ParquetPushDecoder`. The `RowGroup` docs say that the decoder buffers all pages of the row group. - The offset index requirement uses the suggested words. # Are these changes tested? Yes, with the tests of https://github.com/apache/arrow-rs/pull/11238 (`push_decoder_equivalence.rs` and `push_decoder_incremental.rs`). The tests use only the public API, so they did not change. The 2,000 `proptest` scans pass: batch mode returns the same batches as row-group mode. One test is ignored: `test_predicates_release_bytes_pushed_ahead`. It expects predicates that read one page at a time. In this design, a predicate requests its full column chunk as one range. If the caller pushed the chunk one page at a time, the request is not satisfied and the decoder requests the chunk again. `RowGroup` mode has the same behavior on `main`. Bytes and pushes before the first batch, for the test file (row group 0 has 39,098 bytes): | Predicate | https://github.com/apache/arrow-rs/pull/11238 | This PR | |---|---|---| | `b % 2 != 0` | 13,402 bytes, 4 pushes | 13,822 bytes, 2 pushes | | `a >= 550` | 8,182 bytes, 7 pushes | 8,182 bytes, 2 pushes | The difference grows if a predicate column is large compared to one batch, for example a predicate on a wide string column. Not done yet: benchmarks. `RowGroup` mode now locks a mutex for each access to the buffers. `push_decoder/decode/row_group` must show no change. # Are there any significant user-facing changes? There are no breaking changes. New opt-in API: `RequestGranularity` and `ParquetPushDecoderBuilder::with_request_granularity`. ## Merge danger **Door:** two-way. The API is opt-in and `#[non_exhaustive]`. Before a release, the internals can change without a user-visible effect. **Blast radius:** medium. The default `RowGroup` path now accesses its buffers through a mutex. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
