-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Decimal256 coercion #7034
Decimal256 coercion #7034
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @jdye64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jdye64
This seems like a fine incremental improvement to me, but it seems like there is a lot more work needed to properly support Decimal 256, just FYI
@@ -480,7 +480,7 @@ pub fn is_avg_support_arg_type(arg_type: &DataType) -> bool { | |||
_ => matches!( | |||
arg_type, | |||
arg_type if NUMERICS.contains(arg_type) | |||
|| matches!(arg_type, DataType::Decimal128(_, _)) | |||
|| matches!(arg_type, DataType::Decimal128(_, _)| DataType::Decimal256(_, _)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this change imply that people can run AVG(dec256_col)
now? I tried to run this query
create table t as values (arrow_cast(123, 'Decimal256'));
select AVG(column1) from t;
But it seems like datafusion support for Decimal256 is still in progress
❯ create table t as values (arrow_cast(123, 'Decimal256(10,3)'));
Optimizer rule 'simplify_expressions' failed
caused by
This feature is not implemented: Can't create a scalar from array of type "Decimal256(10, 3)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm yes, I was under the impression that it should. In Dask-Sql's case we are not using the physical planner from DataFusion and rather using our own so maybe this error is coming in from some similar missed Decimal256
coverage there? That would also explain why I don't see this issue when running as a library with dask-sql I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplify_expressions
is an optimization pass on the logical plan.
I suppose you're not using those or having your own.
Anyway, would be good to have some more support / (end-to-end) tests for Decimal256
.
I agree. I stumbled across this in a dask-sql test and while this solves my immediate issue there is more to it for sure |
Which issue does this PR close?
Closes #7033