fix: validate wrapped negation during type coercion by myandpr · Pull Request #20965 · apache/datafusion

myandpr · 2026-03-16T15:42:08Z

Which issue does this PR close?

Related to Wrapped unary negation under IS NULL can escape analyzer type validation #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.

myandpr · 2026-03-16T16:07:19Z

Hey @alamb , Could you please help trigger CI for this PR? This is my first contribution here.

I’m hoping to contribute more actively to the project going forward, so I’d really appreciate your help. Thanks!

alamb

Thanks for the contribution @myandpr 🙏

alamb · 2026-03-16T19:21:45Z

+        let predicate_type = expr.get_type(schema)?;
+        if !Self::is_allowed_predicate_type(&predicate_type) {
+            return plan_err!(
+                "Cannot create filter with non-boolean predicate '{expr}' returning {predicate_type}"


most DataFusion type validation happens during the Analyzer phase, not the sql planner (binding phase)

I agree with @Acfboy s original statement that failing at physical planning time is probably too late, but trying to do this in sql is too early

Among other things it means that this type checking will not apply to queries created via the DataFrame API

Thanks @alamb , that makes sense.

I agree that doing this in the SQL planner makes the behavior SQL-specific and misses queries built through the DataFrame API.

I’ll rework this toward the analyzer/type coercion path instead, and I’ll take a closer look at which cases are already covered there versus which ones still need to be handled. I’ll also update the regression tests to match that direction.

@alamb Thanks for the feedback. After reading through the earlier discussion in more detail, I realized this was originally part of the follow-up from #20201, where the broader question was how much of this validation should happen earlier, and in which phase.

Based on your comment, I reworked this to keep the validation in the analyzer/type coercion path instead of the SQL planner, since that is where most of DataFusion's type validation already happens and it also applies to DataFrame API queries.

After checking the current behavior more closely, it looks like the specific remaining gap here was wrapped unary negation under IS NULL, such as (-'a') IS NULL, so I narrowed this PR to that case and added focused regression tests for it.

I also opened #20988 to track this case more precisely. Could you please take another look when you have a chance?

Also, it looks like the new CI runs ended in startup_failure before any jobs were created, likely because the workflows need to be re-triggered.
@alamb Could you please help trigger CI again when convenient? Thanks.

alamb

Thanks @myandpr

alamb · 2026-03-19T14:13:21Z

                get_casted_expr_for_bool_op(*expr, self.schema)?,
            ))),
+            Expr::Negative(expr) => {
+                let data_type = expr.get_type(self.schema)?;


I double checked that this matches the check in physical expr:

datafusion/datafusion/physical-expr/src/expressions/negative.rs

Lines 194 to 195 in 5d37bab

Ok(arg)

} else if !is_signed_numeric(&data_type)

Thanks for double checking, and thanks for the careful review.🙏

alamb · 2026-03-22T15:20:31Z

Thanks again

alamb · 2026-03-22T15:29:57Z

🤔 this one seems to have broken CI for some reason:

https://github.com/apache/datafusion/actions/runs/23406116823/job/68085237860

myandpr · 2026-03-22T16:50:43Z

🤔 this one seems to have broken CI for some reason:

https://github.com/apache/datafusion/actions/runs/23406116823/job/68085237860

Thanks for catching that. I checked the failing job, and this looks like an sqllogictest expectation mismatch.

It looks like this particular sqllogictest case had not been exercised in the earlier CI runs I saw on the PR. The analyzer change now surfaces this negation error through type_coercion, while the existing expectation in scalar.slt still assumes the older error form.

I’ll open a follow-up PR to update that.

) ## Which issue does this PR close? - Fixes CI breakage on main introduced by #20965. link: https://github.com/apache/datafusion/actions/runs/23406116823/job/68085237860 ## Rationale for this change #20965 moved invalid negation validation into the analyzer/type coercion path. After that change, the `SELECT -'100'` sqllogictest case now reports the error through `type_coercion`, but `scalar.slt` was still expecting the older error form. This follow-up updates the sqllogictest expectation to match the current behavior. ## What changes are included in this PR? - update the `scalar.slt` expectation for `SELECT -'100'` - keep the expected error aligned with the analyzer/type coercion error shape introduced by #20965 ## Are these changes tested? Yes. I reproduced the failure locally and verified the updated expectation with: - `cargo test -p datafusion-sqllogictest --test sqllogictests -- scalar` ## Are there any user-facing changes? No. Signed-off-by: yaommen <[email protected]>

alamb · 2026-03-23T13:40:24Z

It looks like this particular sqllogictest case had not been exercised in the earlier CI runs I saw on the PR. The analyzer change now surfaces this negation error through type_coercion, while the existing expectation in scalar.slt still assumes the older error form.

Perhaps it was anewly added test

Thank you for the follow up

## 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.

…che#21102) ## Which issue does this PR close? - Fixes CI breakage on main introduced by apache#20965. link: https://github.com/apache/datafusion/actions/runs/23406116823/job/68085237860 ## Rationale for this change apache#20965 moved invalid negation validation into the analyzer/type coercion path. After that change, the `SELECT -'100'` sqllogictest case now reports the error through `type_coercion`, but `scalar.slt` was still expecting the older error form. This follow-up updates the sqllogictest expectation to match the current behavior. ## What changes are included in this PR? - update the `scalar.slt` expectation for `SELECT -'100'` - keep the expected error aligned with the analyzer/type coercion error shape introduced by apache#20965 ## Are these changes tested? Yes. I reproduced the failure locally and verified the updated expectation with: - `cargo test -p datafusion-sqllogictest --test sqllogictests -- scalar` ## Are there any user-facing changes? No. Signed-off-by: yaommen <[email protected]>

github-actions Bot added sql SQL Planner core Core DataFusion crate labels Mar 16, 2026

myandpr mentioned this pull request Mar 16, 2026

More type checking at logical planning #20356

Open

myandpr force-pushed the 20356-logical-planning-validation branch 4 times, most recently from 75d220e to 0e226bd Compare March 16, 2026 17:41

alamb reviewed Mar 16, 2026

View reviewed changes

fix: validate wrapped negation during type coercion

f83e6d3

myandpr changed the title ~~fix: validate clause expressions during logical planning~~ fix: validate wrapped negation during type coercion Mar 17, 2026

myandpr force-pushed the 20356-logical-planning-validation branch from 0e226bd to f83e6d3 Compare March 17, 2026 12:49

github-actions Bot added optimizer Optimizer rules and removed sql SQL Planner labels Mar 17, 2026

alamb approved these changes Mar 19, 2026

View reviewed changes

alamb added this pull request to the merge queue Mar 22, 2026

Merged via the queue into apache:main with commit 93724b0 Mar 22, 2026
7 checks passed

myandpr mentioned this pull request Mar 22, 2026

test: update sqllogictest expectation for negation type coercion #21102

Merged

Navigation Menu

Repository navigation

fix: validate wrapped negation during type coercion - #20965

fix: validate wrapped negation during type coercion#20965
alamb merged 1 commit into
apache:mainfrom
myandpr:20356-logical-planning-validation

myandpr commented Mar 16, 2026 •

edited

Loading

Uh oh!

myandpr commented Mar 16, 2026

Uh oh!

alamb left a comment

Uh oh!

alamb Mar 16, 2026

Uh oh!

myandpr Mar 17, 2026

Uh oh!

myandpr Mar 17, 2026

Uh oh!

alamb left a comment

Uh oh!

alamb Mar 19, 2026

Uh oh!

myandpr Mar 19, 2026

Uh oh!

alamb commented Mar 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

alamb commented Mar 22, 2026

Uh oh!

myandpr commented Mar 22, 2026

Uh oh!

alamb commented Mar 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

myandpr commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

myandpr commented Mar 16, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

myandpr Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

myandpr Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

myandpr Mar 19, 2026

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alamb commented Mar 22, 2026

Uh oh!

myandpr commented Mar 22, 2026

Uh oh!

alamb commented Mar 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

myandpr commented Mar 16, 2026 •

edited

Loading

alamb commented Mar 22, 2026 •

edited

Loading