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

Ted-Jiang · 2022-10-20T07:55:21Z

…shDown as possible

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

Which issue does this PR close?

Closes #3858 .

Rationale for this change

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-20T08:00:49Z

datafusion/optimizer/src/filter_push_down.rs

@@ -797,7 +817,7 @@ impl OptimizerRule for FilterPushDown {
        plan: &LogicalPlan,
        _: &mut OptimizerConfig,
    ) -> Result<LogicalPlan> {
-        optimize(plan, State::default())
+        optimize(plan, State::default().with_cnf_rewrite())


@alamb test failed cause of after #3880, it runs rules multiple times.
When after running first time it will produce filter front (near scan) the join, but we don't need write the filter-expr after pushDown (expect page index, i will manually add it), so i add this option.🤔

…on_owned

alamb

Thanks @Ted-Jiang -- I have one set of proposed changes here: Ted-Jiang#27

alamb · 2022-10-20T18:09:33Z

datafusion/optimizer/src/filter_push_down.rs

@@ -80,6 +83,11 @@ impl State {
            .zip(predicates.1)
            .for_each(|(expr, cols)| self.filters.push((expr.clone(), cols.clone())))
    }
+
+    fn with_cnf_rewrite(mut self) -> Self {


I think this should have some doccomments about what it is and when one it should/should not be set

alamb · 2022-10-20T18:14:10Z

datafusion/optimizer/src/utils.rs

+    }
+}
+
+impl ExprRewriter for CnfHelper {


I feel like this code is a little confusing -- it isn't really doing a rewrite the same way other ExprRewriters do. Instead it is doing an analysis in one pass through (via pre_visit) and then replacing it in one go

I also feel like number of clones is significant -- it both copies the original expr (in case the rewrite doesn't work) but also copies the exprs in the BinaryExprs

I feel like there must be a way to avoid at least one of those copies 🤔 But I am not sure without trying it myself

@Ted-Jiang did you try to simplify this further -- if not, would you like me to try and find time to give it a try?

@alamb you can try this 😂, maybe i will learn something from you, i am not very familiar with this rewrite framework

I can continue to work on page index

add split_binary_owned rather than change signature of split_conjuction_owned

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

Ted-Jiang · 2022-10-22T13:47:58Z

@alamb Sorry for the late respond.

I feel like this code is a little confusing -- it isn't really doing a rewrite the same way other ExprRewriters do. Instead it is doing an analysis in one pass through (via pre_visit) and then replacing it in one go

Yes😂, i think it because not mutate in the same place, this rewrite build an expr tress in another shape. I try to add split_binary to avoid some clone calls.

Ted-Jiang · 2022-10-22T13:54:06Z

Thanks @Ted-Jiang -- I have one set of proposed changes here: Ted-Jiang#27

Thanks a lot !😄

alamb · 2022-10-24T21:01:50Z

benchmarks/expected-plans/q7.txt

@@ -3,7 +3,7 @@ Sort: shipping.supp_nation ASC NULLS LAST, shipping.cust_nation ASC NULLS LAST,
    Aggregate: groupBy=[[shipping.supp_nation, shipping.cust_nation, shipping.l_year]], aggr=[[SUM(shipping.volume)]]
      Projection: shipping.supp_nation, shipping.cust_nation, shipping.l_year, shipping.volume, alias=shipping
        Projection: n1.n_name AS supp_nation, n2.n_name AS cust_nation, datepart(Utf8("YEAR"), lineitem.l_shipdate) AS l_year, CAST(lineitem.l_extendedprice AS Decimal128(38, 4)) * CAST(Decimal128(Some(100),23,2) - CAST(lineitem.l_discount AS Decimal128(23, 2)) AS Decimal128(38, 4)) AS volume, alias=shipping
-          Filter: n1.n_name = Utf8("FRANCE") AND n2.n_name = Utf8("GERMANY") OR n1.n_name = Utf8("GERMANY") AND n2.n_name = Utf8("FRANCE")
+          Filter: n1.n_name = Utf8("FRANCE") OR n2.n_name = Utf8("FRANCE") AND n2.n_name = Utf8("GERMANY") OR n1.n_name = Utf8("GERMANY")


this is so clever

alamb · 2022-10-25T20:10:48Z

@alamb you can try this 😂, maybe i will learn something from you, i am not very familiar with this rewrite framework

I can continue to work on page index

Sounds good ! I will try and find time tomorrow

alamb

@Ted-Jiang I changed how this worked a little -- any chance you can give it a look?

alamb · 2022-10-26T15:54:12Z

datafusion/optimizer/src/utils.rs

+pub fn cnf_rewrite(expr: Expr) -> Expr {
+    // Find all exprs joined by OR
+    let disjuncts = split_binary(&expr, Operator::Or);
+
+    // For each expr, split now on AND
+    // A OR B OR C --> split each A, B and C
+    let disjunct_conjuncts: VecDeque<Vec<&Expr>> = disjuncts
+        .into_iter()
+        .map(|e| split_binary(e, Operator::And))
+        .collect::<VecDeque<_>>();
+
+    // Decide if we want to distribute the clauses. Heuristic is
+    // chosen to avoid creating huge predicates
+    let num_conjuncts = disjunct_conjuncts
+        .iter()
+        .fold(1usize, |sz, exprs| sz.saturating_mul(exprs.len()));
+
+    if disjunct_conjuncts.iter().any(|exprs| exprs.len() > 1)
+        && num_conjuncts < MAX_CNF_REWRITE_CONJUNCTS
+    {
+        let or_clauses = permutations(disjunct_conjuncts)
+            .into_iter()
+            // form the OR clauses( A OR B OR C ..)
+            .map(|exprs| disjunction(exprs.into_iter().cloned()).unwrap());
+        conjunction(or_clauses).unwrap()
+    }
+    // otherwise return the original expression
+    else {
+        expr
+    }
+}


@Ted-Jiang I got nerdsniped working on this PR -- I hope you don't mind I rewrote your logic into something that avoided cloning unless it is actually doing the rewrite

Yes, without using the rewrite trait seems more clearly 👍

alamb · 2022-10-26T15:54:30Z

datafusion/optimizer/src/utils.rs

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


These tests are all the same

alamb · 2022-10-26T15:55:15Z

datafusion/optimizer/src/utils.rs

+    }
+}
+
+const MAX_CNF_REWRITE_CONJUNCTS: usize = 10;


