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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 20, 2023
Merged

Decimal256 coercion #7034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7b6d4ba
Merge remote-tracking branch 'upstream/main'
jdye64 Apr 19, 2023
37383bb
Merge remote-tracking branch 'upstream/main'
jdye64 Jun 7, 2023
2304dd3
Merge remote-tracking branch 'upstream/main'
jdye64 Jun 14, 2023
35aa178
Merge remote-tracking branch 'upstream/main'
jdye64 Jul 11, 2023
8bcabda
Merge remote-tracking branch 'upstream/main'
jdye64 Jul 13, 2023
ceae98b
Merge remote-tracking branch 'upstream/main'
jdye64 Jul 20, 2023
45af9b8
Add Decimal256 ARM to TypeCoercion is_signed_numeric & is_decimal fun…
jdye64 Jul 20, 2023
4bd2da1
Add Decimal256 to aggregates.rs as well
jdye64 Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions datafusion/expr/src/type_coercion/aggregates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ pub fn is_sum_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(_, _))
),
}
}
Expand All @@ -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(_, _))
Copy link
Contributor

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)"

Copy link
Contributor Author

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?

Copy link
Contributor

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.

),
}
}
Expand Down Expand Up @@ -579,6 +579,7 @@ mod tests {
let input_types = vec![
vec![DataType::Int32],
vec![DataType::Decimal128(10, 2)],
vec![DataType::Decimal256(1, 1)],
vec![DataType::Utf8],
];
for fun in funs {
Expand All @@ -594,6 +595,7 @@ mod tests {
vec![DataType::Int32],
vec![DataType::Float32],
vec![DataType::Decimal128(20, 3)],
vec![DataType::Decimal256(20, 3)],
];
for fun in funs {
for input_type in &input_types {
Expand Down
3 changes: 2 additions & 1 deletion datafusion/expr/src/type_coercion/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ pub fn is_signed_numeric(dt: &DataType) -> bool {
| DataType::Float32
| DataType::Float64
| DataType::Decimal128(_, _)
| DataType::Decimal256(_, _),
)
}

Expand Down Expand Up @@ -91,5 +92,5 @@ pub fn is_utf8_or_large_utf8(dt: &DataType) -> bool {

/// Determine whether the given data type `dt` is a `Decimal`.
pub fn is_decimal(dt: &DataType) -> bool {
matches!(dt, DataType::Decimal128(_, _))
matches!(dt, DataType::Decimal128(_, _) | DataType::Decimal256(_, _))
}