Multiple optimizer passes #3880

andygrove · 2022-10-18T15:08:50Z

Which issue does this PR close?

Closes #3879 and #3881

Possibly helps with #3864 but I have not tested that yet

Rationale for this change

We miss optimizations if we only run the rules once

What changes are included in this PR?

Are there any user-facing changes?

andygrove · 2022-10-18T15:09:21Z

datafusion/core/tests/sql/subqueries.rs

@@ -394,9 +394,9 @@ order by cntrycode;"#;
              Projection: AVG(customer.c_acctbal) AS __value, alias=__sq_1
                Aggregate: groupBy=[[]], aggr=[[AVG(customer.c_acctbal)]]
                  Filter: customer.c_acctbal > Decimal128(Some(0),15,2) AND substr(customer.c_phone, Int64(1), Int64(2)) IN ([Utf8("13"), Utf8("31"), Utf8("23"), Utf8("29"), Utf8("30"), Utf8("18"), Utf8("17")])
-                    TableScan: customer projection=[c_phone, c_acctbal], partial_filters=[CAST(customer.c_acctbal AS Decimal128(30, 15)) > Decimal128(Some(0),30,15), substr(customer.c_phone, Int64(1), Int64(2)) IN ([Utf8("13"), Utf8("31"), Utf8("23"), Utf8("29"), Utf8("30"), Utf8("18"), Utf8("17")])]"#
+                    TableScan: customer projection=[c_phone, c_acctbal], partial_filters=[CAST(customer.c_acctbal AS Decimal128(30, 15)) > Decimal128(Some(0),30,15), substr(customer.c_phone, Int64(1), Int64(2)) IN ([Utf8("13"), Utf8("31"), Utf8("23"), Utf8("29"), Utf8("30"), Utf8("18"), Utf8("17")]), customer.c_acctbal > Decimal128(Some(0),15,2)]"#


Example of an optimization that we previously missed

andygrove · 2022-10-18T15:24:58Z

benchmarks/expected-plans/q19.txt

@@ -3,7 +3,7 @@ Projection: SUM(lineitem.l_extendedprice * Int64(1) - lineitem.l_discount) AS re
    Projection: lineitem.l_extendedprice, lineitem.l_discount
      Filter: part.p_brand = Utf8("Brand#12") AND part.p_container IN ([Utf8("SM CASE"), Utf8("SM BOX"), Utf8("SM PACK"), Utf8("SM PKG")]) AND lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) AND part.p_size <= Int32(5) OR part.p_brand = Utf8("Brand#23") AND part.p_container IN ([Utf8("MED BAG"), Utf8("MED BOX"), Utf8("MED PKG"), Utf8("MED PACK")]) AND lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) AND part.p_size <= Int32(10) OR part.p_brand = Utf8("Brand#34") AND part.p_container IN ([Utf8("LG CASE"), Utf8("LG BOX"), Utf8("LG PACK"), Utf8("LG PKG")]) AND lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2) AND part.p_size <= Int32(15)
        Inner Join: lineitem.l_partkey = part.p_partkey
-          Filter: lineitem.l_shipmode IN ([Utf8("AIR"), Utf8("AIR REG")]) AND lineitem.l_shipinstruct = Utf8("DELIVER IN PERSON") AND lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) OR lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2)
+          Filter: lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) OR lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2) AND lineitem.l_shipmode IN ([Utf8("AIR"), Utf8("AIR REG")]) AND lineitem.l_shipinstruct = Utf8("DELIVER IN PERSON")


The order changed here

andygrove · 2022-10-18T15:25:12Z

benchmarks/expected-plans/q19.txt

            TableScan: lineitem projection=[l_partkey, l_quantity, l_extendedprice, l_discount, l_shipinstruct, l_shipmode]
-          Filter: part.p_size >= Int32(1) AND part.p_brand = Utf8("Brand#12") AND part.p_container IN ([Utf8("SM CASE"), Utf8("SM BOX"), Utf8("SM PACK"), Utf8("SM PKG")]) AND part.p_size <= Int32(5) OR part.p_brand = Utf8("Brand#23") AND part.p_container IN ([Utf8("MED BAG"), Utf8("MED BOX"), Utf8("MED PKG"), Utf8("MED PACK")]) AND part.p_size <= Int32(10) OR part.p_brand = Utf8("Brand#34") AND part.p_container IN ([Utf8("LG CASE"), Utf8("LG BOX"), Utf8("LG PACK"), Utf8("LG PKG")]) AND part.p_size <= Int32(15)
-            TableScan: part projection=[p_partkey, p_brand, p_size, p_container]
+          Filter: part.p_brand = Utf8("Brand#12") AND part.p_container IN ([Utf8("SM CASE"), Utf8("SM BOX"), Utf8("SM PACK"), Utf8("SM PKG")]) AND part.p_size <= Int32(5) OR part.p_brand = Utf8("Brand#23") AND part.p_container IN ([Utf8("MED BAG"), Utf8("MED BOX"), Utf8("MED PKG"), Utf8("MED PACK")]) AND part.p_size <= Int32(10) OR part.p_brand = Utf8("Brand#34") AND part.p_container IN ([Utf8("LG CASE"), Utf8("LG BOX"), Utf8("LG PACK"), Utf8("LG PKG")]) AND part.p_size <= Int32(15) AND part.p_size >= Int32(1)


