Skip to content

Aggregate sum of zero valid values is null#8705

Open
mhk197 wants to merge 9 commits into
developfrom
mk/sum-empty-null
Open

Aggregate sum of zero valid values is null#8705
mhk197 wants to merge 9 commits into
developfrom
mk/sum-empty-null

Conversation

@mhk197

@mhk197 mhk197 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

SUM of zero valid values now yields null , as per SQL standard (SQL SUM, ISO 9075-2 §10.9). This replaces he previous "all-invalid sums to zero" contract. Overflow still yields null. This matches arrow-rs (sum returns None for empty/all-null), DuckDB (SumState.isset), and DataFusion (SumAccumulator { sum: Option }).

The blocker for doing this at the root was that a null partial already means overflow and must poison merges, while "empty" must be the merge identity — one nullable scalar can't encode both, and Sum partials cross persisted merge boundaries (zoned layout stats). So SumPartial now carries a seen flag (DuckDB's isset), and the partial dtype becomes {sum, seen}.

Stat::Sum retains original behavior because persisted stat merging requires identity-on-empty.

Side Effects:

  • mean of all-null input is now null
  • DataFusion ColumnStatistics::sum_value is only exported when null_count < row_count
  • Also changed StructGetItemRule to handle struct partials. get_item on a nullable struct with NonNullable/AllValid validity previously returned the raw non-nullable field.

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
@codspeed-hq

codspeed-hq Bot commented Jul 9, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚡ 12 improved benchmarks
❌ 4 regressed benchmarks
✅ 1621 untouched benchmarks
🆕 4 new benchmarks
⏩ 38 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation fsl_sum_small 57.6 µs 82.5 µs -30.2%
Simulation listview_sum_small 73.4 µs 86.8 µs -15.42%
Simulation slice_empty_vortex 339.4 ns 397.8 ns -14.66%
Simulation rebuild_naive 91.3 µs 105.8 µs -13.75%
Simulation bitwise_not_vortex_buffer_mut[128] 244.4 ns 186.1 ns +31.34%
Simulation bitwise_not_vortex_buffer_mut[1024] 304.7 ns 246.4 ns +23.68%
Simulation chunked_varbinview_opt_canonical_into[(1000, 10)] 205.9 µs 168.9 µs +21.87%
Simulation list_sum_large 135.7 ms 111.7 ms +21.49%
Simulation chunked_varbinview_into_canonical[(1000, 10)] 205.9 µs 169.7 µs +21.34%
Simulation listview_sum_large 108.1 ms 89.1 ms +21.34%
Simulation chunked_varbinview_opt_into_canonical[(1000, 10)] 219.7 µs 183.2 µs +19.93%
Simulation bitwise_not_vortex_buffer_mut[2048] 398.6 ns 340.3 ns +17.14%
Simulation encode_varbin[(10000, 2)] 977.1 µs 836.5 µs +16.8%
Simulation list_sum_nullable_elements_large 493.8 ms 427.2 ms +15.57%
Simulation sum_i32_nullable_all_valid 41.5 µs 36.5 µs +13.73%
Simulation list_sum_nullable_elements_medium 4.9 ms 4.3 ms +11.74%
🆕 Simulation standard_sum_f64_all_valid N/A 80.7 µs N/A
🆕 Simulation standard_sum_f64_clustered_nulls N/A 108.1 µs N/A
🆕 Simulation standard_sum_i32_clustered_nulls N/A 105.7 µs N/A
🆕 Simulation standard_sum_i32_nullable_all_valid N/A 82.6 µs N/A

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing mk/sum-empty-null (ae3dad2) with develop (d2b2378)

Open in CodSpeed

Footnotes

  1. 38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@mhk197 mhk197 marked this pull request as ready for review July 9, 2026 23:59
@mhk197 mhk197 requested a review from gatesn July 10, 2026 00:00

@gatesn gatesn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think... with the new Zoned layout, the aggregate partial has become a wire API.

That said, we haven't updated FileStats to use aggregate partials. And I don't believe zone maps use Sum?

A safer option would be to add a separate Sum function with this new behavior and different partial. @robert3005 any thoughts on a preferred direction?

Either way, we should separately make sure we have forward/backward compatibility tests for zoned statistics!

@mhk197 mhk197 changed the title Sum aggregate follows SQL semantics: sum of zero valid values is null Aggregate sum of zero valid values is null Jul 10, 2026
…s and dead code

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
@mhk197 mhk197 added the changelog/fix A bug fix label Jul 10, 2026
@robert3005

Copy link
Copy Markdown
Contributor

we have SUM in zonemaps

@robert3005

Copy link
Copy Markdown
Contributor

@mhk197 I think simpler would be to add another function. Maybe even rename them such that the new sql semantic sum is called sum and the other is called sum_<I have no idea what a good name is>

mhk197 added 5 commits July 10, 2026 14:04
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
…s vortex.sum

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
…ndardSum, used only by list_sum

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
…ns a lazy masked array

Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants