Add list_sum expression#8676
Conversation
Merging this PR will degrade performance by 18.28%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
Design for a vortex.list.sum scalar function: survey of DuckDB/DataFusion/ ClickHouse/Polars semantics, reuse of the grouped-sum machinery (GroupedAccumulator), SQL NULL-on-empty semantics for engine pushdown parity, NaN handling via NumericalAggregateOpts, and version-gated DataFusion integration (array_sum absent from the pinned DF 54). Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Implements the vortex.list.sum scalar function per the design doc: sums each list's elements through the grouped aggregate machinery (GroupedAccumulator<Sum>) so widening, overflow, and NaN semantics match sum() exactly, then nulls out empty and all-null lists to follow SQL SUM semantics (matching DuckDB list_sum and DataFusion array_sum). NaN handling is controlled by NumericalAggregateOpts, defaulting to skip_nans like sum(). Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
myrrc
left a comment
There was a problem hiding this comment.
Let's push list_sum down to Duckdb and write a sqllogictest for this (can be in a followup PR). I'd also like to see a performance benchmark query, I'll prepare the machinery for that.
|
|
||
| // Canonicalize upfront because `mask_empty_lists` needs access to list elements validity | ||
| // and sizes. | ||
| let canonical = array.clone().execute_until::<AnyCanonical>(ctx)?; |
There was a problem hiding this comment.
I think we want AnyColumnar here and handle constant/canonical version at once
There was a problem hiding this comment.
Ah, this is handled above in execute
There was a problem hiding this comment.
ok no, the code above will be forced to canonicalise
There was a problem hiding this comment.
ah yeah we want to canonicalize a 1 row constant array, do sum, and then broadcast. will fix
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Adds a
vortex.list.sumscalar function: the sum of each row's list elements, one output row per input row — the equivalent of DuckDB'slist_sum()and DataFusion'sarray_sum().Note that list entries that are empty or all null need to be nullified. However,
GroupedAccumulatorreturns 0 in these cases, so we need to either:GroupedAccumulatorand grouped sum (or even sum) kernels orThe first option is probably best but can be applied in a followup PR.