feat(physical-plan): GroupColumn support for List / LargeList - #23648
feat(physical-plan): GroupColumn support for List / LargeList#23648mzabaluev wants to merge 9 commits into
GroupColumn support for List / LargeList#23648Conversation
This is a minimal cherry-pick of the changes in apache#22706, adding GroupColumn support specifically for List and LargeList data types. Co-Authored-By: Qi Zhu <821684824@qq.com>
Saves allocation of a new array.
| for j in 0..lhs_len { | ||
| if !self.child.equal_to(lhs_start + j, &rhs_sublist, j) { | ||
| return false; | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| for j in 0..n { | ||
| self.child.append_val(&sublist, j)?; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23648 +/- ##
=======================================
Coverage ? 80.67%
=======================================
Files ? 1087
Lines ? 366711
Branches ? 366711
=======================================
Hits ? 295859
Misses ? 53250
Partials ? 17602 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| DataType::List(child_field) => { | ||
| let child = make_group_column(child_field.as_ref())?; | ||
| v.push(Box::new(list::ListGroupValueBuilder::<i32>::new( | ||
| Arc::clone(child_field), | ||
| child, | ||
| ))); | ||
| } |
There was a problem hiding this comment.
I'm not sure about using ListGroupValueBuilder::<i32>. What happens when the concatenated length of all the children exceeds i32::MAX? I think ListGroupValueBuilder should drop the O: OffsetSizeTrait generic and always use usize to store its offsets.
There was a problem hiding this comment.
As I understand it, the build/take_n of this builder must produce a ListArray, with i32 offsets. So an overflow would result in an error.
Co-authored-by: Trương Hoàng Long <longtruong2411@gmail.com>
7ee5048 to
553bd2e
Compare
| // First-n offsets: 0, off[1], ..., off[n]. | ||
| let first_n_offsets: Vec<O> = self.offsets[..=n].to_vec(); | ||
|
|
||
| // Remaining offsets shifted so that what was offsets[n] becomes 0. | ||
| // Overwrite the array in place. | ||
| // SAFETY: the write range is at most as large as offsets.len(). | ||
| // Values in the possible overlap are read before being overwritten. | ||
| unsafe { | ||
| let dst = self.offsets.as_mut_ptr(); | ||
| for (i, &off) in self.offsets[n..].iter().enumerate() { | ||
| *dst.add(i) = off - cut_offset; | ||
| } | ||
| } | ||
| self.offsets.truncate(self.offsets.len() - n); |
There was a problem hiding this comment.
@KonaeAkira if we're going into micro-benchmarking territory, the previous code might be better in smaller take case when the remaining vector is drained in place, because it's subtract-while-copying vs. copy, then subtract in place. But this highly depends on vectorization, and the other case benefits from a smaller alloc-and-copy. Maybe there needs to be another split helper with a map closure to address both cases.
There was a problem hiding this comment.
One more thing that just occurred to me: split_vec_min_alloc(&mut self.offsets, n) returns a vector with exact capacity in case n * 2 <= self.offsets.len():
datafusion/datafusion/common/src/utils/mod.rs
Lines 405 to 412 in fb8fe7a
in which case first_n_offsets.push(cut_offset); will always reallocate, which is bad.
Maybe a separate PR can address this. This affects bytes.rs as well.
Post-merge adaptations: Float16 nested type now has `GroupColumn` support. Use RunEndEncoded as the unsupported leaf type case.
3 participants
Which issue does this PR close?
Rationale for this change
This adds column-native
GroupColumnimplementations forList<T>andLargeList<T>, so aGROUP BYcontaining these (along with other supported data types) no longer falls back fromGroupValuesColumntoGroupValuesRows.What changes are included in this PR?
Cherry-picked @zhuqi-lucas's changes in #22706 that pertained to
List/LargeList, and made some optimizations.Are these changes tested?
Cherry-picked the unit tests for the
listmodule from #22706.Added supported/unsupported cases for
Listin thegroup_column_supported_type_matches_make_group_columntest.Benchmarked in a proprietary application using several HashAggregate operations on 20 columns including two string lists, to about 8% cumulative improvement.
Are there any user-facing changes?
No.