You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/02/28 21:25:18 UTC
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5423: feat: add optimization rules for bitwise operations
alamb commented on code in PR #5423:
URL: https://github.com/apache/arrow-datafusion/pull/5423#discussion_r1120767458
##########
datafusion/expr/src/expr.rs:
##########
@@ -639,6 +639,31 @@ impl Expr {
binary_expr(self, Operator::Or, other)
}
+ /// Return `self & other`
Review Comment:
👍
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -700,6 +701,288 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> {
return Err(DataFusionError::ArrowError(ArrowError::DivideByZero));
}
+ //
+ // Rules for BitwiseAnd
+ //
+
+ // A & null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null & A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A & 0 -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_zero(&right) => *right,
Review Comment:
I think this rule also has to check that `A` is non nullable as `null & 0` is `null` (not `0`, which is what this code does)
```sql
postgres=# select NULL & 0 ;
?column?
----------
(1 row)
```
The same comment applies to `0 & A` as well
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1346,6 +1629,361 @@ mod tests {
assert_eq!(simplify(expr), expected);
}
+ #[test]
+ fn test_simplify_bitwise_xor_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A ^ null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseXor, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null ^ A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseXor, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_shift_right_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A >> null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftRight, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null >> A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseShiftRight, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_shift_left_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A << null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftLeft, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null << A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseShiftLeft, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_and_by_zero() {
+ // A & 0 --> 0
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseAnd, lit(0));
+ assert_eq!(simplify(expr), lit(0));
+ }
+ // 0 & A --> 0
+ {
+ let expr = binary_expr(lit(0), Operator::BitwiseAnd, col("c2"));
+ assert_eq!(simplify(expr), lit(0));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_or_by_zero() {
+ // A | 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseOr, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ // 0 | A --> A
+ {
+ let expr = binary_expr(lit(0), Operator::BitwiseOr, col("c2"));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_xor_by_zero() {
+ // A ^ 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseXor, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ // 0 ^ A --> A
+ {
+ let expr = binary_expr(lit(0), Operator::BitwiseXor, col("c2"));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_bitwise_shift_right_by_zero() {
+ // A >> 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftRight, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_bitwise_shift_left_by_zero() {
+ // A << 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftLeft, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_and_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A & null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseAnd, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null & A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseAnd, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_composed_bitwise_and() {
+ // ((c > 5) & (c1 < 6)) & (c > 5) --> ((c > 5) & c1)
+
+ let expr = binary_expr(
+ binary_expr(
+ col("c2").gt(lit(5)),
+ Operator::BitwiseAnd,
+ col("c1").lt(lit(6)),
+ ),
+ Operator::BitwiseAnd,
+ col("c2").gt(lit(5)),
+ );
+ let expected = binary_expr(
+ col("c2").gt(lit(5)),
+ Operator::BitwiseAnd,
+ col("c1").lt(lit(6)),
+ );
+
+ assert_eq!(simplify(expr), expected);
+ }
+
+ #[test]
+ fn test_simplify_composed_bitwise_or() {
+ // ((c > 5) | (c1 < 6)) | (c > 5) --> ((c > 5) | c1)
Review Comment:
```suggestion
// ((c2 > 5) | (c1 < 6)) | (c2 > 5) --> ((c2 > 5) | c1)
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -700,6 +701,288 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> {
return Err(DataFusionError::ArrowError(ArrowError::DivideByZero));
}
+ //
+ // Rules for BitwiseAnd
+ //
+
+ // A & null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null & A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A & 0 -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_zero(&right) => *right,
+
+ // 0 & A -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_zero(&left) => *left,
+
+ // !A & A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // A & !A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // (..A..) & A --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&left, &right, BitwiseAnd) => *left,
+
+ // A & (..A..) --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&right, &left, BitwiseAnd) => *right,
+
+ // A & (A | B) --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&right)? && is_op_with(BitwiseOr, &right, &left) => {
+ *left
+ }
+
+ // (A | B) & A --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&left)? && is_op_with(BitwiseOr, &left, &right) => {
+ *right
+ }
+
+ //
+ // Rules for BitwiseOr
+ //
+
+ // A | null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseOr,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null | A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A | 0 -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_zero(&right) => *left,
+
+ // 0 | A -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_zero(&left) => *right,
+
+ // !A | A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
+ }
+
+ // A | !A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
+ }
+
+ // (..A..) | A --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if expr_contains(&left, &right, BitwiseOr) => *left,
+
+ // A | (..A..) --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if expr_contains(&left, &right, BitwiseOr) => *left,
+
+ // A | (A & B) --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if !info.nullable(&right)? && is_op_with(BitwiseAnd, &right, &left) => {
+ *left
+ }
+
+ // (A & B) | A --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if !info.nullable(&left)? && is_op_with(BitwiseAnd, &left, &right) => {
+ *right
+ }
+
+ //
+ // Rules for BitwiseXor
+ //
+
+ // A ^ null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseXor,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null ^ A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A ^ 0 -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right,
+ }) if is_zero(&right) => *left,
+
+ // 0 ^ A -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right,
+ }) if is_zero(&left) => *right,
+
+ // !A ^ A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
+ }
+
+ // A ^ !A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right,
+ }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
Review Comment:
needs the right `ScalarValue` variant
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -700,6 +701,288 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> {
return Err(DataFusionError::ArrowError(ArrowError::DivideByZero));
}
+ //
+ // Rules for BitwiseAnd
+ //
+
+ // A & null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null & A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A & 0 -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_zero(&right) => *right,
+
+ // 0 & A -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_zero(&left) => *left,
+
+ // !A & A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // A & !A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // (..A..) & A --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&left, &right, BitwiseAnd) => *left,
+
+ // A & (..A..) --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&right, &left, BitwiseAnd) => *right,
+
+ // A & (A | B) --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&right)? && is_op_with(BitwiseOr, &right, &left) => {
+ *left
+ }
+
+ // (A | B) & A --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&left)? && is_op_with(BitwiseOr, &left, &right) => {
+ *right
+ }
+
+ //
+ // Rules for BitwiseOr
+ //
+
+ // A | null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseOr,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null | A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A | 0 -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_zero(&right) => *left,
+
+ // 0 | A -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_zero(&left) => *right,
+
+ // !A | A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
+ }
+
+ // A | !A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
+ }
+
+ // (..A..) | A --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if expr_contains(&left, &right, BitwiseOr) => *left,
+
+ // A | (..A..) --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if expr_contains(&left, &right, BitwiseOr) => *left,
+
+ // A | (A & B) --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if !info.nullable(&right)? && is_op_with(BitwiseAnd, &right, &left) => {
+ *left
+ }
+
+ // (A & B) | A --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if !info.nullable(&left)? && is_op_with(BitwiseAnd, &left, &right) => {
+ *right
+ }
+
+ //
+ // Rules for BitwiseXor
+ //
+
+ // A ^ null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseXor,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null ^ A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A ^ 0 -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right,
+ }) if is_zero(&right) => *left,
+
+ // 0 ^ A -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right,
+ }) if is_zero(&left) => *right,
+
+ // !A ^ A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseXor,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
Review Comment:
needs the right `ScalarValue` variant
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -700,6 +701,288 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> {
return Err(DataFusionError::ArrowError(ArrowError::DivideByZero));
}
+ //
+ // Rules for BitwiseAnd
+ //
+
+ // A & null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null & A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A & 0 -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_zero(&right) => *right,
+
+ // 0 & A -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_zero(&left) => *left,
+
+ // !A & A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // A & !A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // (..A..) & A --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&left, &right, BitwiseAnd) => *left,
+
+ // A & (..A..) --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&right, &left, BitwiseAnd) => *right,
+
+ // A & (A | B) --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&right)? && is_op_with(BitwiseOr, &right, &left) => {
+ *left
+ }
+
+ // (A | B) & A --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&left)? && is_op_with(BitwiseOr, &left, &right) => {
+ *right
+ }
+
+ //
+ // Rules for BitwiseOr
+ //
+
+ // A | null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseOr,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null | A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A | 0 -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_zero(&right) => *left,
+
+ // 0 | A -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_zero(&left) => *right,
+
+ // !A | A -> -1 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(-1)))
Review Comment:
Likewise I think can't always use `ScalarValue::Int32` -- it needs to use the appropriate variant of `ScalarValue` for the type of left/right
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -700,6 +701,288 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> {
return Err(DataFusionError::ArrowError(ArrowError::DivideByZero));
}
+ //
+ // Rules for BitwiseAnd
+ //
+
+ // A & null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null & A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A & 0 -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_zero(&right) => *right,
+
+ // 0 & A -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_zero(&left) => *left,
+
+ // !A & A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
Review Comment:
It seems like this should not always be `ScalarValue::Int32` -- if `left` and `right` are `DataType::Int64` for example, I think this should be `ScalarValue::Int64(Some(0))`. This applies to the other rules as well.
Perhaps we could add a method like `ScalarValue::new_zero(data_type: DataType)` to create such a constant
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -700,6 +701,288 @@ impl<'a, S: SimplifyInfo> ExprRewriter for Simplifier<'a, S> {
return Err(DataFusionError::ArrowError(ArrowError::DivideByZero));
}
+ //
+ // Rules for BitwiseAnd
+ //
+
+ // A & null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null & A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A & 0 -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseAnd,
+ right,
+ }) if is_zero(&right) => *right,
+
+ // 0 & A -> 0
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right: _,
+ }) if is_zero(&left) => *left,
+
+ // !A & A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // A & !A -> 0 (if A not nullable)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+ Expr::Literal(ScalarValue::Int32(Some(0)))
+ }
+
+ // (..A..) & A --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&left, &right, BitwiseAnd) => *left,
+
+ // A & (..A..) --> (..A..)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if expr_contains(&right, &left, BitwiseAnd) => *right,
+
+ // A & (A | B) --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&right)? && is_op_with(BitwiseOr, &right, &left) => {
+ *left
+ }
+
+ // (A | B) & A --> A (if B not null)
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseAnd,
+ right,
+ }) if !info.nullable(&left)? && is_op_with(BitwiseOr, &left, &right) => {
+ *right
+ }
+
+ //
+ // Rules for BitwiseOr
+ //
+
+ // A | null -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left: _,
+ op: BitwiseOr,
+ right,
+ }) if is_null(&right) => *right,
+
+ // null | A -> null
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right: _,
+ }) if is_null(&left) => *left,
+
+ // A | 0 -> A
+ Expr::BinaryExpr(BinaryExpr {
+ left,
+ op: BitwiseOr,
+ right,
+ }) if is_zero(&right) => *left,
Review Comment:
same thing here -- I think it only applies if `right` is not `NULL`
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -77,6 +78,80 @@ pub fn expr_contains(expr: &Expr, needle: &Expr, search_op: Operator) -> bool {
}
}
+/// This enum is needed for the function "delete_expressions_in_complex_expr" for
Review Comment:
```suggestion
/// This enum is needed for the function "delete_xor_in_complex_expr" for
```
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1346,6 +1629,361 @@ mod tests {
assert_eq!(simplify(expr), expected);
}
+ #[test]
+ fn test_simplify_bitwise_xor_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A ^ null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseXor, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null ^ A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseXor, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_shift_right_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A >> null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftRight, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null >> A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseShiftRight, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_shift_left_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A << null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftLeft, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null << A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseShiftLeft, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_and_by_zero() {
+ // A & 0 --> 0
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseAnd, lit(0));
+ assert_eq!(simplify(expr), lit(0));
+ }
+ // 0 & A --> 0
+ {
+ let expr = binary_expr(lit(0), Operator::BitwiseAnd, col("c2"));
+ assert_eq!(simplify(expr), lit(0));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_or_by_zero() {
+ // A | 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseOr, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ // 0 | A --> A
+ {
+ let expr = binary_expr(lit(0), Operator::BitwiseOr, col("c2"));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_xor_by_zero() {
+ // A ^ 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseXor, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ // 0 ^ A --> A
+ {
+ let expr = binary_expr(lit(0), Operator::BitwiseXor, col("c2"));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_bitwise_shift_right_by_zero() {
+ // A >> 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftRight, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_bitwise_shift_left_by_zero() {
+ // A << 0 --> A
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseShiftLeft, lit(0));
+ assert_eq!(simplify(expr), col("c2"));
+ }
+ }
+
+ #[test]
+ fn test_simplify_bitwise_and_by_null() {
+ let null = Expr::Literal(ScalarValue::Null);
+ // A & null --> null
+ {
+ let expr = binary_expr(col("c2"), Operator::BitwiseAnd, null.clone());
+ assert_eq!(simplify(expr), null);
+ }
+ // null & A --> null
+ {
+ let expr = binary_expr(null.clone(), Operator::BitwiseAnd, col("c2"));
+ assert_eq!(simplify(expr), null);
+ }
+ }
+
+ #[test]
+ fn test_simplify_composed_bitwise_and() {
+ // ((c > 5) & (c1 < 6)) & (c > 5) --> ((c > 5) & c1)
Review Comment:
I think this should refer to c1 and c2 (rather than c and c1)
```
// ((c2 > 5) & (c1 < 6)) & (c2 > 5) --> ((c2 > 5) & c1)
```
It might be valuable to add a test for the other grouping as well:
```
// (c > 5) & ((c1 < 6) & (c > 5)) --> c1 & (c > 5)
```
##########
datafusion/optimizer/src/simplify_expressions/utils.rs:
##########
@@ -154,11 +229,16 @@ pub fn is_op_with(target_op: Operator, haystack: &Expr, needle: &Expr) -> bool {
matches!(haystack, Expr::BinaryExpr(BinaryExpr { left, op, right }) if op == &target_op && (needle == left.as_ref() || needle == right.as_ref()))
}
-/// returns true if `not_expr` is !`expr`
+/// returns true if `not_expr` is !`expr` (not)
pub fn is_not_of(not_expr: &Expr, expr: &Expr) -> bool {
matches!(not_expr, Expr::Not(inner) if expr == inner.as_ref())
}
+/// returns true if `not_expr` is !`expr` (bitwise not)
+pub fn is_negative_of(not_expr: &Expr, expr: &Expr) -> bool {
+ matches!(not_expr, Expr::Negative(inner) if expr == inner.as_ref())
Review Comment:
Shouldn't this be checking for `Expr::Not` rather than `Expr::Negative`?
```suggestion
matches!(not_expr, Expr::Not(inner) if expr == inner.as_ref())
```
--
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