Skip to content

fix(stats): widen sum_value integer arithmetic to SUM-compatible types - #20865

Merged
alamb merged 3 commits into
apache:mainfrom
kumarUjjawal:fix/precision_sum_i64
Mar 25, 2026
Merged

fix(stats): widen sum_value integer arithmetic to SUM-compatible types#20865
alamb merged 3 commits into
apache:mainfrom
kumarUjjawal:fix/precision_sum_i64

Conversation

@kumarUjjawal

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

As discussed in the review thread on #20768 and tracked by #20826, sum_value should not keep narrow integer column types during stats aggregation, because merge/multiply paths can overflow before values are widened.

What changes are included in this PR?

This PR updates statistics sum_value arithmetic to match SUM-style widening for small integer types, and applies that behavior consistently across merge and multiplication paths.

Are these changes tested?

Yes

Are there any user-facing changes?

@github-actionsgithub-actionsBot added physical-expr Changes to the physical-expr crates common Related to common crate datasource Changes to the datasource crate physical-plan Changes to the physical-plan crate labels Mar 11, 2026
@kumarUjjawal

Copy link
Copy Markdown
ContributorAuthor

cc @jonathanc-n

@Dandandan

Copy link
Copy Markdown
Contributor

I think this looks good @jonathanc-n can you take a look as well?

@jonathanc-njonathanc-n 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.

Lgtm thanks!

@asolimando

Copy link
Copy Markdown
Member

Thanks for working on this @kumarUjjawal, but I have a few suggestions:

  1. The original idea in Match Precision sum function against Int64 to prevent overflow. #20826 was to change the sum_value field type to always store a wide type, making overflow protection structural. The lazy-widening approach here requires every future call site to remember to use add_for_sum/cast_to_sum_type instead of add/multiply, with no compiler enforcement. At minimum, a doc comment on sum_value warning about this would help.

This forces to change all consumers, but it would prove more robust over time.

  1. The Exact/Inexact arms in cast_to_sum_type are nearly identical and could be collapsed I think

@Dandandan

Copy link
Copy Markdown
Contributor

@kumarUjjawal can you fix CI?

@kumarUjjawal

Copy link
Copy Markdown
ContributorAuthor

@Dandandan Done!

@asolimandoasolimando left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGMT, the PR is hardening as much as possible for a non-breaking change, improving over the current situation

@kumarUjjawal

Copy link
Copy Markdown
ContributorAuthor

@asolimando Thanks for the review!

@kumarUjjawal

Copy link
Copy Markdown
ContributorAuthor

@alamb this is good to go.

@alamb
alamb added this pull request to the merge queueMar 25, 2026
@alamb

alamb commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

Thanks @kumarUjjawal and @Dandandan

Merged via the queue into apache:main with commit 6e0dde0Mar 25, 2026
34 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commonRelated to common cratedatasourceChanges to the datasource cratephysical-exprChanges to the physical-expr cratesphysical-planChanges to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Match Precision sum function against Int64 to prevent overflow.

5 participants

@kumarUjjawal@Dandandan@asolimando@alamb@jonathanc-n