GH-51289: [C++] Add iterator-based bit-run traversal - #51310
akashchamp wants to merge 1 commit into
Conversation
|
|
| bool operator==(std::default_sentinel_t) const { return at_end_; } | ||
| bool operator!=(std::default_sentinel_t) const { return !at_end_; } | ||
| friend bool operator==(std::default_sentinel_t, const BitRunIterator& iterator) { | ||
| return iterator.at_end_; | ||
| } | ||
| friend bool operator!=(std::default_sentinel_t, const BitRunIterator& iterator) { | ||
| return !iterator.at_end_; | ||
| } |
There was a problem hiding this comment.
Why both the friend functions and the instance methods? Probably we don't need both; let's just keep the friend ones?
| bool operator==(std::default_sentinel_t) const { return at_end_; } | ||
| bool operator!=(std::default_sentinel_t) const { return !at_end_; } | ||
| friend bool operator==(std::default_sentinel_t, const SetBitRunIterator& iterator) { | ||
| return iterator.at_end_; | ||
| } | ||
| friend bool operator!=(std::default_sentinel_t, const SetBitRunIterator& iterator) { | ||
| return !iterator.at_end_; | ||
| } |
There was a problem hiding this comment.
Same question as in BitRunIterator.
|
|
||
| TwoSetBitRunIterator(const TwoSetBitRunIterator&) = default; | ||
|
|
||
| TwoSetBitRunIterator& operator=(const TwoSetBitRunIterator& other) { |
There was a problem hiding this comment.
Is it important for this class to be copyable?
| SetBitRunIterator(const SetBitRunIterator&) = default; | ||
|
|
||
| SetBitRunIterator& operator=(const SetBitRunIterator& other) { |
There was a problem hiding this comment.
Is it important for this class to be copyable?
| for (const auto run : range) { | ||
| EXPECT_EQ(run.position, 0); | ||
| ++run_count; | ||
| break; | ||
| } |
|
@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.
| } | ||
| }; | ||
|
|
||
| inline bool operator==(const PositionedBitRun& lhs, const PositionedBitRun& rhs) { |
There was a problem hiding this comment.
I think we may be able to add bool operator==(const PositionedBitRun&) const = default; in the struct definition and don't need to add the two functions. C++20 supports = default for operator== and automatically generates operator!= when operator== is defined (though I am not sure if all our CI compilers support this yet).
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.Are these changes tested?
clang-format --dry-run --Werrorandgit diff --checkarrow-bit-utility-test --gtest_filter='TestSetBitRunReader.Iterate*'(3 tests)arrow-bit-utility-test(217 tests) and its focused CTest entryAre there any user-facing changes?
No public API changes. This adds internal C++ traversal helpers for callers that need iterator control flow.
AI assistance
Assisted-by: Codex
AI assistance was used for the initial implementation and regression tests. I reviewed the final code, kept the change scoped to the requested API, and manually validated the behavior above.