haohuaijin commented on PR #10141: URL: https://github.com/apache/arrow-rs/pull/10141#issuecomment-5015752318
Thanks for the thorough review @hhhizzz — I verified all six findings and they are accurate. The three blocking items are fixed in [496d7cb](https://github.com/apache/arrow-rs/pull/10141/commits/496d7cb62a2aff8064ac35c2bcdc5d682e78d291); [325d774](https://github.com/apache/arrow-rs/pull/10141/commits/325d774420a49430e05399f401d7945ee93901e1) extends the `row_selection_cursor` bench with a `read_auto` mode and mask-backed inputs. ## 1. Regression — fixed Profiling showed the regression was entirely in the selector→bitmap conversion: after the module split, LLVM stopped inlining `BooleanBufferBuilder::append_n` there (self time 3.21% → 5.43%, matching the ~2% regression); the decode loop profiles identically on both sides. `boolean_mask_from_selectors` now fills the bitmap bytes directly instead of calling `append_n` per selector, with a fuzz test asserting bit-exact equivalence. L4–L40 matrix, 10141-auto [325d774](https://github.com/apache/arrow-rs/pull/10141/commits/325d774420a49430e05399f401d7945ee93901e1) vs main `ee30b61` (medians, Apple M2 Max): <img width="1003" height="390" alt="image" src="https://github.com/user-attachments/assets/d4ec292f-1fde-423a-86fc-fbb8f0ce83ec"; /> Both forced paths are now faster than the main at every point, and the Selectors/Mask crossover (between L12 and L16 on this machine) is unchanged by this PR. ## 2. Dense-mask trimming — fixed `trim_mask` no longer popcounts or walks set bits: it fast-paths a set final bit, otherwise reverse-scans bytes for the last set bit. Near-dense masks are now O(1) to trim. Fuzz-tested including sliced (non-zero offset) masks. ## 3. Repeated popcounts — fixed `MaskSelection` caches the selected count in a `OnceLock<usize>`. `split_off` popcounts only the head and derives the tail by subtraction, so the row-group frontier is O(bitmap) total instead of O(row_groups × bitmap); `trim`/`offset`/`limit` propagate the cache, and `Clone` keeps it so `peek_next_row_group`'s frontier clone answers `row_count()` for free. Unit tests cover the propagation. ## 4–6 These are valid findings as well. To keep this PR from growing too large I'd like to address them in follow-up PRs, and I'll file issues for them so they don't get lost. -- 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]
