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

fix: make get_valid_types handle TypeSignature::Numeric correctly #14060

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+69 −3
Open

fix: make get_valid_types handle TypeSignature::Numeric correctly #14060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9afab65
fix get_valid_types with TypeSignature::Numeric
niebayes Jan 9, 2025
3cf710c
fix sqllogictest
niebayes Jan 9, 2025
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
66 changes: 66 additions & 0 deletions datafusion/expr/src/type_coercion/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,6 +551,10 @@ fn get_valid_types(
// and their default type is double precision
if logical_data_type == NativeType::Null {
valid_type = DataType::Float64;
} else if !logical_data_type.is_numeric() {
return plan_err!(
"The signature expected NativeType::Numeric but received {logical_data_type}"
);
Comment on lines +554 to +557
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely clear on the rationale for falling back to Float64 when the argument is of type Null.
However, I choose to keep this logic and make it a short-circuit before the numeric check.

Copy link
Contributor

Choose a reason for hiding this comment

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

f64 is chosen since it is the most compatible type of numeric

}

vec![vec![valid_type; *number]]
Expand Down Expand Up @@ -939,6 +943,7 @@ mod tests {

use super::*;
use arrow::datatypes::Field;
use datafusion_common::assert_contains;

#[test]
fn test_string_conversion() {
Expand Down Expand Up @@ -1003,6 +1008,67 @@ mod tests {
}
}

#[test]
fn test_get_valid_types_numeric() -> Result<()> {
let get_valid_types_flatten =
|signature: &TypeSignature, current_types: &[DataType]| {
get_valid_types(signature, current_types)
.unwrap()
.into_iter()
.flatten()
.collect::<Vec<_>>()
};

// Trivial case.
let got = get_valid_types_flatten(&TypeSignature::Numeric(1), &[DataType::Int32]);
assert_eq!(got, [DataType::Int32]);

// Args are coerced into a common numeric type.
let got = get_valid_types_flatten(
&TypeSignature::Numeric(2),
&[DataType::Int32, DataType::Int64],
);
assert_eq!(got, [DataType::Int64, DataType::Int64]);

// Args are coerced into a common numeric type, specifically, int would be coerced to float.
let got = get_valid_types_flatten(
&TypeSignature::Numeric(3),
&[DataType::Int32, DataType::Int64, DataType::Float64],
);
assert_eq!(
got,
[DataType::Float64, DataType::Float64, DataType::Float64]
);

// Cannot coerce args to a common numeric type.
let got = get_valid_types(
&TypeSignature::Numeric(2),
&[DataType::Int32, DataType::Utf8],
)
.unwrap_err();
assert_contains!(
got.to_string(),
"The signature expected NativeType::Numeric but received NativeType::String"
);

// Fallbacks to float64 if the arg is of type null.
let got = get_valid_types_flatten(&TypeSignature::Numeric(1), &[DataType::Null]);
assert_eq!(got, [DataType::Float64]);

// Rejects non-numeric arg.
let got = get_valid_types(
&TypeSignature::Numeric(1),
&[DataType::Timestamp(TimeUnit::Second, None)],
)
.unwrap_err();
assert_contains!(
got.to_string(),
"The signature expected NativeType::Numeric but received NativeType::Timestamp(Second, None)"
);

Ok(())
}

#[test]
fn test_get_valid_types_one_of() -> Result<()> {
let signature =
Expand Down
6 changes: 3 additions & 3 deletions datafusion/sqllogictest/test_files/math.slt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,15 @@ statement error
SELECT abs(1, 2);

# abs: unsupported argument type
query error This feature is not implemented: Unsupported data type Utf8 for function abs
query error DataFusion error: Error during planning: The signature expected NativeType::Numeric but received NativeType::String
SELECT abs('foo');

# abs: numeric string
# TODO: In Postgres, '-1.2' is unknown type and interpreted to float8 so they don't fail on this query
query error DataFusion error: This feature is not implemented: Unsupported data type Utf8 for function abs
query error DataFusion error: Error during planning: The signature expected NativeType::Numeric but received NativeType::String
select abs('-1.2');

query error DataFusion error: This feature is not implemented: Unsupported data type Utf8 for function abs
query error DataFusion error: Error during planning: The signature expected NativeType::Numeric but received NativeType::String
select abs(arrow_cast('-1.2', 'Utf8'));

statement ok
Expand Down
Loading