Skip to content

Commit 93724b0

Browse files
authored
fix: validate wrapped negation during type coercion (apache#20965)
## Which issue does this PR close? - Related to apache#20988 . ## Rationale for this change Invalid wrapped negation expressions such as `(-'a') IS NULL` could still escape type validation during optimization. This PR keeps the fix in the analyzer/type coercion path, which is where most of DataFusion's type validation already happens, rather than adding SQL-planner-specific validation. ## What changes are included in this PR? - add type validation for `Expr::Negative` in `TypeCoercionRewriter` - return an error for invalid negation operands during optimization - add focused regression tests for wrapped negation ## Are these changes tested? Yes. Added: - a unit test for `(-'a') IS NULL` in `type_coercion` - an integration test that verifies the same case fails during `into_optimized_plan()` ## Are there any user-facing changes? Yes. Invalid wrapped negation expressions now fail during optimization instead of escaping type validation until later phases.
1 parent 85a34e9 commit 93724b0

File tree

2 files changed

+45
-2
lines changed

2 files changed

+45
-2
lines changed

datafusion/core/tests/sql/sql_api.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
// under the License.
1717

1818
use datafusion::prelude::*;
19+
use datafusion_common::assert_contains;
1920

2021
use tempfile::TempDir;
2122

@@ -206,3 +207,19 @@ async fn ddl_can_not_be_planned_by_session_state() {
206207
"This feature is not implemented: Unsupported logical plan: DropTable"
207208
);
208209
}
210+
211+
#[tokio::test]
212+
async fn invalid_wrapped_negation_fails_during_optimization() {
213+
let ctx = SessionContext::new();
214+
let err = ctx
215+
.sql("SELECT * FROM (SELECT 1) WHERE ((-'a') IS NULL)")
216+
.await
217+
.unwrap()
218+
.into_optimized_plan()
219+
.unwrap_err();
220+
221+
assert_contains!(
222+
err.strip_backtrace(),
223+
"Negation only supports numeric, interval and timestamp types"
224+
);
225+
}

datafusion/optimizer/src/analyzer/type_coercion.rs

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,12 @@ use datafusion_expr::expr_schema::cast_subquery;
4343
use datafusion_expr::logical_plan::Subquery;
4444
use datafusion_expr::type_coercion::binary::{comparison_coercion, like_coercion};
4545
use datafusion_expr::type_coercion::functions::{UDFCoercionExt, fields_with_udf};
46-
use datafusion_expr::type_coercion::is_datetime;
4746
use datafusion_expr::type_coercion::other::{
4847
get_coerce_type_for_case_expression, get_coerce_type_for_list,
4948
};
49+
use datafusion_expr::type_coercion::{
50+
is_datetime, is_interval, is_signed_numeric, is_timestamp,
51+
};
5052
use datafusion_expr::utils::merge_schema;
5153
use datafusion_expr::{
5254
Cast, Expr, ExprSchemable, Join, Limit, LogicalPlan, Operator, Projection, Union,
@@ -559,6 +561,20 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
559561
Expr::IsNotUnknown(expr) => Ok(Transformed::yes(is_not_unknown(
560562
get_casted_expr_for_bool_op(*expr, self.schema)?,
561563
))),
564+
Expr::Negative(expr) => {
565+
let data_type = expr.get_type(self.schema)?;
566+
if data_type.is_null()
567+
|| is_signed_numeric(&data_type)
568+
|| is_interval(&data_type)
569+
|| is_timestamp(&data_type)
570+
{
571+
Ok(Transformed::no(Expr::Negative(expr)))
572+
} else {
573+
plan_err!(
574+
"Negation only supports numeric, interval and timestamp types"
575+
)
576+
}
577+
}
562578
Expr::Like(Like {
563579
negated,
564580
expr,
@@ -753,7 +769,6 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
753769
| Expr::SimilarTo(_)
754770
| Expr::IsNotNull(_)
755771
| Expr::IsNull(_)
756-
| Expr::Negative(_)
757772
| Expr::Cast(_)
758773
| Expr::TryCast(_)
759774
| Expr::Wildcard { .. }
@@ -1369,6 +1384,17 @@ mod test {
13691384
)
13701385
}
13711386

1387+
#[test]
1388+
fn negative_expr_wrapped_by_is_null_errors() -> Result<()> {
1389+
let predicate = Expr::IsNull(Box::new(Expr::Negative(Box::new(lit("a")))));
1390+
let plan = LogicalPlan::Filter(Filter::try_new(predicate, empty())?);
1391+
1392+
assert_type_coercion_error(
1393+
plan,
1394+
"Negation only supports numeric, interval and timestamp types",
1395+
)
1396+
}
1397+
13721398
#[test]
13731399
fn test_coerce_union() -> Result<()> {
13741400
let left_plan = Arc::new(LogicalPlan::EmptyRelation(EmptyRelation {

0 commit comments

Comments
 (0)