Skip to content

Conversation

@coderfender
Copy link
Contributor

@coderfender coderfender commented Oct 17, 2025

Which issue does this PR close?

Closes #531 .
(Partially closes 531) . The ANSI changes to support AVG will be tracked in a new PR

Rationale for this change

DataFusion's default SUM doesn't match Spark's overflow semantics. This implementation ensures we get the exact same behavior as Spark for integer overflow in all 3 eval modes (ANSI , Legacy and Try mode)

What changes are included in this PR?

This PR adds native Rust implementation for SUM aggregation on integer types (byte, short, int, long)

Native changes (Rust):
(Inspired from SumDecimal and spark's SUM impl)

  1. New SumInteger aggregate function that handles SUM for all integer types (in coherence with Spark)
  2. Support all eval modes (ANSI , Legacy and Try)
    ( Implemented code in similar fashion of spark leveraging Option<i64> to represent NULL and numeric values for sum , and an additional parameter called has_all_nulls which is leveraged in Try mode to distinguish if NULL sum is caused by all NULL inputs or the fact that the sum overflowed. (Spark does this with shouldTrackIsEmpty and assigning NULL to running sum which is a long datatype) )
  3. Implemented groups accumulator (which should optimize group by op)

Scala side changes :

  1. Update CometSum to add ANSI support (ANSI and Try)
  2. Change proto schema to pass along eval_mode instead of fail_on_error to support Legacy, ANSI and Try eval modes
  3. Added tests for NULL (had to force cast nulls as java.lang.Long to avoid Scala's auto-boxing feature which auto casts objects to primitive types there by casting nulls to 0s) handling in both simple and GROUP BY agg .
  4. Added tests for both ANSI and non-ANSI mode tests for the input types Int, Short, Byte
  5. Added try_sum overflow tests with GROUP BY (mixed overflow/non-overflow groups)

How are these changes tested?

  1. Unit tests on Scala side to test various aggregation use cases in all 3 eval modes .
  2. Added new tests for groupBy use cases to test out groups accumulator

@coderfender coderfender marked this pull request as draft October 17, 2025 19:13
@coderfender
Copy link
Contributor Author

Draft PR to support sum function - WIP

@coderfender coderfender marked this pull request as ready for review October 28, 2025 07:53
@coderfender coderfender changed the title [WIP]: Support ANSI mode sum expr feat: Support ANSI mode sum expr Oct 29, 2025
@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.06%. Comparing base (f09f8af) to head (3f9aff7).
⚠️ Report is 717 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2600      +/-   ##
============================================
+ Coverage     56.12%   59.06%   +2.93%     
- Complexity      976     1476     +500     
============================================
  Files           119      165      +46     
  Lines         11743    15060    +3317     
  Branches       2251     2504     +253     
============================================
+ Hits           6591     8895    +2304     
- Misses         4012     4900     +888     
- Partials       1140     1265     +125     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderfender coderfender force-pushed the support_overflow_sum_function branch from 970d693 to c0715aa Compare November 7, 2025 01:16
@coderfender
Copy link
Contributor Author

@andygrove , @comphead I believe this should be ready for review (pending CI)

@comphead
Copy link
Contributor

comphead commented Nov 7, 2025

- Windows support *** FAILED *** (3 seconds, 973 milliseconds)
  Expected only Comet native operators, but found Project.
  plan: Project
  +- Window
     +- CometExchange
        +- CometScan [native_iceberg_compat] parquet

Thats actually interesting why would window fall back, we planning to fallback on windows in #2726 but the test would still preserve windows, so I think we need to check a fallback reason

@coderfender coderfender force-pushed the support_overflow_sum_function branch from 6dcf47b to 13b7d68 Compare November 11, 2025 22:02
@coderfender
Copy link
Contributor Author

- Windows support *** FAILED *** (3 seconds, 973 milliseconds)
  Expected only Comet native operators, but found Project.
  plan: Project
  +- Window
     +- CometExchange
        +- CometScan [native_iceberg_compat] parquet

Thats actually interesting why would window fall back, we planning to fallback on windows in #2726 but the test would still preserve windows, so I think we need to check a fallback reason

This is no longer a failing test after I rebased with main branch

@coderfender
Copy link
Contributor Author

@andygrove @comphead . All the tests passed and the PR is rest for review

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

@coderfender This LGTM. I am going to pull the PR locally and do some testing/benchmarking today. Could you rebease/upmerge?

@coderfender
Copy link
Contributor Author

Thank you @andygrove . Let me rebase Ave fix the merge conflicts

@andygrove
Copy link
Member

Here are microbenchmark results:

Legacy mode

OpenJDK 64-Bit Server VM 17.0.16+8-Ubuntu-0ubuntu122.04.1 on Linux 6.8.0-87-generic
AMD Ryzen 9 7950X3D 16-Core Processor
TPCDS Micro Benchmarks:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
agg_sum_integers_no_grouping                       1720           1812         129        167.4           6.0       1.0X
agg_sum_integers_no_grouping: Comet                1171           1179          11        246.0           4.1       1.5X

Running benchmark: TPCDS Micro Benchmarks
  Running case: agg_sum_integers_with_grouping
  Stopped after 2 iterations, 5105 ms
  Running case: agg_sum_integers_with_grouping: Comet
  Stopped after 2 iterations, 2803 ms

OpenJDK 64-Bit Server VM 17.0.16+8-Ubuntu-0ubuntu122.04.1 on Linux 6.8.0-87-generic
AMD Ryzen 9 7950X3D 16-Core Processor
TPCDS Micro Benchmarks:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
agg_sum_integers_with_grouping                     2529           2553          34        113.9           8.8       1.0X
agg_sum_integers_with_grouping: Comet              1396           1402           8        206.3           4.8       1.8X

ANSI mode

OpenJDK 64-Bit Server VM 17.0.16+8-Ubuntu-0ubuntu122.04.1 on Linux 6.8.0-87-generic
AMD Ryzen 9 7950X3D 16-Core Processor
TPCDS Micro Benchmarks:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
agg_sum_integers_no_grouping                       1751           1802          72        164.5           6.1       1.0X
agg_sum_integers_no_grouping: Comet                1358           1360           3        212.1           4.7       1.3X

Running benchmark: TPCDS Micro Benchmarks
  Running case: agg_sum_integers_with_grouping
  Stopped after 2 iterations, 5563 ms
  Running case: agg_sum_integers_with_grouping: Comet
  Stopped after 2 iterations, 3668 ms

OpenJDK 64-Bit Server VM 17.0.16+8-Ubuntu-0ubuntu122.04.1 on Linux 6.8.0-87-generic
AMD Ryzen 9 7950X3D 16-Core Processor
TPCDS Micro Benchmarks:                   Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
agg_sum_integers_with_grouping                     2772           2782          14        103.9           9.6       1.0X
agg_sum_integers_with_grouping: Comet              1834           1834           1        157.1           6.4       1.5X

@coderfender
Copy link
Contributor Author

coderfender commented Nov 21, 2025

@andygrove , Please take a look whenever you get a chance . I made changes per review and fixed merge conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ANSI support in SUM and AVG

5 participants