haohuaijin opened a new pull request, #10443: URL: https://github.com/apache/arrow-rs/pull/10443
# Which issue does this PR close? - Closes #10422. # Rationale for this change Follow up to #10141, which made `RowSelection` optionally backed by a `BooleanBuffer`. For mask-backed selections, `RowSelection::iter()` lazily materializes an RLE selector cache inside `MaskSelection` (a `OnceLock<Vec<RowSelector>>`). Every path that converted such a selection into `RowSelector`s ignored that cache and called `mask_to_selectors()` again, so a caller that iterates a selection and then consumes it run-length-encodes the whole bitmap twice. For a large selection that is a wasted O(bitmap) pass plus a second selector `Vec` held at the same time. # What changes are included in this PR? - `MaskSelection::into_selectors` takes the `OnceLock`'s value when it was populated and otherwise falls back to `mask_to_selectors`. It is used by `RowSelection::into_selectors_vec` (which backs `From<RowSelection>` for both `Vec<RowSelector>` and `VecDeque<RowSelector>`), and by `build_cursor` when lowering a mask-backed selection onto the selector cursor. - `MaskSelection::borrowed_selectors` does the same for the borrowing callers in `intersection` and `union`. It returns `Cow::Borrowed` when the cache is populated and otherwise converts into a temporary, rather than populating the cache of a selection the caller still owns. - `union` now matches on `(&self.inner, &other.inner)` like `intersection` does, instead of nesting two matches. - `mask_to_selectors` no longer has callers outside the `selection` module, so its `pub(crate)` re-export is dropped. # Are these changes tested? Yes, four new unit tests in `arrow_reader::selection::boolean`: - `test_into_selectors_takes_the_iter_cache` compares the `Vec`'s pointer before and after the conversion, asserting the cache is moved out rather than re-encoded. - `test_into_selectors_without_cache_still_converts` covers the cold-cache fallback and the `VecDeque` conversion. - `test_borrowed_selectors_reuses_cache_without_populating_it` asserts `Cow::Owned` when cold and `Cow::Borrowed` when warm. - `test_set_algebra_agrees_whether_or_not_the_cache_is_populated` checks `intersection`/`union` agree in both cache states. `cargo test -p parquet --all-features` passes. ## Benchmarks `row_selector` gains two scenarios over a mask-backed selection of 300k rows, across alternating run lengths and a random mask: `mask_iterate_then_consume` iterates the selection and then converts it, matching what `ParquetAccessPlan::into_overall_row_selection` does; `mask_consume` converts without iterating first, so it measures the cold-cache path. ``` cargo bench -p parquet --bench row_selector -- mask_ ``` | benchmark | main | this PR | change | | --- | --- | --- | --- | | `mask_iterate_then_consume/run01` | 2.237 ms | 1.202 ms | -46.5% | | `mask_iterate_then_consume/run04` | 272.5 µs | 132.4 µs | -51.4% | | `mask_iterate_then_consume/run16` | 73.7 µs | 32.5 µs | -55.9% | | `mask_iterate_then_consume/run64` | 26.0 µs | 13.6 µs | -47.8% | | `mask_iterate_then_consume/random` | 784.1 µs | 404.1 µs | -48.7% | The cold-cache `mask_consume` cases are unchanged. Their medians over three runs per side land within 1.5% of `main`, which is inside the run-to-run spread `main` shows on its own. `arrow_reader_clickbench`, `row_selector_boolean_buffer` and the rest of `row_selector` show no change beyond noise: a control run of the same binary against the same baseline drifts by about as much as the comparison run does. # Are there any user-facing changes? No. All of the added methods are internal, and the public API and its behavior are unchanged. -- 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]
