Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should not fold a non-constant expression when it is not clear to do so #1244

Closed
NGA-TRAN opened this issue Nov 4, 2021 · 4 comments · Fixed by #1245
Closed

Should not fold a non-constant expression when it is not clear to do so #1244

NGA-TRAN opened this issue Nov 4, 2021 · 4 comments · Fixed by #1245
Assignees
Labels
bug Something isn't working

Comments

@NGA-TRAN
Copy link
Contributor

NGA-TRAN commented Nov 4, 2021

Describe the bug
This expression col < 10 and boolean(NULL) can either be NULL or false depending on the result of col < 10. Hence it should not be rewritten to a constant but it is current can.

To Reproduce
Add the following test in optimize_expr_bool_and, you will see it was rewritten to c1 BETWEEN Int32(0) AND Int32(10) which is wrong.

      // c1 BETWEEN Int32(0) AND Int32(10) AND Boolean(NULL)
      // it can be either NULL or false depending on the value of `c1 BETWEEN Int32(0) AND Int32(10`
       let expr = Expr::Between{ expr: Box::new(col("c1")), negated: false, low: Box::new(lit(0)), high: Box::new(lit(10))};
       let expr = expr.and(Expr::Literal(ScalarValue::Boolean(None)));
       let result = expr.clone().rewrite(&mut rewriter)?;
       println!("expr: {:#?} \n result: {:#?}", expr, result);
       assert_eq!(expr, result);

Expected behavior
Do not rewritten expression that does not have deterministic result yet

@NGA-TRAN NGA-TRAN added the bug Something isn't working label Nov 4, 2021
@NGA-TRAN NGA-TRAN changed the title Should not fold a non-constant expression Should not fold a non-constant expression when it is not clear to do so Nov 4, 2021
@NGA-TRAN
Copy link
Contributor Author

NGA-TRAN commented Nov 4, 2021

I am working on a fix for this @alamb

@alamb
Copy link
Contributor

alamb commented Nov 4, 2021

thank you @NGA-TRAN -- FYI @matthewmturner I lead you astray in #1208 🤦 about nulls. NULLs!!!!

@matthewmturner
Copy link
Contributor

@alamb got it - thanks for keeping me posted!

@alamb
Copy link
Contributor

alamb commented Nov 6, 2021

👍 FWIW I think handling NULLs correctly is the single hardest thing in databases (at least the thing that is the most fraught with strange corner cases). The fact we are dealing with them in DataFusion now I think is a sign of its maturity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants