Conversation
alamb
changed the title
ListArraysconcat for sliced ListArrays
arrow-select/src/concat.rs
Outdated
There was a problem hiding this comment.
The issue is that the values of sliced ListArrays do not necessairly start at zero
For the test in this PR, the offsets are:
concat_lists offsets: OffsetBuffer(ScalarBuffer([5, 5, 5]))
concat_lists offsets: OffsetBuffer(ScalarBuffer([0, 0, 3, 4]))
arrow-select/src/concat.rs
Outdated
There was a problem hiding this comment.
While I was futzing with this, I figured I would avoid slice on the values unless it was necessary.
I can make this code simpler if we always slice but that will always make an ArrayRef clone
There was a problem hiding this comment.
Is slice really expensive? Isn't it zero copy as the actual values (everything that is using Buffer) are behind arc?
There was a problem hiding this comment.
I don't think they are very expensive (they are several Arc::clone)
There was a problem hiding this comment.
Given this isn't a hot loop, it is invoked once per input array, I suspect the overheads of slice would likely be irrelevant, but this is also fine
alamb
force-pushed
the
alamb/list_array_sliced
branch
from
d87a00c to
6c6c992
Compare
| Some(vec![Some(10)]), | ||
| ]; | ||
| let list1_array = ListArray::from_iter_primitive::<Int64Type, _, _>(list1.clone()); | ||
| let list1_array = list1_array.slice(1, 2); |
There was a problem hiding this comment.
I would add assertion here that the offsets really don't start from 0 to make sure we actually cover that case
There was a problem hiding this comment.
Good idea. Thank you for the suggestion. done in e22760e 👍
tustvold
left a comment
tustvold
left a comment
There was a problem hiding this comment.
Looks good to me, thank you
arrow-select/src/concat.rs
Outdated
There was a problem hiding this comment.
Given this isn't a hot loop, it is invoked once per input array, I suspect the overheads of slice would likely be irrelevant, but this is also fine
Co-authored-by: Raz Luvaton <[email protected]>
|
I plan to merge this tomorrow morning and make a 54.1.0 release candidate |
|
Merging and will make a release candidate shortly |
3 participants
Which issue does this PR close?
ListArrays is broken #7034Rationale for this change
Fixes a regression introduced in
concatkernel will merge dictionary values for list of dictionaries #6893What changes are included in this PR?
I also tested that this fixes the issue I found downstream in
54.1.0datafusion#14328Are there any user-facing changes?
Fix a bug