Merge pull request #30615 from ggevay/threshold-elision-not-physical

Remove `ThresholdElision` from the end of the physical pass
MaterializeInc · Nov 25, 2024 · d0f68ce · d0f68ce
2 parents b8735eb + 4bc2919
commit d0f68ce
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 12 deletions.
diff --git a/src/transform/src/lib.rs b/src/transform/src/lib.rs
@@ -579,7 +579,9 @@ impl Optimizer {
             // TODO: lift filters/maps to maximize ability to collapse
             // things down?
             Box::new(fuse_and_collapse()),
-            // 3. Structure-aware cleanup that needs to happen before ColumnKnowledge
+            // 3. Needs to happen before ColumnKnowledge, LiteralLifting, EquivalencePropagation
+            // make (literal) filters look more complicated than what the NonNegative Analysis can
+            // recognize.
             Box::new(threshold_elision::ThresholdElision),
             // 4. Move predicate information up and down the tree.
             //    This also fixes the shape of joins in the plan.
@@ -701,13 +703,6 @@ impl Optimizer {
                 limit: Some(FOLD_CONSTANTS_LIMIT),
             }),
             Box::new(canonicalization::ReduceScalars),
-            // Remove threshold operators which have no effect.
-            // Must be done at the very end of the physical pass, because before
-            // that (at least at the moment) we cannot be sure that all trees
-            // are simplified equally well so they are structurally almost
-            // identical. Check the `threshold_elision.slt` tests that fail if
-            // you remove this transform for examples.
-            Box::new(threshold_elision::ThresholdElision),
             // We need this to ensure that `CollectIndexRequests` gets a normalized plan.
             // (For example, `FoldConstants` can break the normalized form by removing all
             // references to a Let, see https://github.com/MaterializeInc/database-issues/issues/6371)

diff --git a/test/sqllogictest/transform/threshold_elision.slt b/test/sqllogictest/transform/threshold_elision.slt
@@ -167,8 +167,6 @@ EOF
 
 # Complex example: EXCEPT ALL.
 # Here ThresholdElision can only match in after some prior simplifications
-# and for some reason (TBD later) this means that we need to run it at the
-# end of the physical pass.
 query T multiline
 EXPLAIN OPTIMIZED PLAN WITH(non negative) FOR
 (
@@ -210,8 +208,6 @@ EOF
 
 # Complex example: EXCEPT.
 # Here ThresholdElision can only match in after some prior simplifications
-# and for some reason (TBD later) this means that we need to run it at the
-# end of the physical pass.
 query T multiline
 EXPLAIN OPTIMIZED PLAN WITH(non negative) FOR
 (
@@ -544,3 +540,30 @@ Source materialize.public.people
 Target cluster: quickstart
 
 EOF
+
+statement ok
+CREATE TABLE t1 (f1 INTEGER, f2 INTEGER);
+
+# Literal filter, which would be made unrecognizable by
+# ColumnKnowledge, LiteralLifting, EquivalencePropagation.
+query T multiline
+EXPLAIN OPTIMIZED PLAN WITH(non negative) FOR
+SELECT f1 FROM t1 EXCEPT SELECT f1 FROM t1 WHERE f1 = 5;
+----
+Explained Query:
+  Union // { non_negative: false }
+    Distinct project=[#0] // { non_negative: true }
+      Project (#0) // { non_negative: true }
+        ReadStorage materialize.public.t1 // { non_negative: true }
+    Negate // { non_negative: false }
+      Map (5) // { non_negative: true }
+        Distinct project=[] // { non_negative: true }
+          Project () // { non_negative: true }
+            Filter (#0 = 5) // { non_negative: true }
+              ReadStorage materialize.public.t1 // { non_negative: true }
+
+Source materialize.public.t1
+
+Target cluster: quickstart
+
+EOF