From 5e418d5ea2a503a5e90f58cca0a76f234fd8b89c Mon Sep 17 00:00:00 2001 From: guojidan <1948535941@qq.com> Date: Wed, 24 Jan 2024 07:46:29 +0000 Subject: [PATCH 1/4] optimize NOT Expr logic --- datafusion/optimizer/src/analyzer/type_coercion.rs | 4 ++++ datafusion/physical-expr/src/expressions/not.rs | 14 +------------- datafusion/sqllogictest/test_files/scalar.slt | 8 ++++---- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs index 8c4e907e67342..16ff445e17aa2 100644 --- a/datafusion/optimizer/src/analyzer/type_coercion.rs +++ b/datafusion/optimizer/src/analyzer/type_coercion.rs @@ -176,6 +176,10 @@ impl TreeNodeRewriter for TypeCoercionRewriter { negated, ))) } + Expr::Not(expr) => { + let expr = is_not_true(get_casted_expr_for_bool_op(&expr, &self.schema)?); + Ok(expr) + } Expr::IsTrue(expr) => { let expr = is_true(get_casted_expr_for_bool_op(&expr, &self.schema)?); Ok(expr) diff --git a/datafusion/physical-expr/src/expressions/not.rs b/datafusion/physical-expr/src/expressions/not.rs index 4ceccc6932fe4..1afe0ce663da8 100644 --- a/datafusion/physical-expr/src/expressions/not.rs +++ b/datafusion/physical-expr/src/expressions/not.rs @@ -26,9 +26,7 @@ use crate::physical_expr::down_cast_any_ref; use crate::PhysicalExpr; use arrow::datatypes::{DataType, Schema}; use arrow::record_batch::RecordBatch; -use datafusion_common::{ - cast::as_boolean_array, internal_err, DataFusionError, Result, ScalarValue, -}; +use datafusion_common::{cast::as_boolean_array, Result, ScalarValue}; use datafusion_expr::ColumnarValue; /// Not expression @@ -80,16 +78,6 @@ impl PhysicalExpr for NotExpr { ))) } ColumnarValue::Scalar(scalar) => { - if scalar.is_null() { - return Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None))); - } - let value_type = scalar.data_type(); - if value_type != DataType::Boolean { - return internal_err!( - "NOT '{:?}' can't be evaluated because the expression's type is {:?}, not boolean or NULL", - self.arg, value_type - ); - } let bool_value: bool = scalar.try_into()?; Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some( !bool_value, diff --git a/datafusion/sqllogictest/test_files/scalar.slt b/datafusion/sqllogictest/test_files/scalar.slt index 3e8ebe54c09c6..13cf1d07f3348 100644 --- a/datafusion/sqllogictest/test_files/scalar.slt +++ b/datafusion/sqllogictest/test_files/scalar.slt @@ -1391,7 +1391,7 @@ query B SELECT NOT c1 FROM test_boolean ---- true -NULL +true false @@ -1527,15 +1527,15 @@ SELECT not(true), not(false) ---- false true -query error +query error type_coercion\ncaused by\nError during planning: Cannot infer common argument type for comparison operation Int64 IS DISTINCT FROM Boolean SELECT not(1), not(0) query ?B SELECT null, not(null) ---- -NULL NULL +NULL true -query error +query error type_coercion\ncaused by\nError during planning: Cannot infer common argument type for comparison operation Utf8 IS DISTINCT FROM Boolean SELECT NOT('hi') # test_negative_expressions() From db2e9e7bac6f48be6820aabdcb718f682b3fca4a Mon Sep 17 00:00:00 2001 From: guojidan <1948535941@qq.com> Date: Wed, 24 Jan 2024 09:04:14 +0000 Subject: [PATCH 2/4] fix Null type --- datafusion/optimizer/src/analyzer/type_coercion.rs | 4 ++-- datafusion/physical-expr/src/expressions/not.rs | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs index 16ff445e17aa2..3322ae0b742c2 100644 --- a/datafusion/optimizer/src/analyzer/type_coercion.rs +++ b/datafusion/optimizer/src/analyzer/type_coercion.rs @@ -47,7 +47,7 @@ use datafusion_expr::{ is_false, is_not_false, is_not_true, is_not_unknown, is_true, is_unknown, type_coercion, AggregateFunction, BuiltinScalarFunction, Expr, ExprSchemable, LogicalPlan, Operator, Projection, ScalarFunctionDefinition, Signature, WindowFrame, - WindowFrameBound, WindowFrameUnits, + WindowFrameBound, WindowFrameUnits, not, }; use crate::analyzer::AnalyzerRule; @@ -177,7 +177,7 @@ impl TreeNodeRewriter for TypeCoercionRewriter { ))) } Expr::Not(expr) => { - let expr = is_not_true(get_casted_expr_for_bool_op(&expr, &self.schema)?); + let expr = not(get_casted_expr_for_bool_op(&expr, &self.schema)?); Ok(expr) } Expr::IsTrue(expr) => { diff --git a/datafusion/physical-expr/src/expressions/not.rs b/datafusion/physical-expr/src/expressions/not.rs index 1afe0ce663da8..f17df73e30708 100644 --- a/datafusion/physical-expr/src/expressions/not.rs +++ b/datafusion/physical-expr/src/expressions/not.rs @@ -78,6 +78,9 @@ impl PhysicalExpr for NotExpr { ))) } ColumnarValue::Scalar(scalar) => { + if scalar.is_null() { + return Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None))); + } let bool_value: bool = scalar.try_into()?; Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some( !bool_value, From 03e267f37681c3c38d77f6c8940283031337592f Mon Sep 17 00:00:00 2001 From: guojidan <1948535941@qq.com> Date: Wed, 24 Jan 2024 09:06:55 +0000 Subject: [PATCH 3/4] fix test case --- datafusion/sqllogictest/test_files/scalar.slt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datafusion/sqllogictest/test_files/scalar.slt b/datafusion/sqllogictest/test_files/scalar.slt index 13cf1d07f3348..5b3ecab5fd769 100644 --- a/datafusion/sqllogictest/test_files/scalar.slt +++ b/datafusion/sqllogictest/test_files/scalar.slt @@ -1391,7 +1391,7 @@ query B SELECT NOT c1 FROM test_boolean ---- true -true +NULL false @@ -1533,7 +1533,7 @@ SELECT not(1), not(0) query ?B SELECT null, not(null) ---- -NULL true +NULL NULL query error type_coercion\ncaused by\nError during planning: Cannot infer common argument type for comparison operation Utf8 IS DISTINCT FROM Boolean SELECT NOT('hi') From 53a927744fac1318af7457867a99924dbacf83a8 Mon Sep 17 00:00:00 2001 From: guojidan <1948535941@qq.com> Date: Wed, 24 Jan 2024 09:16:02 +0000 Subject: [PATCH 4/4] fmt --- datafusion/optimizer/src/analyzer/type_coercion.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs index 3322ae0b742c2..c0dad2ef40063 100644 --- a/datafusion/optimizer/src/analyzer/type_coercion.rs +++ b/datafusion/optimizer/src/analyzer/type_coercion.rs @@ -44,10 +44,10 @@ use datafusion_expr::type_coercion::other::{ use datafusion_expr::type_coercion::{is_datetime, is_utf8_or_large_utf8}; use datafusion_expr::utils::merge_schema; use datafusion_expr::{ - is_false, is_not_false, is_not_true, is_not_unknown, is_true, is_unknown, + is_false, is_not_false, is_not_true, is_not_unknown, is_true, is_unknown, not, type_coercion, AggregateFunction, BuiltinScalarFunction, Expr, ExprSchemable, LogicalPlan, Operator, Projection, ScalarFunctionDefinition, Signature, WindowFrame, - WindowFrameBound, WindowFrameUnits, not, + WindowFrameBound, WindowFrameUnits, }; use crate::analyzer::AnalyzerRule;