The order changed here

benchmarks/src/bin/tpch.rs

andygrove · 2022-10-18T16:14:57Z

datafusion/optimizer/src/projection_push_down.rs

+    let with_dupe_projection_removed = match new_plan? {
+        LogicalPlan::Projection(p) => match p.input.as_ref() {
+            LogicalPlan::Projection(p2) if projection_equal(&p, p2) => {
+                LogicalPlan::Projection(p2.clone())
+            }
+            _ => LogicalPlan::Projection(p),
+        },
+        other => other,
+    };


This is the fix for #3881

isidentical · 2022-10-18T16:57:45Z

datafusion/optimizer/src/optimizer.rs

+            // TODO this is an expensive way to see if the optimizer did anything and
+            // it would be better to change the OptimizerRule trait to return an Option
+            // instead
+            let new_plan_str = format!("{}", new_plan.display_indent());
+            if plan_str == new_plan_str {
+                // plan did not change, so no need to continue trying to optimize
+                debug!("optimizer pass {} did not make changes", i);
+                break;
+            }


+1! This is probably a follow-up item; but instead of having the optimizer decide on this (by returning an option), it might also make sense to compute a unique plan id (bottom up) so that we can also use this to detect optimization cycles.

A very basic example is (assuming each letter is a unique plan id) A -> B -> C -> A -> B -> [max passes times more], where even though the previous plan is different from the current one we would still need to exit the loop. Having a unique id would mean we can just store a set somewhere and check against if known_plans.contains(new_plan.id) and it would break the loop.

That's a great idea. Thanks @isidentical. I will write up an issue.

I filed #3892 to track this

Dandandan · 2022-10-18T17:10:08Z

It probably makes sense to have some information now how long the optimization takes & maybe provide a conservative number for the maximum optimization passes as @alamb hinted in an earlier PR

alamb

Makes sense to me

andygrove · 2022-10-19T13:43:39Z

It probably makes sense to have some information now how long the optimization takes & maybe provide a conservative number for the maximum optimization passes as @alamb hinted in an earlier PR

I reduced the default max_passes to 3 and added debug logging for optimizer elapsed time

andygrove added 3 commits October 18, 2022 08:42

Multiple optimizer passes

c8d9b29

Run optimizer multiple times

f8e4772

fix

762b559

andygrove commented Oct 18, 2022

View reviewed changes

update test, add comment

801def1

andygrove mentioned this pull request Oct 18, 2022

projection_push_down adds duplicate projections with multiple passes #3881

Closed

update docs and test

8417e7e

andygrove commented Oct 18, 2022

View reviewed changes

benchmarks/src/bin/tpch.rs Outdated Show resolved Hide resolved

fix regression

ee4cd70

andygrove commented Oct 18, 2022

View reviewed changes

andygrove marked this pull request as ready for review October 18, 2022 16:15

andygrove requested review from Dandandan, alamb and liukun4515 October 18, 2022 16:15

isidentical reviewed Oct 18, 2022

View reviewed changes

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

alamb approved these changes Oct 19, 2022

View reviewed changes

andygrove added 2 commits October 19, 2022 07:25

upmerge and reduce default max_passes to 3

800afc3

add debug logging for optimizer duration

beb70d2

andygrove added 2 commits October 19, 2022 07:44

fix merge conflict

c1f5c7b

fix merge

b41c277

andygrove merged commit b004ec7 into apache:master Oct 19, 2022

andygrove deleted the optimizer-passes branch October 19, 2022 14:55

Ted-Jiang mentioned this pull request Oct 20, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple optimizer passes #3880

Multiple optimizer passes #3880

andygrove commented Oct 18, 2022 •

edited

Loading

andygrove Oct 18, 2022

andygrove Oct 18, 2022

andygrove Oct 18, 2022

andygrove Oct 18, 2022

isidentical Oct 18, 2022

andygrove Oct 19, 2022

andygrove Oct 19, 2022

Dandandan commented Oct 18, 2022

alamb left a comment

andygrove commented Oct 19, 2022

Multiple optimizer passes #3880

Multiple optimizer passes #3880

Conversation

andygrove commented Oct 18, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

andygrove Oct 18, 2022

Choose a reason for hiding this comment

andygrove Oct 18, 2022

Choose a reason for hiding this comment

andygrove Oct 18, 2022

Choose a reason for hiding this comment

andygrove Oct 18, 2022

Choose a reason for hiding this comment

isidentical Oct 18, 2022

Choose a reason for hiding this comment

andygrove Oct 19, 2022

Choose a reason for hiding this comment

andygrove Oct 19, 2022

Choose a reason for hiding this comment

Dandandan commented Oct 18, 2022

alamb left a comment

Choose a reason for hiding this comment

andygrove commented Oct 19, 2022

andygrove commented Oct 18, 2022 •

edited

Loading