You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/11/05 00:47:49 UTC
[GitHub] [arrow-datafusion] NGA-TRAN opened a new pull request #1245: fix: not do boolean folding on NULL and/or expr
NGA-TRAN opened a new pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245
# Which issue does this PR close?
Closes #1244
# Rationale for this change
Do not fold `Boolean(NULL) and/or expr` because it depends on the value of the `expr`
# What changes are included in this PR?
`Boolean(NULL) and/or expr` is key as-is (aka not rewrite)
# Are there any user-facing changes?
This bug was hit by [IOx's storage_api::test](https://app.circleci.com/pipelines/github/influxdata/influxdb_iox/12405/workflows/59a98d12-d79b-4410-867a-d4a49f16acc2/jobs/89410)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#discussion_r743542556
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -135,6 +135,98 @@ impl<'a> Simplifier<'a> {
false
}
+
+ fn boolean_folding_for_or(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool OR bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE or expr (including NULL) = TRUE
+ Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
+ // FALSE or expr (including NULL) = expr
+ Some(false) => *bool_expr,
+ None => match *bool_expr {
+ // NULL or TRUE = TRUE
+ Expr::Literal(ScalarValue::Boolean(Some(true))) => {
+ Expr::Literal(ScalarValue::Boolean(Some(true)))
+ }
+ // NULL or FALSE = NULL
+ Expr::Literal(ScalarValue::Boolean(Some(false))) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or NULL = NULL
+ Expr::Literal(ScalarValue::Boolean(None)) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or expr can be either NULL or TRUE
+ // So let us not rewrite it
+ _ => {
+ let mut left =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ let mut right = bool_expr;
+ if !left_right_order {
+ left = right;
+ right =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
Review comment:
I think you might be able to use `swap` here if you wanted: https://doc.rust-lang.org/stable/std/mem/fn.swap.html
so something like `std::mem::swap(&mut left, &mut right)`
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -135,6 +135,98 @@ impl<'a> Simplifier<'a> {
false
}
+
+ fn boolean_folding_for_or(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool OR bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE or expr (including NULL) = TRUE
+ Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
+ // FALSE or expr (including NULL) = expr
+ Some(false) => *bool_expr,
+ None => match *bool_expr {
+ // NULL or TRUE = TRUE
+ Expr::Literal(ScalarValue::Boolean(Some(true))) => {
+ Expr::Literal(ScalarValue::Boolean(Some(true)))
+ }
+ // NULL or FALSE = NULL
+ Expr::Literal(ScalarValue::Boolean(Some(false))) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or NULL = NULL
+ Expr::Literal(ScalarValue::Boolean(None)) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or expr can be either NULL or TRUE
+ // So let us not rewrite it
+ _ => {
+ let mut left =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ let mut right = bool_expr;
+ if !left_right_order {
+ left = right;
+ right =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ }
+
+ Expr::BinaryExpr {
+ left,
+ op: Operator::Or,
+ right,
+ }
+ }
+ },
+ }
+ }
+
+ fn boolean_folding_for_and(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool AND bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE and expr (including NULL) = expr
+ Some(true) => *bool_expr,
+ // FALSE and expr (including NULL) = FALSE
+ Some(false) => Expr::Literal(ScalarValue::Boolean(Some(false))),
+ None => match *bool_expr {
+ // NULL and TRUE = NULL
Review comment:
FWIW I am working on getting these constant cases to work with the `ConstEvaluator` (so they wouldn't be strictly necessary) but it turns out these types of expressions don't actually work in the general evaluator
I am working on it in https://github.com/apache/arrow-datafusion/pull/1163
But in any event I think this code is good 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] NGA-TRAN commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961543687
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] xudong963 commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
xudong963 commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961584923
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961978601
> @xudong963 Those are great queries. I have tried to add them in the test but I think we have not done constant folding for Between yet (or at least not for the queries in your examples) so they are not folded at this time. Hence, I do not commit them in because they are the same as the current between query.
>
> I can open a new ticket for improving these cases though. What do you think @alamb ?
I think the constant folding is handled by `ConstEvaluator` (in a different module). The `Simplifier` is supposed to be focused on items that can't be folded (like `TRUE OR <expr>` --> `<expr>`).
The fact we have some boolean constant evaluation in here is a historical artifact and due to the fact we don't have full expression evaluation support for all boolean operations (yet).
Thus I don't think a separate ticket is needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961978601
> @xudong963 Those are great queries. I have tried to add them in the test but I think we have not done constant folding for Between yet (or at least not for the queries in your examples) so they are not folded at this time. Hence, I do not commit them in because they are the same as the current between query.
>
> I can open a new ticket for improving these cases though. What do you think @alamb ?
I think the constant folding is handled by `ConstEvaluator` (in a different module). The `Simplifier` is supposed to be focused on items that can't be folded (like `TRUE OR <expr>` --> `<expr>`).
The fact we have some boolean constant evaluation in here is a historical artifact and due to the fact we don't have full expression evaluation support for all boolean operations (yet).
Thus I don't think a separate ticket is needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] NGA-TRAN commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961543687
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] xudong963 commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
xudong963 commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961584923
Thanks for your nice work 🎉@NGA-TRAN
FYI
```shell
case1
postgres=# select 11 BETWEEN 0 AND 10 and NULL as c;
c
---
f
(1 row)
case2
postgres=# select 1 BETWEEN 0 AND 10 and NULL as c;
c
---
(1 row)
case3
postgres=# select 1 BETWEEN 0 AND 10 or NULL as c;
c
---
t
(1 row)
case4
postgres=# select 11 BETWEEN 0 AND 10 or NULL as c;
c
---
(1 row)
```
So I think we can add two tests about case1 and case4. @NGA-TRAN
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] NGA-TRAN commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961543687
@alamb and @matthewmturner Can you help review this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] NGA-TRAN commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961949030
@xudong963 Those are great queries. I have tried to add them in the test but I think we have not done constant folding for `Between` yet (or at least not for the queries in your examples) so they are not folded at this time. Hence, I do not commit them in because they are the same as the current between query.
I can open a new ticket for improving these cases though. What do you think @alamb ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#discussion_r743542556
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -135,6 +135,98 @@ impl<'a> Simplifier<'a> {
false
}
+
+ fn boolean_folding_for_or(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool OR bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE or expr (including NULL) = TRUE
+ Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
+ // FALSE or expr (including NULL) = expr
+ Some(false) => *bool_expr,
+ None => match *bool_expr {
+ // NULL or TRUE = TRUE
+ Expr::Literal(ScalarValue::Boolean(Some(true))) => {
+ Expr::Literal(ScalarValue::Boolean(Some(true)))
+ }
+ // NULL or FALSE = NULL
+ Expr::Literal(ScalarValue::Boolean(Some(false))) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or NULL = NULL
+ Expr::Literal(ScalarValue::Boolean(None)) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or expr can be either NULL or TRUE
+ // So let us not rewrite it
+ _ => {
+ let mut left =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ let mut right = bool_expr;
+ if !left_right_order {
+ left = right;
+ right =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
Review comment:
I think you might be able to use `swap` here if you wanted: https://doc.rust-lang.org/stable/std/mem/fn.swap.html
so something like `std::mem::swap(&mut left, &mut right)`
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -135,6 +135,98 @@ impl<'a> Simplifier<'a> {
false
}
+
+ fn boolean_folding_for_or(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool OR bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE or expr (including NULL) = TRUE
+ Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
+ // FALSE or expr (including NULL) = expr
+ Some(false) => *bool_expr,
+ None => match *bool_expr {
+ // NULL or TRUE = TRUE
+ Expr::Literal(ScalarValue::Boolean(Some(true))) => {
+ Expr::Literal(ScalarValue::Boolean(Some(true)))
+ }
+ // NULL or FALSE = NULL
+ Expr::Literal(ScalarValue::Boolean(Some(false))) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or NULL = NULL
+ Expr::Literal(ScalarValue::Boolean(None)) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or expr can be either NULL or TRUE
+ // So let us not rewrite it
+ _ => {
+ let mut left =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ let mut right = bool_expr;
+ if !left_right_order {
+ left = right;
+ right =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ }
+
+ Expr::BinaryExpr {
+ left,
+ op: Operator::Or,
+ right,
+ }
+ }
+ },
+ }
+ }
+
+ fn boolean_folding_for_and(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool AND bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE and expr (including NULL) = expr
+ Some(true) => *bool_expr,
+ // FALSE and expr (including NULL) = FALSE
+ Some(false) => Expr::Literal(ScalarValue::Boolean(Some(false))),
+ None => match *bool_expr {
+ // NULL and TRUE = NULL
Review comment:
FWIW I am working on getting these constant cases to work with the `ConstEvaluator` (so they wouldn't be strictly necessary) but it turns out these types of expressions don't actually work in the general evaluator
I am working on it in https://github.com/apache/arrow-datafusion/pull/1163
But in any event I think this code is good 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961978601
> @xudong963 Those are great queries. I have tried to add them in the test but I think we have not done constant folding for Between yet (or at least not for the queries in your examples) so they are not folded at this time. Hence, I do not commit them in because they are the same as the current between query.
>
> I can open a new ticket for improving these cases though. What do you think @alamb ?
I think the constant folding is handled by `ConstEvaluator` (in a different module). The `Simplifier` is supposed to be focused on items that can't be folded (like `TRUE OR <expr>` --> `<expr>`).
The fact we have some boolean constant evaluation in here is a historical artifact and due to the fact we don't have full expression evaluation support for all boolean operations (yet).
Thus I don't think a separate ticket is needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#discussion_r743542556
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -135,6 +135,98 @@ impl<'a> Simplifier<'a> {
false
}
+
+ fn boolean_folding_for_or(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool OR bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE or expr (including NULL) = TRUE
+ Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
+ // FALSE or expr (including NULL) = expr
+ Some(false) => *bool_expr,
+ None => match *bool_expr {
+ // NULL or TRUE = TRUE
+ Expr::Literal(ScalarValue::Boolean(Some(true))) => {
+ Expr::Literal(ScalarValue::Boolean(Some(true)))
+ }
+ // NULL or FALSE = NULL
+ Expr::Literal(ScalarValue::Boolean(Some(false))) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or NULL = NULL
+ Expr::Literal(ScalarValue::Boolean(None)) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or expr can be either NULL or TRUE
+ // So let us not rewrite it
+ _ => {
+ let mut left =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ let mut right = bool_expr;
+ if !left_right_order {
+ left = right;
+ right =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
Review comment:
I think you might be able to use `swap` here if you wanted: https://doc.rust-lang.org/stable/std/mem/fn.swap.html
so something like `std::mem::swap(&mut left, &mut right)`
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -135,6 +135,98 @@ impl<'a> Simplifier<'a> {
false
}
+
+ fn boolean_folding_for_or(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool OR bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE or expr (including NULL) = TRUE
+ Some(true) => Expr::Literal(ScalarValue::Boolean(Some(true))),
+ // FALSE or expr (including NULL) = expr
+ Some(false) => *bool_expr,
+ None => match *bool_expr {
+ // NULL or TRUE = TRUE
+ Expr::Literal(ScalarValue::Boolean(Some(true))) => {
+ Expr::Literal(ScalarValue::Boolean(Some(true)))
+ }
+ // NULL or FALSE = NULL
+ Expr::Literal(ScalarValue::Boolean(Some(false))) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or NULL = NULL
+ Expr::Literal(ScalarValue::Boolean(None)) => {
+ Expr::Literal(ScalarValue::Boolean(None))
+ }
+ // NULL or expr can be either NULL or TRUE
+ // So let us not rewrite it
+ _ => {
+ let mut left =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ let mut right = bool_expr;
+ if !left_right_order {
+ left = right;
+ right =
+ Box::new(Expr::Literal(ScalarValue::Boolean(*const_bool)));
+ }
+
+ Expr::BinaryExpr {
+ left,
+ op: Operator::Or,
+ right,
+ }
+ }
+ },
+ }
+ }
+
+ fn boolean_folding_for_and(
+ const_bool: &Option<bool>,
+ bool_expr: Box<Expr>,
+ left_right_order: bool,
+ ) -> Expr {
+ // See if we can fold 'const_bool AND bool_expr' to a constant boolean
+ match const_bool {
+ // TRUE and expr (including NULL) = expr
+ Some(true) => *bool_expr,
+ // FALSE and expr (including NULL) = FALSE
+ Some(false) => Expr::Literal(ScalarValue::Boolean(Some(false))),
+ None => match *bool_expr {
+ // NULL and TRUE = NULL
Review comment:
FWIW I am working on getting these constant cases to work with the `ConstEvaluator` (so they wouldn't be strictly necessary) but it turns out these types of expressions don't actually work in the general evaluator
I am working on it in https://github.com/apache/arrow-datafusion/pull/1163
But in any event I think this code is good 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] NGA-TRAN commented on pull request #1245: fix: not do boolean folding on NULL and/or expr
Posted by GitBox <gi...@apache.org>.
NGA-TRAN commented on pull request #1245:
URL: https://github.com/apache/arrow-datafusion/pull/1245#issuecomment-961949953
@alamb I have addressed the comments. It should be ready for merge when all checks pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org