By setting limiting the number of exprs to be created, I also avoided having to explicitly disable the cnf rewrite

Ted-Jiang · 2022-10-27T03:17:11Z

datafusion/optimizer/src/utils.rs

+
+    // Decide if we want to distribute the clauses. Heuristic is
+    // chosen to avoid creating huge predicates
+    let num_conjuncts = disjunct_conjuncts


Nice check!

Ted-Jiang · 2022-10-27T03:19:20Z

datafusion/optimizer/src/utils.rs

+pub fn cnf_rewrite(expr: Expr) -> Expr {
+    // Find all exprs joined by OR
+    let disjuncts = split_binary(&expr, Operator::Or);
+
+    // For each expr, split now on AND
+    // A OR B OR C --> split each A, B and C
+    let disjunct_conjuncts: VecDeque<Vec<&Expr>> = disjuncts
+        .into_iter()
+        .map(|e| split_binary(e, Operator::And))
+        .collect::<VecDeque<_>>();
+
+    // Decide if we want to distribute the clauses. Heuristic is
+    // chosen to avoid creating huge predicates
+    let num_conjuncts = disjunct_conjuncts
+        .iter()
+        .fold(1usize, |sz, exprs| sz.saturating_mul(exprs.len()));
+
+    if disjunct_conjuncts.iter().any(|exprs| exprs.len() > 1)
+        && num_conjuncts < MAX_CNF_REWRITE_CONJUNCTS
+    {
+        let or_clauses = permutations(disjunct_conjuncts)
+            .into_iter()
+            // form the OR clauses( A OR B OR C ..)
+            .map(|exprs| disjunction(exprs.into_iter().cloned()).unwrap());
+        conjunction(or_clauses).unwrap()
+    }
+    // otherwise return the original expression
+    else {
+        expr
+    }
+}


Yes, without using the rewrite trait seems more clearly 👍

Ted-Jiang · 2022-10-27T03:23:01Z

datafusion/optimizer/src/utils.rs

        }
-        Expr::Alias(expr, _) => split_conjunction_owned_impl(*expr, exprs),
+        Expr::Alias(expr, _) => split_binary_owned_impl(*expr, operator, exprs),


my bad 😂

I think that was actually my bad when I refactored it back to conjunction 🤦

Ted-Jiang · 2022-10-27T03:35:32Z

datafusion/optimizer/src/utils.rs

+        let or_clauses = permutations(disjunct_conjuncts)
+            .into_iter()
+            // form the OR clauses( A OR B OR C ..)
+            .map(|exprs| disjunction(exprs.into_iter().cloned()).unwrap());


seems we only clone the exprs once here🤔

Yes that is my analysis too -- this code will clone exprs only if it rewrites the expression -- if not, it will not clone anything

Ted-Jiang · 2022-10-27T03:39:36Z

@alamb Thanks for your help. This change looks better than mine 😄

alamb · 2022-10-27T14:15:26Z

@alamb Thanks for your help. This change looks better than mine 😄

Thanks for all your work on Page Index ❤️ !

alamb · 2022-10-27T14:17:13Z

Perhaps @isidentical or @mingmwang would have time to review this logic as well

isidentical · 2022-10-27T17:49:17Z

I'll take a look at it tonight, thanks for the ping @alamb!

isidentical

Amazing work people, love it!

isidentical · 2022-10-27T18:42:13Z

datafusion/optimizer/src/utils.rs

+
+    #[test]
+    fn test_permutations_two_and_one_and_two() {
+        // [[a, b], [c], [d, e]] --> [[a, c, d], [a, c, e], [b, c, d], [b, c, e]]


These are super useful, thanks 💯

isidentical · 2022-10-27T19:02:57Z

benchmarks/expected-plans/q7.txt

@@ -3,7 +3,7 @@ Sort: shipping.supp_nation ASC NULLS LAST, shipping.cust_nation ASC NULLS LAST,
    Aggregate: groupBy=[[shipping.supp_nation, shipping.cust_nation, shipping.l_year]], aggr=[[SUM(shipping.volume)]]
      Projection: shipping.supp_nation, shipping.cust_nation, shipping.l_year, shipping.volume, alias=shipping
        Projection: n1.n_name AS supp_nation, n2.n_name AS cust_nation, datepart(Utf8("YEAR"), lineitem.l_shipdate) AS l_year, CAST(lineitem.l_extendedprice AS Decimal128(38, 4)) * CAST(Decimal128(Some(100),23,2) - CAST(lineitem.l_discount AS Decimal128(23, 2)) AS Decimal128(38, 4)) AS volume, alias=shipping
-          Filter: n1.n_name = Utf8("FRANCE") AND n2.n_name = Utf8("GERMANY") OR n1.n_name = Utf8("GERMANY") AND n2.n_name = Utf8("FRANCE")
+          Filter: (n1.n_name = Utf8("FRANCE") OR n2.n_name = Utf8("FRANCE")) AND (n2.n_name = Utf8("GERMANY") OR n1.n_name = Utf8("GERMANY"))


I've also wanted to check TPC-H (it shouldn't affect, but just to see if there is an unexpected regression). It seems like there aren't any regressions (873.27 ms vs 878.54 ms, only noise) 🚀

@isidentical thanks for testing👍! I think this rewrite will make row_filter work, I think it will boost up query when row_filter is stable! 🤔

Thanks for checking @isidentical -- I think there are only 25 rows in the NATION table in TPCH, so the ordering of predicates doesn't really matter for performance in that case 😆

think there are only 25 rows in the NATION table in TPCH,

😂

alamb

I plan to merge this tomorrow unless anyone has any final thoughts

mingmwang · 2022-10-28T10:00:11Z

I think for the Physical FilterExec, we should still keep the original form or a simplified form for runtime evaluation.
And since we are working on the CBO stats and Filter selectivity estimation, it better to use the simplified form.

alamb · 2022-10-28T10:17:56Z

I think for the Physical FilterExec, we should still keep the original form or a simplified form for runtime evaluation.

@mingmwang I didn't quite understand this comment -- do you mean you think this PR is a good idea, or that it needs more work (to keep the original form)?

mingmwang · 2022-10-28T10:24:14Z

@alamb
I'm OK to merge the PR. But we might need a following PR to add a new rule to simply expressions before translate the logic Filter to physical FilterExec.

alamb · 2022-10-28T13:14:22Z

I'm OK to merge the PR. But we might need a following PR to add a new rule to simply expressions before translate the logic Filter to physical FilterExec.

I double checked @mingmwang

This code is called as part of FilterPushdown

https://github.com/apache/arrow-datafusion/blob/318b4ad56c826b317b0201f7031a47609ad215b5/datafusion/optimizer/src/optimizer.rs#L174

And then simplification is then called

https://github.com/apache/arrow-datafusion/blob/318b4ad56c826b317b0201f7031a47609ad215b5/datafusion/optimizer/src/optimizer.rs#L178-L183

so I think the simplifications should happen before physical planning

alamb · 2022-10-29T11:53:03Z

I merged this with master locally and all the tests pass -- so I am merging this one in

alamb · 2022-10-29T11:53:14Z

Thanks again @Ted-Jiang

ursabot · 2022-10-29T12:02:30Z

Benchmark runs are scheduled for baseline = 6fae0bd and contender = 3452345. 3452345 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

apache#3903) * Factorize common AND factors out of OR predicates to support filterPushDown as possible Signed-off-by: yangjiang <yangjiang@ebay.com> * add split_binary_owned rather than change signature of split_conjuction_owned * add `split_binary` and avoid some clone calls Signed-off-by: yangjiang <yangjiang@ebay.com> * Update plans * Simplify tests * Update tests * Rewrite CNF without recursion * Change heuristic * Keep cleaning up * tests pass * cleanup * cleanups * Clean up docstrings * Clippy, restore missing test Signed-off-by: yangjiang <yangjiang@ebay.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

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

280b3d1

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

github-actions bot added core Core DataFusion crate optimizer Optimizer rules labels Oct 20, 2022

Ted-Jiang commented Oct 20, 2022

View reviewed changes

add split_binary_owned rather than change signature of split_conjucti…

0faf776

…on_owned

alamb mentioned this pull request Oct 20, 2022

add split_binary_owned rather than change signature of split_conjuction_owned Ted-Jiang/arrow-datafusion#27

Merged

alamb reviewed Oct 20, 2022

View reviewed changes

Ted-Jiang added 2 commits October 22, 2022 21:00

Merge pull request #27 from alamb/alamb/small_cleaups

e556c7c

add split_binary_owned rather than change signature of split_conjuction_owned

add split_binary and avoid some clone calls

9f720c2

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

Ted-Jiang requested a review from alamb October 22, 2022 15:16

alamb reviewed Oct 24, 2022

View reviewed changes

Merge branch 'master' into issue_3858_3

40e686c

alamb added 11 commits October 26, 2022 08:30

Merge remote-tracking branch 'apache/master' into issue_3858_3

b233f5c

Update plans

7cd016c

Simplify tests

e3ff3f7

Update tests

d70c730

Rewrite CNF without recursion

564974e

Change heuristic

97afb9d

Keep cleaning up

2fd4166

tests pass

3f71f15

cleanup

16f1c1e

cleanups

76e91a7

Clean up docstrings

50d0664

alamb reviewed Oct 26, 2022

View reviewed changes

Clippy, restore missing test

6dd1ce3

Ted-Jiang commented Oct 27, 2022

View reviewed changes

Ted-Jiang requested a review from alamb October 27, 2022 03:51

isidentical approved these changes Oct 27, 2022

View reviewed changes

alamb reviewed Oct 28, 2022

View reviewed changes

alamb merged commit 3452345 into apache:master Oct 29, 2022

alamb mentioned this pull request Dec 5, 2022

fix push_down_projection push redundant columns. #4487

Merged

jackwener mentioned this pull request Dec 9, 2022

reimplement push_down_projection and prune_column. #4465

Merged

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

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

Conversation

Ted-Jiang commented Oct 20, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Ted-Jiang Oct 20, 2022 • edited Loading

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

Ted-Jiang Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ted-Jiang commented Oct 22, 2022

Ted-Jiang commented Oct 22, 2022

Choose a reason for hiding this comment

alamb commented Oct 25, 2022

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

alamb Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ted-Jiang commented Oct 27, 2022

alamb commented Oct 27, 2022

alamb commented Oct 27, 2022

isidentical commented Oct 27, 2022

isidentical left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidentical Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ted-Jiang Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

mingmwang commented Oct 28, 2022

alamb commented Oct 28, 2022

mingmwang commented Oct 28, 2022

alamb commented Oct 28, 2022

alamb commented Oct 29, 2022

alamb commented Oct 29, 2022

ursabot commented Oct 29, 2022

Ted-Jiang Oct 20, 2022 •

edited

Loading

Ted-Jiang Oct 25, 2022 •

edited

Loading

alamb Oct 27, 2022 •

edited

Loading

isidentical Oct 27, 2022 •

edited

Loading

Ted-Jiang Oct 28, 2022 •

edited

Loading