From 5c4bc1c8403daa89b8b20f54195a44e2d3e22c94 Mon Sep 17 00:00:00 2001 From: Trung Dinh Date: Mon, 29 Jan 2024 23:21:35 -0800 Subject: [PATCH 1/3] handle invalid types for negation --- .../physical-expr/src/expressions/negative.rs | 30 ++++++++++++++++--- datafusion/sqllogictest/test_files/scalar.slt | 3 ++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/negative.rs b/datafusion/physical-expr/src/expressions/negative.rs index b64b4a0c86def..1a249a6930010 100644 --- a/datafusion/physical-expr/src/expressions/negative.rs +++ b/datafusion/physical-expr/src/expressions/negative.rs @@ -30,7 +30,7 @@ use arrow::{ datatypes::{DataType, Schema}, record_batch::RecordBatch, }; -use datafusion_common::{internal_err, DataFusionError, Result}; +use datafusion_common::{DataFusionError, Result}; use datafusion_expr::interval_arithmetic::Interval; use datafusion_expr::{ type_coercion::{is_interval, is_null, is_signed_numeric, is_timestamp}, @@ -164,9 +164,9 @@ pub fn negative( && !is_interval(&data_type) && !is_timestamp(&data_type) { - internal_err!( - "Can't create negative physical expr for (- '{arg:?}'), the type of child expr is {data_type}, not signed numeric" - ) + Err(DataFusionError::Plan( + "Negation only supports numeric, interval and timestamp types".to_string(), + )) } else { Ok(Arc::new(NegativeExpr::new(arg))) } @@ -252,4 +252,26 @@ mod tests { ); Ok(()) } + + #[test] + fn test_negation_valid_types() -> Result<()> { + let negatable_types = [ + DataType::Int8, + DataType::Timestamp(TimeUnit::Second, None), + DataType::Interval(IntervalUnit::YearMonth), + ]; + for negatable_type in negatable_types { + let schema = Schema::new(vec![Field::new("a", negatable_type, true)]); + let _expr = negative(col("a", &schema)?, &schema)?; + } + Ok(()) + } + + #[test] + fn test_negation_invalid_types() -> Result<()> { + let schema = Schema::new(vec![Field::new("a", DataType::Utf8, true)]); + let expr = negative(col("a", &schema)?, &schema).unwrap_err(); + matches!(expr, DataFusionError::Plan(_)); + Ok(()) + } } diff --git a/datafusion/sqllogictest/test_files/scalar.slt b/datafusion/sqllogictest/test_files/scalar.slt index 0f76c722e9466..ca313820ab020 100644 --- a/datafusion/sqllogictest/test_files/scalar.slt +++ b/datafusion/sqllogictest/test_files/scalar.slt @@ -1546,6 +1546,9 @@ SELECT null, -null ---- NULL NULL +query error DataFusion error: Error during planning: Negation only supports numeric, interval and timestamp types +SELECT -'100' + statement ok drop table test_boolean From ec9240d031fc0e2123657a28a61c4847fbb39131 Mon Sep 17 00:00:00 2001 From: Trung Dinh Date: Tue, 30 Jan 2024 13:32:02 -0800 Subject: [PATCH 2/3] Update datafusion/physical-expr/src/expressions/negative.rs Use `plan_err!(..)` Co-authored-by: Andrew Lamb --- datafusion/physical-expr/src/expressions/negative.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/negative.rs b/datafusion/physical-expr/src/expressions/negative.rs index 1a249a6930010..849636a0329c9 100644 --- a/datafusion/physical-expr/src/expressions/negative.rs +++ b/datafusion/physical-expr/src/expressions/negative.rs @@ -164,9 +164,7 @@ pub fn negative( && !is_interval(&data_type) && !is_timestamp(&data_type) { - Err(DataFusionError::Plan( - "Negation only supports numeric, interval and timestamp types".to_string(), - )) + plan_err!("Negation only supports numeric, interval and timestamp types") } else { Ok(Arc::new(NegativeExpr::new(arg))) } From 36716cd482f60750d7fd799cdc0e144b39bcc8d9 Mon Sep 17 00:00:00 2001 From: Trung Dinh Date: Tue, 30 Jan 2024 14:32:15 -0800 Subject: [PATCH 3/3] github auto commit fallout --- datafusion/physical-expr/src/expressions/negative.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/physical-expr/src/expressions/negative.rs b/datafusion/physical-expr/src/expressions/negative.rs index 849636a0329c9..6b5c208bae81e 100644 --- a/datafusion/physical-expr/src/expressions/negative.rs +++ b/datafusion/physical-expr/src/expressions/negative.rs @@ -30,7 +30,7 @@ use arrow::{ datatypes::{DataType, Schema}, record_batch::RecordBatch, }; -use datafusion_common::{DataFusionError, Result}; +use datafusion_common::{plan_err, DataFusionError, Result}; use datafusion_expr::interval_arithmetic::Interval; use datafusion_expr::{ type_coercion::{is_interval, is_null, is_signed_numeric, is_timestamp},