-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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-32947: [Go] Implement Concatenate for REE Array #14126
Conversation
zeroshade
commented
Sep 14, 2022
•
edited by github-actions
bot
Loading
edited by github-actions
bot
- Closes: [Go] Concatenate Implementation for REE Array #32947
CC @zagto |
fb90e87
to
aa29f37
Compare
bccb88f
to
230ff03
Compare
|
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.
nit: PR/Issue should be "REE" array right?
go/arrow/array/concat.go
Outdated
|
||
physicalLength, overflow = addOvf(physicalLength, int(plen)) | ||
if overflow { | ||
return nil, fmt.Errorf("%w: run length encoded array length must fit into a 32-bit signed integer", |
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.
Hmm, shouldn't this depend on the offset type?
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 suppose there are separate checks below, and since lengths are limited to 32-bit...
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.
so, this is more of a short circuit. We have to add up all of the physical lengths so we know the size to allocate for the full concatenated array. If we end up overflowing when adding up the physical length we don't have to bother calculating the new run ends, we know we're going to overflow no matter what the offset type is. So this doesn't need to depend on the offset type.
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.
Fair enough. I suppose you could short circuit here earlier for int16, but as long as it properly fails either way.
I'll merge this at EOD if no one else has comments |
Benchmark runs are scheduled for baseline = 4a1229c and contender = 3b21e0c. 3b21e0c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
* Closes: apache#32947 Authored-by: Matt Topol <zotthewizard@gmail.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>
* Closes: apache#32947 Authored-by: Matt Topol <zotthewizard@gmail.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>