feat: add optimization rules for bitwise operations #5423

izveigor · 2023-02-27T18:08:40Z

alamb

Thank you @izveigor -- this PR looks quite nice. The code is clean and well commented 🏅 and easy to read

I had a few comments but otherwise I think this PR is close to ready

alamb · 2023-02-28T21:05:57Z

datafusion/expr/src/expr.rs

@@ -639,6 +639,31 @@ impl Expr {
        binary_expr(self, Operator::Or, other)
    }

+    /// Return `self & other`


alamb · 2023-02-28T21:07:55Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+            // A & 0 -> 0
+            Expr::BinaryExpr(BinaryExpr {
+                left: _,
+                op: BitwiseAnd,
+                right,
+            }) if is_zero(&right) => *right,


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)

postgres=# select NULL & 0 ; ?column? ---------- (1 row)

The same comment applies to 0 & A as well

datafusion/optimizer/src/simplify_expressions/utils.rs

alamb · 2023-02-28T21:12:29Z

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


Suggested change

/// This enum is needed for the function "delete_expressions_in_complex_expr" for

/// This enum is needed for the function "delete_xor_in_complex_expr" for

alamb · 2023-02-28T21:14:44Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+
+    #[test]
+    fn test_simplify_composed_bitwise_and() {
+        // ((c > 5) & (c1 < 6)) & (c > 5) --> ((c > 5) & c1)


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)

alamb · 2023-02-28T21:17:37Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                left,
+                op: BitwiseOr,
+                right,
+            }) if is_zero(&right) => *left,


same thing here -- I think it only applies if right is not NULL

alamb · 2023-02-28T21:20:49Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                op: BitwiseAnd,
+                right,
+            }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+                Expr::Literal(ScalarValue::Int32(Some(0)))


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

alamb · 2023-02-28T21:21:38Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                op: BitwiseOr,
+                right,
+            }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+                Expr::Literal(ScalarValue::Int32(Some(-1)))


Likewise I think can't always use ScalarValue::Int32 -- it needs to use the appropriate variant of ScalarValue for the type of left/right

alamb · 2023-02-28T21:23:17Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                op: BitwiseXor,
+                right,
+            }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+                Expr::Literal(ScalarValue::Int32(Some(-1)))


needs the right ScalarValue variant

alamb · 2023-02-28T21:23:21Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                op: BitwiseXor,
+                right,
+            }) if is_negative_of(&right, &left) && !info.nullable(&left)? => {
+                Expr::Literal(ScalarValue::Int32(Some(-1)))


needs the right ScalarValue variant

izveigor · 2023-03-02T16:26:56Z

Hello, @alamb!
Thanks to your response, I took into account your comments and fixed all defects. I hope this PR will be merged into main soon.

alamb · 2023-03-03T22:11:50Z

I plan to review this PR tomorrow

izveigor · 2023-03-04T16:32:20Z

datafusion/optimizer/src/simplify_expressions/utils.rs

+}
+
+/// Returns a new int type based on data type of an expression
+pub fn new_int_by_expr_data_type(val: i64, expr_data_type: &DataType) -> Expr {


The behavior of the function is affected by the following PR: #5476

does it need to be pub?

izveigor · 2023-03-04T16:34:17Z

datafusion/optimizer/src/simplify_expressions/context.rs

@@ -123,6 +126,18 @@ impl<'a> SimplifyInfo for SimplifyContext<'a> {
            })
    }

+    /// Returns data type of this expr needed for determining optimized int type of a value
+    fn get_data_type(&self, expr: &Expr) -> DataType {


Should Int32 be the default value?

I think this should simply return the error if the data type can not be determined -- it seems wrong to treat it as a 32 bit number 🤔 .

alamb

This s looking very good @izveigor . Thank you. The test coverage is especially thorough. I had a few style comments/ suggestions but all I think is needed prior to merge is:

address https://github.com/apache/arrow-datafusion/pull/5423/files#r1120773109
Don't ignore errors in get_datatype

Nice to have:

use ScalarValue::zero / consolidate new_int_by_expr_data_type in ScalarValue

alamb · 2023-03-04T16:42:16Z

datafusion/optimizer/src/simplify_expressions/context.rs

@@ -123,6 +126,18 @@ impl<'a> SimplifyInfo for SimplifyContext<'a> {
            })
    }

+    /// Returns data type of this expr needed for determining optimized int type of a value
+    fn get_data_type(&self, expr: &Expr) -> DataType {


I think this should simply return the error if the data type can not be determined -- it seems wrong to treat it as a 32 bit number 🤔 .

alamb · 2023-03-04T16:43:25Z

datafusion/optimizer/src/simplify_expressions/utils.rs

+}
+
+/// Returns a new int type based on data type of an expression
+pub fn new_int_by_expr_data_type(val: i64, expr_data_type: &DataType) -> Expr {


does it need to be pub?

alamb · 2023-03-04T16:46:51Z

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

+                op: BitwiseAnd,
+                right,
+            }) if is_negative_of(&left, &right) && !info.nullable(&right)? => {
+                new_int_by_expr_data_type(0, &info.get_data_type(&left))


I think ScalarValue::new_zero might be better here

https://github.com/apache/arrow-datafusion/blob/e9852074bacd8c891d84eba38b3417aa16a2d18c/datafusion/common/src/scalar.rs#L1023-L1043

I think you could perhaps add a either new_negative_one or new_int(val: i64, data_type: &DataType) -> Result<ScalarValue> to handle the -1 case

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

alamb · 2023-03-04T16:54:37Z

datafusion/optimizer/src/simplify_expressions/utils.rs

@@ -77,6 +78,61 @@ pub fn expr_contains(expr: &Expr, needle: &Expr, search_op: Operator) -> bool {
    }
 }

+/// Deletes all 'needles' or remains one 'needle' that are found in a chain of xor
+/// expressions. Such as: A ^ (A ^ (B ^ A))
+pub fn delete_xor_in_complex_expr(expr: &Expr, needle: &Expr, is_left: bool) -> Expr {


I wonder if you could implement this using ExprRewriter: https://github.com/apache/arrow-datafusion/blob/e9852074bacd8c891d84eba38b3417aa16a2d18c/datafusion/expr/src/utils.rs#L519

🤔

datafusion/optimizer/src/simplify_expressions/utils.rs

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

…s nullable

alamb

Thank you @izveigor -- I think this looks very nice. Thanks for sticking with it.

alamb · 2023-03-07T20:11:58Z

datafusion/core/tests/simplification.rs

@@ -49,6 +49,13 @@ impl SimplifyInfo for MyInfo {
    fn execution_props(&self) -> &ExecutionProps {
        &self.execution_props
    }
+
+    fn get_data_type(&self, expr: &Expr) -> Result<DataType> {
+        match expr.get_type(&self.schema) {


I think this can be expressed slightly more concisely -- see #5510

Agree, this code is redundant.

feat: add optimization rules for bitwise operations

2ed544e

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Feb 27, 2023

alamb reviewed Feb 28, 2023

View reviewed changes

izveigor added 3 commits March 2, 2023 16:34

fix: cargo fmt

1add43f

fix: comments, determining data type of an int literal, additional tests

febd022

fix: add "get_data_type" to impl SimplifyInfo for MyInfo

ef983ae

github-actions bot added the core Core DataFusion crate label Mar 2, 2023

izveigor added 3 commits March 2, 2023 17:23

fix: add behavior of the function "get_data_type"

35f741e

add information about "get_data_type" to the docs

59efc92

fix: doc tests for "expr_simplifier.rs"

34208c9

fix: delete_xor_in_complex_expr

f792392

izveigor commented Mar 4, 2023

View reviewed changes

alamb reviewed Mar 4, 2023

View reviewed changes

izveigor added 6 commits March 5, 2023 18:25

fix: some bitwise optimization rules, that can work even if an expr i…

c7312a5

…s nullable

fix: prefer ScalarValue methods over "new_int_by_expr_data_type"

d2779df

fix: resolve merge conflict

9d71417

fix: ScalarValue::new_negative_one

336aa4b

fix: get_data_type

a2a6706

fix: cargo clippy and fmt style

5ee2f5b

alamb approved these changes Mar 7, 2023

View reviewed changes

alamb merged commit d7b2212 into apache:main Mar 7, 2023

alamb mentioned this pull request Mar 7, 2023

minor: improve error style #5510

Merged

alamb reviewed Mar 7, 2023

View reviewed changes

alamb mentioned this pull request Mar 7, 2023

Simplify simplify test cases, support ^, &, |, << and >> operators for building exprs #5511

Merged

This was referenced Mar 8, 2023

feat: add the similar optimization function for bitwise negative #5516

Merged

feat: support type coercion for negative operator #5550

Closed

andygrove added the enhancement New feature or request label Mar 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add optimization rules for bitwise operations #5423

feat: add optimization rules for bitwise operations #5423

izveigor commented Feb 27, 2023

alamb left a comment

alamb Feb 28, 2023

alamb Feb 28, 2023

alamb Feb 28, 2023

alamb Feb 28, 2023

alamb Feb 28, 2023

alamb Feb 28, 2023

alamb Feb 28, 2023

alamb Feb 28, 2023

alamb Feb 28, 2023

izveigor commented Mar 2, 2023

alamb commented Mar 3, 2023

izveigor Mar 4, 2023

alamb Mar 4, 2023

izveigor Mar 4, 2023

alamb Mar 4, 2023

alamb left a comment

alamb Mar 4, 2023

alamb Mar 4, 2023

alamb Mar 4, 2023

alamb Mar 4, 2023

alamb Mar 4, 2023

alamb left a comment

alamb Mar 7, 2023

izveigor Mar 7, 2023

	/// This enum is needed for the function "delete_expressions_in_complex_expr" for
	/// This enum is needed for the function "delete_xor_in_complex_expr" for

feat: add optimization rules for bitwise operations #5423

feat: add optimization rules for bitwise operations #5423

Conversation

izveigor commented Feb 27, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izveigor commented Mar 2, 2023

alamb commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment