Factorize common AND factors out of OR predicates to support filterPu… #3859

Ted-Jiang · 2022-10-17T14:17:00Z

…shDown as possible

Signed-off-by: yangjiang yangjiang@ebay.com

Which issue does this PR close?

Rationale for this change

A predicate is of the from p AND q1 OR p AND q2 it would help to factorize out the AND factor p, to the equivalent expression p AND (q1 OR q2). This is useful when p in predicates can be pushed down without q.

What changes are included in this PR?

Are there any user-facing changes?

…shDown as possible Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang · 2022-10-17T14:17:55Z

datafusion/optimizer/src/filter_push_down.rs

+        // rewrite to CNF
+        // (c = 1 OR c = 1) [can pushDown] AND (c = 1 OR b > 3) AND (b > 2 OR C = 1) AND (b > 2 OR b > 3)
+
+        let expected = "\


It used to get not pushed:

Filter: test.c = Int64(1) AND b > Int64(2) OR test.c = Int64(1) AND b > Int64(3) Aggregate: groupBy=[[test.a]], aggr=[[SUM(test.b) AS b]] TableScan: test

@alamb @andygrove @Dandandan Do you think this is a reasonable improvement 🤔?
If this ok, i will modify the another test😂

I think it is a good improvement for sure -- thank you

Ted-Jiang · 2022-10-17T14:20:56Z

datafusion/optimizer/src/filter_push_down.rs

+        let expected = "\
+            Filter: test.c = Int64(1) OR b > Int64(3) AND b > Int64(2) OR test.c = Int64(1) AND b > Int64(2) OR b > Int64(3)\
+            \n  Aggregate: groupBy=[[test.a]], aggr=[[SUM(test.b) AS b]]\
+            \n    Filter: test.c = Int64(1) OR test.c = Int64(1)\


Somewhere need simplify this

I think this should be a rule in https://github.com/apache/arrow-datafusion/blob/37fe938261636bb7710b26cf26e2bbd010e1dbf0/datafusion/optimizer/src/simplify_expressions.rs#L264

That pass gets run several times

I recommend we file a follow on ticket for that particular rewrite (the same thing applies to a AND a in addition to a OR a

Filed #3895

Dandandan · 2022-10-17T14:49:27Z

Seems useful and correct to me - makes sense for predicate pushdown indeed.

Dandandan · 2022-10-17T14:49:56Z

Note that one of the tests now fails because of the optimization 👍

Dandandan · 2022-10-17T14:52:31Z

datafusion/optimizer/src/filter_push_down.rs

        LogicalPlan::Filter(filter) => {
-            let predicates = utils::split_conjunction(filter.predicate());
+            let filter_cnf = utils::CnfHelper::new().rewrite_to_cnf_impl(filter.predicate());
+            let predicates = utils::split_conjunction(&filter_cnf);


I wonder whether we should do this in filter pushdown or in the expression simplifier optimization?

I recommend putting it in SimplifyExpressions -- specifically call it from https://github.com/apache/arrow-datafusion/blob/37fe938261636bb7710b26cf26e2bbd010e1dbf0/datafusion/optimizer/src/simplify_expressions.rs#L264 somehow

You could also reuse the testing framework in that file as well

@alamb @Dandandan I prefer keep this just in filter_push_down, as you seen in test_rewrite_cnf_overflow, this is a heavy operation. It makes the exprs size expansion. IMO, we only need to spilt the filter expr to CNF to push filter down as many as possible, we should use normal expr to pass throuh plan.🤔

alamb

This is looking great @Ted-Jiang -- thank you.

alamb · 2022-10-17T21:54:20Z

datafusion/optimizer/src/utils.rs

    }
 }

+///  Converts an expression to conjunctive normal form (CNF).


💯 for this comment

alamb · 2022-10-17T21:57:06Z

