optimizer: avoid deconstructing temporal filters in ANF

Author: ggevay

src/expr/src/linear.rs

@@ -1490,6 +1490,7 @@ pub fn memoize_expr(
             if let MirScalarExpr::If { cond, .. } = e {
                 return Some(vec![cond]);
             }
+
             // We should not eagerly memoize `COALESCE` expressions after the first,
             // as they are only meant to be evaluated if the preceding expressions
             // evaluate to NULL. We could memoize any preceding by expressions that
@@ -1501,6 +1502,15 @@ pub fn memoize_expr(
             {
                 return Some(exprs.iter_mut().take(1).collect());
             }
+
+            // We should not deconstruct temporal filters, because `MfpPlan::create_from` expects
+            // those to be in a specific form. However, we _should_ attend to the expression that is
+            // on the opposite side of mz_now(), because it might be a complex expression in itself,
+            // and is ok to deconstruct.
+            if let Some(other_side) = e.is_temporal_filter() {
+                return Some(vec![other_side]);
+            }
+
             None
         },
         &mut |e| {
@@ -1520,6 +1530,8 @@ pub fn memoize_expr(
                     }
                 }
                 _ => {
+                    // TODO: OOO (Optimizer Optimization Opportunity):
+                    // we are quadratic in expression size because of this .iter().position
                     if let Some(position) = memoized_parts.iter().position(|e2| e2 == e) {
                         // Any complex expression that already exists as a prior column can
                         // be replaced by a reference to that column.
@@ -1634,6 +1646,7 @@ pub mod util {
 pub mod plan {
     use std::iter;
 
+    use mz_ore::soft_assert_or_log;
     use mz_proto::{IntoRustIfSome, ProtoType, RustType, TryFromProtoError};
     use mz_repr::{Datum, Diff, Row, RowArena};
     use proptest::prelude::*;
@@ -1842,7 +1855,8 @@ pub mod plan {
                 }
             });
 
-            for predicate in temporal.into_iter() {
+            for mut predicate in temporal.into_iter() {
+                let is_temporal_filter = predicate.is_temporal_filter().is_some(); // for the asserts
                 // Supported temporal predicates are exclusively binary operators.
                 if let MirScalarExpr::CallBinary {
                     mut func,
@@ -1863,6 +1877,10 @@ pub mod plan {
                             BinaryFunc::Gt => BinaryFunc::Lt,
                             BinaryFunc::Gte => BinaryFunc::Lte,
                             x => {
+                                soft_assert_or_log!(
+                                    !is_temporal_filter,
+                                    "MfpPlan::create_from and is_temporal_filter should agree"
+                                );
                                 return Err(format!(
                                     "Unsupported binary temporal operation: {:?}",
                                     x
@@ -1876,6 +1894,10 @@ pub mod plan {
                         || *expr1
                             != MirScalarExpr::CallUnmaterializable(UnmaterializableFunc::MzNow)
                     {
+                        soft_assert_or_log!(
+                            !is_temporal_filter,
+                            "MfpPlan::create_from and is_temporal_filter should agree"
+                        );
                         return Err(format!(
                             "Unsupported temporal predicate. Note: `mz_now()` must be directly compared to a mz_timestamp-castable expression. Expression found: {}",
                             MirScalarExpr::CallBinary { func, expr1, expr2 },
@@ -1913,7 +1935,15 @@ pub mod plan {
                             ));
                         }
                     }
+                    soft_assert_or_log!(
+                        is_temporal_filter, // successfully found a temporal filter
+                        "MfpPlan::create_from and is_temporal_filter should agree"
+                    );
                 } else {
+                    soft_assert_or_log!(
+                        !is_temporal_filter,
+                        "MfpPlan::create_from and is_temporal_filter should agree"
+                    );
                     return Err(format!(
                         "Unsupported temporal predicate. Note: `mz_now()` must be directly compared to a non-temporal expression of mz_timestamp-castable type. Expression found: {}",
                         predicate,

src/expr/src/scalar.rs

@@ -728,6 +728,27 @@ impl MirScalarExpr {
         )
     }
 
+    /// Checks whether the expression is a standard temporal filter, that is:
+    /// - mz_now() by itself on one side of an Eq | Gt | Gte | Lt | Lte comparison, and
+    /// - no mz_now() anywhere inside the other side of the comparison.
+    ///
+    /// If yes, then it returns the other side of the comparison.
+    pub fn is_temporal_filter(&mut self) -> Option<&mut MirScalarExpr> {
+        use BinaryFunc::*;
+        if let MirScalarExpr::CallBinary { func, expr1, expr2 } = self {
+            if matches!(func, Eq | Gt | Gte | Lt | Lte) {
+                let mz_now = MirScalarExpr::CallUnmaterializable(UnmaterializableFunc::MzNow);
+                if **expr1 == mz_now && !expr2.contains_temporal() {
+                    return Some(expr2);
+                }
+                if **expr2 == mz_now && !expr1.contains_temporal() {
+                    return Some(expr1);
+                }
+            }
+        }
+        None
+    }
+
     #[deprecated = "Use `might_error` instead"]
     pub fn contains_error_if_null(&self) -> bool {
         let mut worklist = vec![self];

test/sqllogictest/temporal.slt

@@ -245,6 +245,9 @@ WHERE insert_ts >= mz_now() - 30;
 statement ok
 CREATE TABLE t2(ts timestamp, x int);
 
+statement ok
+INSERT INTO t2 VALUES ('2024-01-03 00:00:00', 5), ('2024-01-04 00:00:00', 6);
+
 statement ok
 SELECT *
 FROM t2
@@ -256,6 +259,9 @@ SELECT *
 FROM t2
 WHERE ts + INTERVAL '30' minutes >= mz_now();
 
+statement ok
+SELECT * FROM mv1;
+
 query error db error: ERROR: WHERE clause error: operator does not exist: mz_timestamp \- interval
 SELECT *
 FROM t2
@@ -313,15 +319,21 @@ WHERE
   ts + INTERVAL '30' minutes >= mz_now();
 
 statement ok
-SELECT * FROM v_one_off;
+SELECT * FROM v_maintained;
 
 statement ok
 CREATE DEFAULT INDEX ON v_maintained;
 
+statement ok
+SELECT * FROM v_maintained;
+
 statement ok
 CREATE MATERIALIZED VIEW mv2 AS
 SELECT * FROM v_maintained;
 
+statement ok
+SELECT * FROM mv2;
+
 simple
 DECLARE c CURSOR FOR SUBSCRIBE v_maintained;
 FETCH 0 c;
@@ -338,6 +350,9 @@ WHERE
   ts + INTERVAL '30' minutes >= mz_now()
   AND x != 7;
 
+statement ok
+SELECT * FROM mv3;
+
 # UNION ALL workaround for OR
 
 query error db error: ERROR: Unsupported temporal predicate\. Note: `mz_now\(\)` must be directly compared to a non\-temporal expression of mz_timestamp\-castable type\. Expression found: \(\(#1\{x\} = 7\) OR \(timestamp_to_mz_timestamp\(\(#0\{ts\} \+ 00:30:00\)\) >= mz_now\(\)\)\)
@@ -363,3 +378,31 @@ UNION ALL
     ts + INTERVAL '30' minutes >= mz_now()
     AND x != 7
 );
+
+statement ok
+SELECT * FROM mv4;
+
+# Regression test for https://github.com/MaterializeInc/database-issues/issues/9643
+# Two different temporal filters on different reads of the same source.
+
+statement ok
+SELECT *
+FROM t2
+WHERE ts + INTERVAL '30' minutes >= mz_now()
+UNION ALL
+SELECT *
+FROM t2
+WHERE ts + INTERVAL '60' minutes >= mz_now();
+
+statement ok
+CREATE MATERIALIZED VIEW mv5 AS
+SELECT *
+FROM t2
+WHERE ts + INTERVAL '30' minutes >= mz_now()
+UNION ALL
+SELECT *
+FROM t2
+WHERE ts + INTERVAL '60' minutes >= mz_now();
+
+statement ok
+SELECT * FROM mv5;