You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "AdamGS (via GitHub)" <gi...@apache.org> on 2025/09/10 10:07:38 UTC
[PR] Support Decimal32/64 types [datafusion]
AdamGS opened a new pull request, #17501:
URL: https://github.com/apache/datafusion/pull/17501
## Which issue does this PR close?
Closes #17489.
## Rationale for this change
Since Arrow 56 and #16690, support has been added to the two smaller decimal variants. This PR tries to bring support to them on-par with the support for the bigger types.
I've run into this issue when upgrading [Vortex](https://github.com/vortex-data/vortex/pull/4577) to use the upcoming release (50), one of our [benchmarks](https://github.com/apache/datafusion/compare/main...AdamGS:adamg/extended-decimal-support?expand=1) fails with:
```
Error during planning: Execution error: Function 'sum' user-defined coercion failed with "Execution error: Sum not supported for Decimal32(7, 2)" No function matches the given name and argument types 'sum(Decimal32(7, 2))'. You might need to add explicit type casts.
Candidate functions:
sum(UserDefined)
```
## What changes are included in this PR?
Adding support for handling decimal32/64 wherever other decimal types are handled. I've tried to use multiple search methods to find places where they matter, but there might be some matches/if conditions that make them hard to find.
## Are these changes tested?
The testing strategy to some of these parts is unclear to me, both around SQL but also for aggregate functions like `SUM` and `AVG`.
## Are there any user-facing changes?
Code that used to fail while reading some decimals should now work.
--
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] Support Decimal32/64 types [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3304139945
> @xudong963 while working on the SQL part I've found some more parts I've missed, so I pushed them here in the last commit. I've also found a [bug](https://github.com/apache/arrow-rs/pull/8363) in Arrow that breaks casting with decimal64 (more specifically - when using SQL to insert a literal floating-point value), so that part is essentially blocked on that in addition to the 56.1 release.
Update is the fix for the arrow bug https://github.com/apache/arrow-rs/pull/8363 will be in arrow 56.2.0. I hope to make an RC later today or tomorrow.
I have already prepared an upgrade here:
- https://github.com/apache/datafusion/pull/17631
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3307354538
I think this PR is now ready for review (assuming all tests pass), I have found a few more spots I missed (see the last commit) and rebased on top of @Jefffrey's https://github.com/apache/datafusion/pull/17560.
I hope to have the follow up ready later today or tomorrow, which will have more SLT style tests.
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3292271850
SGTM. I'll open a follow-up PR with the the changes to datafusion-sql, I think that for them to work well I'll still need https://github.com/apache/datafusion/pull/17275 (unsurprisingly - @alamb beat me to upgrading arrow and parquet), but seems like its pretty close to being ready.
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3305011305
always seemed like a reasonable order of merging to me! once I fix this one I think it should be ready to be merged, with the follow-up only merged after arrow is upgraded to 56.2 (I also plan on opening it tomorrow)
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#discussion_r2359160594
##########
datafusion/functions-aggregate/src/average.rs:
##########
@@ -286,6 +286,7 @@ impl AggregateUDFImpl for Avg {
| DataType::Decimal32(_, _)
| DataType::Decimal64(_, _)
| DataType::Decimal128(_, _)
+ | DataType::Decimal256(_, _)
Review Comment:
I think it was just overlooked here, because the group accumulator does exist.
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -881,7 +881,8 @@ fn coerced_from<'a>(
| Float64
| Decimal32(_, _)
| Decimal64(_, _)
- | Decimal128(_, _),
+ | Decimal128(_, _)
+ | Decimal256(_, _),
Review Comment:
https://github.com/apache/datafusion/pull/17501/commits/f0564d443261b1b719e17aa8d880c90f9385b1ab#r2359160594
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3291653091
One issue that is potentially breaking for backwards compatibility and is also an issue for slt tests, is when parsing SQL we use the precision and scale to infer the decimal size, if we add new types there it might change the behavior of existing code (will parse SQL that used to create decimal128 into 64/32). What do you think?
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3292180022
Assuming support for decimal32/64 is desirable, I think the best course of action is:
1. Initially - the SQL interface just won't support creating decimal32/64 columns. Merge this PR as-is (once we have arrow 56.1.0 for casting).
2. Open a separate PR to make the breaking change to the SQL interface, but only merge it closer to the next major release (51, should be late November).
We can also decide to merge this PR (with the addition of the SQL support and SLT tests) closer to the 51 release, but personally I feel like its better to at least have the basic support merged so its actually used, which will help us find other missing functionality or potential bugs.
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#discussion_r2359161115
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -881,7 +881,8 @@ fn coerced_from<'a>(
| Float64
| Decimal32(_, _)
| Decimal64(_, _)
- | Decimal128(_, _),
+ | Decimal128(_, _)
+ | Decimal256(_, _),
Review Comment:
https://github.com/apache/datafusion/pull/17501/commits/f0564d443261b1b719e17aa8d880c90f9385b1ab#r2359160594
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3299471246
@xudong963 while working on the SQL part I've found some more parts I've missed, so I pushed them here in the last commit.
I've also found a [bug](https://github.com/apache/arrow-rs/pull/8363) in Arrow that breaks casting with decimal64 (more specifically - when using SQL to insert a literal floating-point value), so that part is essentially blocked on that in addition to the 56.1 release.
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3292193016
[This](https://github.com/apache/datafusion/pull/16290/files) is a similar but more major change, we can have a transition period with a dedicated config. I'm not a fan but I'll yield to people with more experience here if they think that's required.
--
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] Support Decimal32/64 types [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #17501:
URL: https://github.com/apache/datafusion/pull/17501
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on code in PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#discussion_r2359157355
##########
datafusion/expr-common/src/type_coercion/binary.rs:
##########
@@ -27,6 +27,8 @@ use arrow::compute::can_cast_types;
use arrow::datatypes::{
Review Comment:
Completely missed this whole are on previous passes
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3291741873
Another issue I've just run into - casting support in arrow-rs was only released in 56.1, so this PR should probably only merge once that's available here. The [dependabot PR](https://github.com/apache/datafusion/actions/runs/17726508245/job/50368207867?pr=17434) seems to have some failures, I'll take that, hopefully its nothing too hard.
--
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] Support Decimal32/64 types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3292255183
> Assuming support for decimal32/64 is desirable, I think the best course of action is:
>
> 1. Initially - the SQL interface just won't support creating decimal32/64 columns. Merge this PR as-is (once we have arrow 56.1.0 for casting).
>
> 2. Open a separate PR to make the breaking change to the SQL interface, but only merge it closer to the next major release (51, should be late November).
>
>
> We can also decide to merge this PR (with the addition of the SQL support and SLT tests) closer to the 51 release, but personally I feel like its better to at least have the basic support merged so its actually used, which will help us find other missing functionality or potential bugs.
Steps 1 & 2 sound good, if we can merge the majority of decimal 32/64 work in this PR and isolate that specific SQL parsing change into a separate issue/PR so it would be easier to review & discuss (a presumably smaller 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] Support Decimal32/64 types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3292141647
> One issue that is potentially breaking for backwards compatibility and is also an issue for slt tests, is when parsing SQL we use the precision and scale to infer the decimal size, if we add new types there it might change the behavior of existing code (will parse SQL that used to create decimal128 into 64/32). What do you think?
This is a good pickup; frankly I'm not sure what to do either 😅
--
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] Support Decimal32/64 types [datafusion]
Posted by "AdamGS (via GitHub)" <gi...@apache.org>.
AdamGS commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3291625590
@Jefffrey thanks for pointing those out, I'll add some tests, should take too long.
--
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] Support Decimal32/64 types [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3304949849
I've merged #17560 to main so will need some adjustments to 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] Support Decimal32/64 types [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #17501:
URL: https://github.com/apache/datafusion/pull/17501#issuecomment-3313464490
Thanks again @AdamGS
--
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