adriangb commented on issue #11234: URL: https://github.com/apache/arrow-rs/issues/11234#issuecomment-5859399040
@alamb, as you asked on https://github.com/apache/arrow-rs/pull/11223#issuecomment-5857360169, I split https://github.com/apache/arrow-rs/pull/11223 (+4,156 lines) into the PRs below. This comment gives the reason for each cut and a suggested review order. # How the split works | Rule | Result | |---|---| | Each PR is useful without the PRs after it | The 4 PRs on `main` fix or speed up the current decoder. Each PR on top of the core adds one improvement to batch mode. | | No PR adds a temporary error or fallback that a later PR removes | Predicates must be in the core PR, because a batch mode without predicates needs a fallback for filtered scans | | Later PRs only add optimizations to code that is already correct | Page release, predicate cache and selection policy are follow-ups | | One decode path | The core has one windowed engine. A scan without predicates is the case with zero predicates. This removed the separate no-predicate path of #11223 and about 600 lines. | # Suggested review order ```text Step 1 (in parallel, all on main) Step 2 Step 3 (in parallel) #11218 test (yours) ─┐ #11235 bug fix #11233 ─┴──────▶ #11238 ──┬──▶ #11239 page release #11236 rebuild releases bytes ├──▶ #11240 predicate cache #11237 sorted PushBuffers └──▶ #11241 selection policy ``` | Step | PR | What users get | New lines | |---|---|---|---:| | 1 | https://github.com/apache/arrow-rs/pull/11218 | A test of the current behavior | +83 | | 1 | https://github.com/apache/arrow-rs/pull/11235 | Fixes https://github.com/apache/arrow-rs/issues/11233: the decoder keeps all pushed bytes until the end of the scan if the caller merges the requested ranges | +311 | | 1 | https://github.com/apache/arrow-rs/pull/11236 | `into_builder().build()` releases the pushed bytes of the row groups that the new decoder skips | +204 | | 1 | https://github.com/apache/arrow-rs/pull/11237 | Sorted `PushBuffers`: `Nbuf/100000ranges` goes from 574 ms to 35 ms | +225 / −41 | | 2 | https://github.com/apache/arrow-rs/pull/11238 | **`FetchGranularity::Batch`**: the first batch of a row group as soon as its pages are pushed | +2,847 (about 1,580 tests) | | 3 | https://github.com/apache/arrow-rs/pull/11239 | Batch mode: peak memory of about one batch, not one row group | +171 | | 3 | https://github.com/apache/arrow-rs/pull/11240 | Batch mode: filtered scans decode the predicate columns one time | +149 | | 3 | https://github.com/apache/arrow-rs/pull/11241 | Batch mode: the configured `RowSelectionPolicy` in each window | +176 | ## Step 2: how to read #11238 It is the largest PR, because the core cannot be smaller without a temporary fallback. About half of it is tests. Suggested reading order: 1. The docs of `FetchGranularity` in `push_decoder/mod.rs`: the behavior that users see, and the I/O contract. 2. The docs of `IncrementalRowGroup` and `Stage` in `reader_builder/incremental.rs`: the design. 3. `IncrementalRowGroup::step` and `ColumnChunkPages`: the engine. 4. The wiring in `reader_builder/mod.rs`, `remaining.rs` and `push_decoder/mod.rs`, which is small. 5. `incremental_tests.rs`: most tests compare batch mode with the default mode on the same scan. The PR description also records two decisions from review: windows stay `batch_size` rows of the row group, and the caller decides how far to read ahead. # Merge notes | PRs | Note | |---|---| | #11235, #11236, #11237 | All edit `util/push_buffers.rs`. The PR that merges last needs a small rebase: remove the duplicate `merge_ranges` and test helper, and update the maximum buffer length of #11237 in `release_ranges` and `retain_ranges`. | | #11239, #11240 | If both merge, the second one adds one rule: a column in the predicate cache is released at cache-batch boundaries | | #11238 and step 3 | I will rebase them after each merge | https://github.com/apache/arrow-rs/pull/11223 stays open as a reference for the full change. I will close it when the stack merges. -- 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]
