Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce return type for aggregate sum #8141

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

jayzhan211
Copy link
Contributor

Which issue does this PR close?

Part of #7960

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@github-actions github-actions bot added the physical-expr Physical Expressions label Nov 12, 2023
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 marked this pull request as draft November 12, 2023 09:17
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@jayzhan211 jayzhan211 marked this pull request as ready for review November 12, 2023 14:09
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

The idea that aggregators might produce different result types with some inputs makes sense.

This PR is generally LGTM 👍. I wonder if some unit tests are needed to reflect this change (for now only decimal's behaviors are different, right?).

@@ -53,11 +56,12 @@ impl Sum {
name: impl Into<String>,
data_type: DataType,
) -> Self {
let data_type = sum_return_type(&data_type).unwrap();
let return_type = sum_return_type(&data_type).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

@alamb alamb merged commit eef654c into apache:main Nov 14, 2023
22 checks passed
@alamb
Copy link
Contributor

alamb commented Nov 14, 2023

Thank you @jayzhan211 and @waynexia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants