Skip to content

fix coercion of null for decimal math in binary_rules #3549

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

Merged
alamb merged 2 commits into from
Sep 22, 2022
Merged

fix coercion of null for decimal math in binary_rules #3549

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0107d05
fix
kmitchener Sep 20, 2022
c775f70
revert refactor -- handedness matters to decimal rules
kmitchener Sep 20, 2022
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
22 changes: 10 additions & 12 deletions datafusion/expr/src/binary_rule.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,9 @@ fn mathematics_numerical_coercion(
(Decimal128(_, _), Decimal128(_, _)) => {
coercion_decimal_mathematics_type(mathematics_op, lhs_type, rhs_type)
}
(Null, dec_type @ Decimal128(_, _)) | (dec_type @ Decimal128(_, _), Null) => {
Some(dec_type.clone())
Copy link
Contributor Author

@kmitchener kmitchener Sep 20, 2022

Choose a reason for hiding this comment

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

this is the actual fix, the rest is some refactoring

Copy link
Contributor

@alamb alamb Sep 21, 2022

Choose a reason for hiding this comment

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

Could the same coercion (Null to that type) be applied for all the other numeric types as well?

In other words, I wonder if we need to special case Decimal128 ? Is it to skip the special decimal coercion logic below?

Copy link
Contributor Author

@kmitchener kmitchener Sep 21, 2022

Choose a reason for hiding this comment

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

Yes, it's basically to shortcut that next match arm for figuring out the Decimal's precision and scale, which depends on the datatype being math'd to the decimal ... none of that matters if it's a null, you just need to return the same datatype. Back further up the chain, the null gets cast to this datatype.

The other number types don't have the rules that Decimals do, so you'll see in the same match they just return their own datatype, no complexity.

}
(Decimal128(_, _), _) => {
let converted_decimal_type = coerce_numeric_type_to_decimal(rhs_type);
match converted_decimal_type {
Expand Down Expand Up @@ -624,19 +627,12 @@ fn eq_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
}

/// coercion rules from NULL type. Since NULL can be casted to most of types in arrow,
/// either lhs or rhs is NULL, if NULL can be casted to type of the other side, the coecion is valid.
/// either lhs or rhs is NULL, if NULL can be casted to type of the other side, the coercion is valid.
fn null_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {
match (lhs_type, rhs_type) {
(DataType::Null, _) => {
if can_cast_types(&DataType::Null, rhs_type) {
Some(rhs_type.clone())
} else {
None
}
}
(_, DataType::Null) => {
if can_cast_types(&DataType::Null, lhs_type) {
Some(lhs_type.clone())
(DataType::Null, other_type) | (other_type, DataType::Null) => {
Copy link
Contributor

@alamb alamb Sep 21, 2022

Choose a reason for hiding this comment

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

this is a nice cleanup

if can_cast_types(&DataType::Null, other_type) {
Some(other_type.clone())
} else {
None
}
Expand Down Expand Up @@ -679,6 +675,7 @@ mod tests {
DataType::Float64,
DataType::Decimal128(38, 10),
DataType::Decimal128(20, 8),
DataType::Null,
];
let result_types = [
DataType::Decimal128(20, 3),
Expand All @@ -689,6 +686,7 @@ mod tests {
DataType::Decimal128(32, 15),
DataType::Decimal128(38, 10),
DataType::Decimal128(25, 8),
DataType::Decimal128(20, 3),
];
let comparison_op_types = [
Operator::NotEq,
Expand Down Expand Up @@ -778,7 +776,7 @@ mod tests {
}

#[test]
fn test_dictionary_type_coersion() {
fn test_dictionary_type_coercion() {
use DataType::*;

let lhs_type = Dictionary(Box::new(Int8), Box::new(Int32));
Expand Down