datafusion/optimizer/src/filter_push_down.rs

        LogicalPlan::Filter(filter) => {
-            let predicates = utils::split_conjunction(filter.predicate());
+            let filter_cnf = utils::CnfHelper::new().rewrite_to_cnf_impl(filter.predicate());
+            let predicates = utils::split_conjunction(&filter_cnf);


I recommend putting it in SimplifyExpressions -- specifically call it from https://github.com/apache/arrow-datafusion/blob/37fe938261636bb7710b26cf26e2bbd010e1dbf0/datafusion/optimizer/src/simplify_expressions.rs#L264 somehow

You could also reuse the testing framework in that file as well

alamb · 2022-10-17T21:58:45Z

datafusion/optimizer/src/filter_push_down.rs

+        let expected = "\
+            Filter: test.c = Int64(1) OR b > Int64(3) AND b > Int64(2) OR test.c = Int64(1) AND b > Int64(2) OR b > Int64(3)\
+            \n  Aggregate: groupBy=[[test.a]], aggr=[[SUM(test.b) AS b]]\
+            \n    Filter: test.c = Int64(1) OR test.c = Int64(1)\


I think this should be a rule in https://github.com/apache/arrow-datafusion/blob/37fe938261636bb7710b26cf26e2bbd010e1dbf0/datafusion/optimizer/src/simplify_expressions.rs#L264

That pass gets run several times

I recommend we file a follow on ticket for that particular rewrite (the same thing applies to a AND a in addition to a OR a

alamb · 2022-10-17T21:59:37Z

datafusion/optimizer/src/utils.rs

+        self.current_count >= self.max_count
+    }
+
+    pub fn rewrite_to_cnf_impl(&mut self, expr: &Expr) -> Expr {


I think if you used the ExprRewriter framework here you would avoid many of the clones

I will try this.

alamb · 2022-10-17T22:00:03Z

datafusion/optimizer/src/filter_push_down.rs

+        // rewrite to CNF
+        // (c = 1 OR c = 1) [can pushDown] AND (c = 1 OR b > 3) AND (b > 2 OR C = 1) AND (b > 2 OR b > 3)
+
+        let expected = "\


I think it is a good improvement for sure -- thank you

Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang · 2022-10-18T13:55:42Z

datafusion/optimizer/src/utils.rs

+        // Test rewrite on a1_and_b2 and a2_and_b1 -> (((a1 and b2) and a2) and b1)
+        let mut helper = CnfHelper::new();
+        let expr1 = and(and(a_1.clone(), b_2.clone()), and(a_2.clone(), b_1.clone()));
+        let expect = and(a_1.clone(), b_2.clone())


We need improve the Expr fmt: like

let expr1 = and(a_1.clone(), b_2.clone()) .and(a_2.clone()) .and(b_1.clone()); let expr2 = and(and(a_1.clone(), b_2.clone()), and(a_2.clone(), b_1.clone())); assert_eq!(expr1, expr2);

This is not equal but they have the same string format:

a = Int64(1) AND b = Int64(2) AND a = Int64(2) AND b = Int64(1)

This is not equal but they have the same string format:

I agree this is not ideal

Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang · 2022-10-19T07:09:41Z

datafusion/optimizer/src/utils.rs

 /// ```
-pub fn split_conjunction_owned(expr: Expr) -> Vec<Expr> {
-    split_conjunction_owned_impl(expr, vec![])
+pub fn split_conjunction_owned(expr: Expr, op: Operator) -> Vec<Expr> {


@alamb I think we could support on split on other types.

I agree this is useful -- though I would prefer this function to remain named split_conjunction (as the conjunction implies AND and instead have a new function called split_operator or something. Maybe I can make a quick PR

alamb

I think this looks really nice @Ted-Jiang 🏅

thank you

alamb · 2022-10-19T19:02:19Z

datafusion/optimizer/src/utils.rs

+        // Test rewrite on a1_and_b2 and a2_and_b1 -> (((a1 and b2) and a2) and b1)
+        let mut helper = CnfHelper::new();
+        let expr1 = and(and(a_1.clone(), b_2.clone()), and(a_2.clone(), b_1.clone()));
+        let expect = and(a_1.clone(), b_2.clone())


This is not equal but they have the same string format:

I agree this is not ideal

alamb · 2022-10-19T19:03:30Z

datafusion/optimizer/src/utils.rs

 /// ```
-pub fn split_conjunction_owned(expr: Expr) -> Vec<Expr> {
-    split_conjunction_owned_impl(expr, vec![])
+pub fn split_conjunction_owned(expr: Expr, op: Operator) -> Vec<Expr> {


I agree this is useful -- though I would prefer this function to remain named split_conjunction (as the conjunction implies AND and instead have a new function called split_operator or something. Maybe I can make a quick PR

alamb · 2022-10-19T19:04:50Z

datafusion/optimizer/src/utils.rs

+    }
+
+    #[test]
+    fn test_rewrite_cnf_overflow() {


alamb · 2022-10-19T19:11:39Z

datafusion/optimizer/src/filter_push_down.rs

+        let expected = "\
+            Filter: test.c = Int64(1) OR b > Int64(3) AND b > Int64(2) OR test.c = Int64(1) AND b > Int64(2) OR b > Int64(3)\
+            \n  Aggregate: groupBy=[[test.a]], aggr=[[SUM(test.b) AS b]]\
+            \n    Filter: test.c = Int64(1) OR test.c = Int64(1)\


Filed #3895

alamb · 2022-10-19T20:12:43Z

🤔 this seems to have caused a logical merge conflict: https://github.com/apache/arrow-datafusion/actions/runs/3284319035/jobs/5410351533

I am working on a fix

alamb · 2022-10-19T20:14:23Z

Actually, I think the regression is pretty bad (there are many filters) -- I am going to revert this PR to get the tests green again. I am really sorry @Ted-Jiang

…filterPu… (apache#3859)" This reverts commit ddfd052.

…filterPu… (#3859)" (#3897) This reverts commit ddfd052.

alamb · 2022-10-19T20:20:12Z

@Ted-Jiang can you please re-create this PR (or I can do so to) and we can iterate on it some more. I am really sorry about that

Ted-Jiang · 2022-10-20T02:25:09Z

@Ted-Jiang can you please re-create this PR (or I can do so to) and we can iterate on it some more. I am really sorry about that

@alamb of course！This is not your fault 😂

Ted-Jiang · 2022-10-20T02:30:33Z

Actually, I think the regression is pretty bad (there are many filters) -- I am going to revert this PR to get the tests green again. I am really sorry @Ted-Jiang

seems strange it pass in my local.

Factorize common AND factors out of OR predicates to support filterPu…

3e5ea52

…shDown as possible Signed-off-by: yangjiang <yangjiang@ebay.com>

github-actions bot added the optimizer Optimizer rules label Oct 17, 2022

Ted-Jiang commented Oct 17, 2022

View reviewed changes

Dandandan reviewed Oct 17, 2022

View reviewed changes

alamb reviewed Oct 17, 2022

View reviewed changes

Ted-Jiang added 3 commits October 18, 2022 21:28

add test and use ExprRewriter framework

9b89398

Signed-off-by: yangjiang <yangjiang@ebay.com>

Merge branch 'master' into Issue_3858

bd101a3

add test and use farmework 2

3e076a0

Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang commented Oct 18, 2022

View reviewed changes

Ted-Jiang mentioned this pull request Oct 18, 2022

Improve the Expr string format #3878

Closed

Ted-Jiang added 2 commits October 18, 2022 21:59

rebase master for binaryExpr change

fce97fe

Signed-off-by: yangjiang <yangjiang@ebay.com>

fix tests and support split_conjunction on other type

e8cda82

Signed-off-by: yangjiang <yangjiang@ebay.com>

Ted-Jiang commented Oct 19, 2022

View reviewed changes

Ted-Jiang requested a review from alamb October 19, 2022 07:15

github-actions bot added the core Core DataFusion crate label Oct 19, 2022

Ted-Jiang requested review from Dandandan and alamb and removed request for Dandandan and alamb October 19, 2022 08:09

alamb mentioned this pull request Oct 19, 2022

Simplify a AND a and a OR a. #3895

Closed

alamb approved these changes Oct 19, 2022

View reviewed changes

alamb merged commit ddfd052 into apache:master Oct 19, 2022

alamb mentioned this pull request Oct 19, 2022

Rename some split functions #3896

Closed

alamb added a commit to alamb/datafusion that referenced this pull request Oct 19, 2022

Revert "Factorize common AND factors out of OR predicates to support …

e6f6706

…filterPu… (apache#3859)" This reverts commit ddfd052.

This was referenced Oct 19, 2022

Revert "Factorize common AND factors out of OR predicates to supportfilter Pu… (#3859)" #3897

Merged

Factorize common AND factors out of OR predicates to support filterPushDown as possible #3858

Closed

alamb added a commit that referenced this pull request Oct 19, 2022

Revert "Factorize common AND factors out of OR predicates to support …

57e445a

…filterPu… (#3859)" (#3897) This reverts commit ddfd052.

isidentical mentioned this pull request Oct 21, 2022

Add test for Simplify redundant predicates #3915

Merged

Factorize common AND factors out of OR predicates to support filterPu… #3859

Factorize common AND factors out of OR predicates to support filterPu… #3859

Uh oh!

Conversation

Ted-Jiang commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Ted-Jiang Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ted-Jiang Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dandandan commented Oct 17, 2022

Uh oh!

Dandandan commented Oct 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Oct 19, 2022

Uh oh!

alamb commented Oct 19, 2022

Uh oh!

alamb commented Oct 19, 2022

Uh oh!

Ted-Jiang commented Oct 20, 2022

Uh oh!

Ted-Jiang commented Oct 20, 2022

Ted-Jiang commented Oct 17, 2022 •

edited

Loading

Ted-Jiang Oct 17, 2022 •

edited

Loading

Ted-Jiang Oct 17, 2022 •

edited

Loading