Skip to content

Fix incorrect NULL IN () optimization#17092

Merged
findepi merged 2 commits into
apache:mainfrom
findepi:findepi/fix-incorrect-null-in-optimization-d6d781
Aug 11, 2025
Merged

Fix incorrect NULL IN () optimization#17092
findepi merged 2 commits into
apache:mainfrom
findepi:findepi/fix-incorrect-null-in-optimization-d6d781

Conversation

@findepi

@findepi findepi commented Aug 8, 2025

Copy link
Copy Markdown
Member

The optimization does not manifest as a bug in typical queries, because
the ConstantEvaluator prevents that. However, it still needs to be
corrected (or removed), otherwise it's a bug waiting to manifest
somewhere.

@findepi
findepi requested review from jayzhan211 and timsaucer August 8, 2025 11:56
@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Aug 8, 2025
@findepi
findepi requested a review from alamb August 9, 2025 20:44

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you @findepi

It is unfortunate that we need to complicate the actual code to add a testing hook.

Is it posisble to just unit test the code in question -- for example, perhaps we could write a unit test that usedSimplifier directly rather than ExprSimplifier 🤔

Comment thread datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs Outdated
Comment thread datafusion/sqllogictest/test_files/predicates.slt
@findepi

findepi commented Aug 10, 2025

Copy link
Copy Markdown
Member Author

Is it posisble to just unit test the code in question -- for example, perhaps we could write a unit test that used Simplifier directly rather than ExprSimplifier 🤔

One can argue that one is an impl detail of the other... are there any tests for Simplifier?

It is unfortunate that we need to complicate the actual code to add a testing hook.

we can go without a regression test for this fix.
testing Simplifier directly and not ExprSimplifier would not constitute a regression test anyway.

The optimization does not manifest as a bug in typical queries, because
the `ConstantEvaluator` prevents that. However, it still needs to be
corrected (or removed), otherwise it's a bug waiting to manifest
somewhere.
@findepi

findepi commented Aug 11, 2025

Copy link
Copy Markdown
Member Author

we could write a unit test that usedSimplifier directly rather than ExprSimplifier

done

@findepi
findepi force-pushed the findepi/fix-incorrect-null-in-optimization-d6d781 branch from 7727b4c to 0a7b3a9 Compare August 11, 2025 08:09

@xudong963 xudong963 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you @findepi

@findepi
findepi merged commit 9463cf6 into apache:main Aug 11, 2025
27 checks passed
@findepi
findepi deleted the findepi/fix-incorrect-null-in-optimization-d6d781 branch August 11, 2025 09:51
assert_eq!(simplify(expr), lit(true));
}

#[test]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

3 participants