-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-20351: [C++] Kernel input type matcher for run-end encoded types #34503
Conversation
e565a55
to
591b22a
Compare
Lead-authored-by: Tobias Zagorni <tobias@zagorni.eu> Co-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
b4e986f
to
1601f74
Compare
Iterator iterator(int64_t logical_pos) const { | ||
assert(logical_pos < length()); | ||
return iterator(logical_pos, PhysicalIndex(logical_pos)); | ||
return iterator(logical_pos, logical_pos < length() | ||
? PhysicalIndex(logical_pos) | ||
: RunEndsArray(array_span).length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this caught by a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use this class for something new and I realized this assert was too restrictive: it's not necessary for the correct working of the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PhysicalIndex(logical_pos)
triggers an assert if a bad index is passed, so that's why I have the conditional now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Benchmark runs are scheduled for baseline = dbc1e90 and contender = 9c218ab. 9c218ab is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Rationale for this change
This is necessary to be able to specify dispatching of filters based on run-end encoded array (upcoming PR).
Are these changes tested?
Yes. By unit tests.