GH-51289: [C++] Add iterator-based bit-run traversal - #51310
akashchamp wants to merge 2 commits into
Conversation
|
|
|
@akashchamp Is it possible to find potential call sites in the codebase that would benefit from these constructs? @HuaHuaY Perhaps you'll be interested in taking a look at this. |
github-actions
Bot
added
awaiting committer review
HuaHuaY
left a comment
HuaHuaY
left a comment
There was a problem hiding this comment.
I submit a suggestion first and I’ll review the PR in detail later. Please rebase main — that should fix some of the random CI errors.
Address review feedback on the iterator-based bit-run traversal: - Keep a single friend operator==(iterator, std::default_sentinel_t) per iterator and let C++20 rewritten comparisons provide operator!= and the reversed argument order, instead of also declaring instance methods. - Give PositionedBitRun a defaulted operator== instead of hand-written free operator==/operator!=. - Drop the const qualifier on BaseSetBitRunReader::length_ so the reader is assignable; this lets SetBitRunIterator and TwoSetBitRunIterator use the implicitly generated copy/move special members and removes the hand-written copy assignment operators and CopyReader helper. - Remove a range-for/break block from TestSetBitRunReader.IterateBitRuns that only exercised language semantics already covered by the range.begin() checks that follow it.
2691d37 to
32a63cc
Compare
|
|
|
Yes, there are several hand-rolled NextRun loops that would collapse into a range-for. The clearest ones: cpp/src/arrow/compare.cc:685 (RangeDataEqualsImpl) loops while(true) on SetBitRunReader::NextRun, checks length == 0 to return, and sets result_ = false on the first mismatch, which becomes for (const auto run : IterateSetBitRuns(...)) with an early return; cpp/src/arrow/compute/kernels/pivot_internal.cc:134 searches for the first unset validity bit by accumulating null_pos += run.length, and with IterateBitRuns the PositionedBitRun already carries the position so it is a find-first with break; cpp/src/arrow/array/concatenate.cc:616, cpp/src/arrow/compute/kernels/scalar_if_else.cc:1136 and :1155, and cpp/src/arrow/compute/kernels/hash_aggregate_numeric.cc:652 all keep a manual position counter and an if (run.length == 0) break sentinel check around BitRunReader that IterateBitRuns makes unnecessary; cpp/src/arrow/util/list_util.cc:75 (MinViewOffset) has the same while(true) shape around SetBitRunReader. I have kept this PR to the new helpers plus tests so it stays easy to review and would convert those sites in a follow-up, but I am happy to do compare.cc here if you would rather see one real caller in this PR. |
|
@github-actions crossbow submit -g cpp |
|
Revision: 32a63cc Submitted crossbow builds: ursacomputing/crossbow @ actions-7417ccf146
|
3 participants
Fixes #51289
Rationale for this change
The callback-based bit-run visitors require callers to route early termination through callback status handling. These ranges allow normal iterator control flow, including breaking once a caller has found the run it needs.
What changes are included in this PR?
IterateBitRuns,IterateSetBitRuns, andIterateTwoSetBitRunsas C++20 input ranges.BitRuntype and introducePositionedBitRunfor full bit runs so iterator values expose position, length, and set state.friend operator==(iterator, std::default_sentinel_t)and relies on C++20 rewritten comparisons for!=and the reversed order;PositionedBitRunuses a defaultedoperator==.BaseSetBitRunReader::length_is no longerconst, so the reader (and the iterators wrapping it) get compiler-generated copy/move special members instead of hand-written ones.Are these changes tested?
clang-format --dry-run --Werror(clang-format 18.1.8, as pinned in.pre-commit-config.yaml) andgit diff --checkarrow-bit-utility-test --gtest_filter='*BitRun*:*Bitmap*:*BitUtil*'on macOS (Apple clang 17, Debug build): 147 tests from 19 suites pass, including the newTestSetBitRunReader.IterateBitRuns,IterateSetBitRunsandIterateTwoSetBitRunsstd::ranges::input_range,std::input_iterator,std::sentinel_for<std::default_sentinel_t, ...>andstd::copyablefor all three iterators, and exercisesit == end,end == it,it != end,end != it.Are there any user-facing changes?
No public API changes. This adds internal C++ traversal helpers for callers that need iterator control flow.
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by:
Assisted-by: Codex, Claude
AI assistance was used for the initial implementation and regression tests, and for the review follow-ups (the comparison-operator cleanup and the removal of the hand-written copy members). I reviewed the final code, kept the change scoped to the requested API, and validated the behavior above.