You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rkrishn7 (via GitHub)" <gi...@apache.org> on 2025/09/28 23:56:36 UTC
[PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
rkrishn7 opened a new pull request, #17815:
URL: https://github.com/apache/datafusion/pull/17815
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #17699
## What changes are included in this PR?
- Allows configuration of Sum aggregate UDF to maintain decimal precision
- Rewrites `SUM`s during logical optimizer rule `SingleDistinctToGroupBy`
## Are these changes tested?
Yes
## Are there any user-facing changes?
Adds `SumProperties`
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
Posted by "rkrishn7 (via GitHub)" <gi...@apache.org>.
rkrishn7 commented on PR #17815:
URL: https://github.com/apache/datafusion/pull/17815#issuecomment-3471482124
Hi @2010YOUY01 - just bumping this PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #17815:
URL: https://github.com/apache/datafusion/pull/17815#issuecomment-3708711176
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
Posted by "rkrishn7 (via GitHub)" <gi...@apache.org>.
rkrishn7 commented on PR #17815:
URL: https://github.com/apache/datafusion/pull/17815#issuecomment-3348090551
> Thank you for the fix. I have a question:
>
> The original error message (in the issue) is: `Arrow error: Invalid argument error: Invalid comparison operation: Decimal128(35, 2) > Decimal128(25, 2)` This operation seems valid
>
> ```
> > CREATE TABLE orders (
> d1 decimal(25,2),
> d2 decimal(35,2));
> 0 row(s) fetched.
> Elapsed 0.008 seconds.
>
> > select d1 > d2 from orders;
> +-----------------------+
> | orders.d1 > orders.d2 |
> +-----------------------+
> +-----------------------+
> 0 row(s) fetched.
> Elapsed 0.008 seconds.
> ```
Hey @2010YOUY01 - yes it's a valid operation. The issue in this case is that the optimizer rule inserts the new node after type coercion runs. In your example this does not happen.
> Is it possible to make this comparison work, and keep the precision increase behavior required by Spark? It looks simpler.
Yes, but in my opinion the precision should not increase in the first place. The precision should be equivalent with two-phase aggregation so I think it's better not to change it. But happy to hear your thoughts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #17815:
URL: https://github.com/apache/datafusion/pull/17815#issuecomment-3386525516
The code that does the comparison I believe is in arrow-rs's [cmp.rs](https://github.com/apache/arrow-rs/blob/02fa779a9cb122c5218293be3afb980832701683/arrow-ord/src/cmp.rs#L206). It requires exact type matching (I've seen issues here with Timestamp types as well)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on PR #17815:
URL: https://github.com/apache/datafusion/pull/17815#issuecomment-3345800466
Thank you for the fix. I have a question:
The original error message (in the issue) is: `Arrow error: Invalid argument error: Invalid comparison operation: Decimal128(35, 2) > Decimal128(25, 2)`
This operation seems valid
```
> CREATE TABLE orders (
d1 decimal(25,2),
d2 decimal(35,2));
0 row(s) fetched.
Elapsed 0.008 seconds.
> select d1 > d2 from orders;
+-----------------------+
| orders.d1 > orders.d2 |
+-----------------------+
+-----------------------+
0 row(s) fetched.
Elapsed 0.008 seconds.
```
Is it possible to make this comparison work, and keep the precision increase behavior required by Spark? It looks simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17815:
URL: https://github.com/apache/datafusion/pull/17815#issuecomment-3490941689
A concern I have with this approach is we might have to repeat this pattern for other functions that similarly alter the resultant decimal precision & scale. Off the top of my head, `avg()` might encounter a similar issue?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: Maintain `SUM` precision during two-phase aggregation [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #17815: fix: Maintain `SUM` precision during two-phase aggregation
URL: https://github.com/apache/datafusion/pull/17815
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org