jayzhan211 commented on code in PR #26375:
URL: https://github.com/apache/datafusion/pull/26375#discussion_r4373662478
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -835,6 +835,20 @@ impl<'a> Simplifier<'a> {
}
}
+/// Simplifies `expr {=, !=} literal` in either operand order after checking
Review Comment:
nit: the helper doesn't check the operand types itself; the match guards at
the call sites do. Maybe word it as "Callers must ensure both operands are
boolean."
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -835,6 +835,20 @@ impl<'a> Simplifier<'a> {
}
}
+/// Simplifies `expr {=, !=} literal` in either operand order after checking
+/// that both operands have boolean type.
+fn simplify_boolean_comparison(
+ literal: &Expr,
+ other: Box<Expr>,
+ op: Operator,
+) -> Result<Expr> {
+ Ok(match (as_bool_lit(literal)?, op) {
Review Comment:
nit: the `(Some(_), _)` arm is a catch-all, so if anyone ever calls this
with another operator it silently becomes `NOT other`. Listing the four cases
explicitly reads like the old rule table and might also help with the
readability concern:
```diff
- Ok(match (as_bool_lit(literal)?, op) {
- (Some(true), Operator::Eq) | (Some(false), Operator::NotEq) =>
*other,
- (Some(_), _) => Expr::Not(other),
- (None, _) => lit_bool_null(),
+ Ok(match (op, as_bool_lit(literal)?) {
+ (Operator::Eq, Some(true)) | (Operator::NotEq, Some(false)) =>
*other,
+ (Operator::Eq, Some(false)) | (Operator::NotEq, Some(true)) =>
Expr::Not(other),
+ (Operator::Eq | Operator::NotEq, None) => lit_bool_null(),
+ _ => return internal_err!("unexpected operator {op} for boolean
comparison"),
})
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -4152,11 +4144,22 @@ mod tests {
assert_eq!(col("c2").get_type(&schema).unwrap(), DataType::Boolean);
- // c2 != true -> !c2
- assert_eq!(simplify(col("c2").not_eq(lit(true))), col("c2").not(),);
-
- // c2 != false -> c2
- assert_eq!(simplify(col("c2").not_eq(lit(false))), col("c2"),);
+ for name in ["c2", "c2_non_null"] {
Review Comment:
nit: I agree with @alamb here. Could you keep the original
`simplify(col("c2").eq(lit(true)))` style assertions unchanged and add the new
`simplify_no_canonicalize` loops alongside them? The originals cover the
default path with canonicalization on, and leaving them untouched makes it easy
to see the refactor didn't change any expected output. I ran the new loops
against the pre-PR code and they pass there too, so the extra coverage is a
good addition.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
