feat(physical-plan): GroupColumn support for List / LargeList - #23648
feat(physical-plan): GroupColumn support for List / LargeList#23648mzabaluev wants to merge 22 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 81.62% 81.65% +0.03%
==========================================
Files 1123 1124 +1
Lines 409637 410327 +690
Branches 409637 410327 +690
==========================================
+ Hits 334383 335070 +687
- Misses 55624 55629 +5
+ Partials 19630 19628 -2 ☔ 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.
github-actions
Bot
added
auto detected api change
MSRV doesn't let us use `if let`, but instead the whole function can be rewritten to not use a vector to collect the single element (which was asserted to always be the case) and DRY on the error path.
|
We can create a benchmark PR target this PR first, so we can compare it in CI benchmark. |
This comment was marked as resolved.
This comment was marked as resolved.
Use element ranges to look at the values.
The micro-benchmark I have added in #24824 shows 19-35% improvement on minimalistic cases. |
Benchmark to accompany apache#23648.
|
run benchmark multi_group_by |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing nested-group-column-for-lists (559e774) to 80ae9e5 (merge-base) diff Run configurationrun benchmark multi_group_byResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark multi_group_by |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing nested-group-column-for-lists (c701d04) to da89c7c (merge-base) diff Run configurationrun benchmark multi_group_byResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark multi_group_by |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing nested-group-column-for-lists (c701d04) to da89c7c (merge-base) diff Run configurationrun benchmark multi_group_byResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark multi_group_by |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing nested-group-column-for-lists (c701d04) to da89c7c (merge-base) diff Run configurationrun benchmark multi_group_byResults will be posted here when complete File an issue against this benchmark runner |
|
@adriangb Do you have any idea why it is unable to run the benchmarks? |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
|
run benchmark multi_group_by |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing nested-group-column-for-lists (6f73226) to 3a4c310 (merge-base) diff Run configurationrun benchmark multi_group_byResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
…un failed (#29) * runner: enable a criterion target's required-features `run_criterion_side` hardcoded `--features=parquet`, so any bench target declaring `required-features` was unrunnable. Cargo does not skip such a target, it refuses the whole invocation: error: target `multi_group_by` in package `datafusion-physical-plan` requires the features: `test_utils` Both sides fail identically, which is what apache/datafusion#23648 hit. The baseline failure was swallowed by the "new bench?" fallback and the branch failure ended the run. `cargo metadata` already reports `required-features` per target, on the same call that decides criterion-vs-bench.sh routing, so carry them through and append them to the feature list. `parquet` stays in front: it is what every target has been run with, six targets require it by name, and no target rejects it. The run is invoked from the workspace root, where cargo resolves each bare feature name against the members declaring it, so features owned by different packages list together. Against current `apache/datafusion` this makes 14 further targets runnable, including `aggregate_vectorized`, which the README already lists as working: crypto, datetime_expressions, dictionary_encoding, encoding, hash_join_semi_anti, map_query_sql, math_expressions, multi_group_by, regex_expressions, sort_merge_join, string_expressions, unicode_expressions Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * controller: quote the runner's log in a terminal failure comment The comment a terminal Kubernetes failure posts named the reason and nothing else, so a benchmark that failed on its own terms reached the PR as a bare `BackoffLimitExceeded`. On apache/datafusion#23648 that was a missing cargo feature, and the person who triggered the run had no way to learn it: the reason describes the pod's exit, never its cause, and the Job's TTL deletes the pod before `kubectl logs` would help even for someone with cluster access. The controller already holds a Kubernetes client and reconciles the failure, so read the last 40 lines of the failed pod's log there and lead the comment's details with them, ahead of the Kubernetes message. Newest pod wins: `backoffLimit` is 0, so there is normally one, but a preempted spot node can leave an earlier pod behind describing a different run. Log output is pod output, so it gets the fence widening the Kubernetes message already gets, and a benchmark printing a fence cannot inject markdown into the comment. Reading it is best effort throughout: a missing pod or an API error drops the section rather than the notification, which is the one thing a failed run must still produce. The controller Role gains `pods` and `pods/log`; it held only `batch/jobs`. Separately, the runner's own top-level handler logged `{}` on an `anyhow::Error`, printing just the outermost context ("run multi_group_by (branch, criterion)") and dropping the command's stderr below it. `{:#}` walks the chain. The baseline path already used it, which is the only reason the real message was recoverable from Cloud Logging at all. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
let's see if adriangb/datafusion-benchmarking#29 fixes it |
7 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.