Merge pull request #31146 from ggevay/hir-visitation

Remove some deprecated HIR visitation functions
MaterializeInc · Jan 22, 2025 · 52ffcad · 52ffcad
2 parents ff2d597 + 7e1e1c5
commit 52ffcad
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 231 deletions.
diff --git a/src/sql/src/plan/hir.rs b/src/sql/src/plan/hir.rs
@@ -3097,121 +3097,6 @@ impl HirScalarExpr {
         mem::replace(self, HirScalarExpr::literal_null(ScalarType::String))
     }
 
-    pub fn visit<'a, F>(&'a self, f: &mut F)
-    where
-        F: FnMut(&'a Self),
-    {
-        self.visit1(|e: &HirScalarExpr| e.visit(f));
-        f(self);
-    }
-
-    pub fn visit1<'a, F>(&'a self, mut f: F)
-    where
-        F: FnMut(&'a Self),
-    {
-        use HirScalarExpr::*;
-        match self {
-            Column(..) | Parameter(..) | Literal(..) | CallUnmaterializable(..) => (),
-            CallUnary { expr, .. } => f(expr),
-            CallBinary { expr1, expr2, .. } => {
-                f(expr1);
-                f(expr2);
-            }
-            CallVariadic { exprs, .. } => {
-                for expr in exprs {
-                    f(expr);
-                }
-            }
-            If { cond, then, els } => {
-                f(cond);
-                f(then);
-                f(els);
-            }
-            Exists(..) | Select(..) => (),
-            Windowing(expr) => {
-                let _ = expr.visit_expressions(&mut |e| -> Result<(), ()> {
-                    f(e);
-                    Ok(())
-                });
-            }
-        }
-    }
-
-    #[deprecated = "Use `Visit::visit_post` instead."]
-    pub fn visit_mut<F>(&mut self, f: &mut F)
-    where
-        F: FnMut(&mut Self),
-    {
-        #[allow(deprecated)]
-        self.visit1_mut(|e: &mut HirScalarExpr| e.visit_mut(f));
-        f(self);
-    }
-
-    #[deprecated = "Use `Visit::visit_mut_pre` instead."]
-    pub fn visit_mut_pre<F>(&mut self, f: &mut F)
-    where
-        F: FnMut(&mut Self),
-    {
-        f(self);
-        #[allow(deprecated)]
-        self.visit1_mut(|e: &mut HirScalarExpr| e.visit_mut(f));
-    }
-
-    #[deprecated = "Use `VisitChildren<HirScalarExpr>::visit_children` instead."]
-    pub fn visit1_mut<F>(&mut self, mut f: F)
-    where
-        F: FnMut(&mut Self),
-    {
-        use HirScalarExpr::*;
-        match self {
-            Column(..) | Parameter(..) | Literal(..) | CallUnmaterializable(..) => (),
-            CallUnary { expr, .. } => f(expr),
-            CallBinary { expr1, expr2, .. } => {
-                f(expr1);
-                f(expr2);
-            }
-            CallVariadic { exprs, .. } => {
-                for expr in exprs {
-                    f(expr);
-                }
-            }
-            If { cond, then, els } => {
-                f(cond);
-                f(then);
-                f(els);
-            }
-            Exists(..) | Select(..) => (),
-            Windowing(expr) => {
-                let _ = expr.visit_expressions_mut(&mut |e| -> Result<(), ()> {
-                    f(e);
-                    Ok(())
-                });
-            }
-        }
-    }
-
-    #[deprecated = "Use `Visit::visit_pre_post` instead."]
-    /// A generalization of `visit`. The function `pre` runs on a
-    /// `HirScalarExpr` before it runs on any of the child `HirScalarExpr`s.
-    /// The function `post` runs on child `HirScalarExpr`s first before the
-    /// parent. Optionally, `pre` can return which child `HirScalarExpr`s, if
-    /// any, should be visited (default is to visit all children).
-    pub fn visit_pre_post<F1, F2>(&self, pre: &mut F1, post: &mut F2)
-    where
-        F1: FnMut(&Self) -> Option<Vec<&Self>>,
-        F2: FnMut(&Self),
-    {
-        let to_visit = pre(self);
-        if let Some(to_visit) = to_visit {
-            for e in to_visit {
-                e.visit_pre_post(pre, post);
-            }
-        } else {
-            self.visit1(|e| e.visit_pre_post(pre, post));
-        }
-        post(self);
-    }
-
     #[deprecated = "Redefine this based on the `Visit` and `VisitChildren` methods."]
     /// Visits the column references in this scalar expression.
     ///

diff --git a/src/sql/src/plan/lowering.rs b/src/sql/src/plan/lowering.rs
@@ -39,17 +39,19 @@
 use std::collections::{BTreeMap, BTreeSet};
 use std::iter::repeat;
 
+use itertools::Itertools;
+use mz_expr::visit::Visit;
+use mz_expr::{AccessStrategy, AggregateFunc, MirRelationExpr, MirScalarExpr};
+use mz_ore::collections::CollectionExt;
+use mz_ore::stack::maybe_grow;
+use mz_repr::*;
+
 use crate::optimizer_metrics::OptimizerMetrics;
 use crate::plan::hir::{
     AggregateExpr, ColumnOrder, ColumnRef, HirRelationExpr, HirScalarExpr, JoinKind, WindowExprType,
 };
 use crate::plan::{transform_hir, PlanError};
 use crate::session::vars::SystemVars;
-use itertools::Itertools;
-use mz_expr::{AccessStrategy, AggregateFunc, MirRelationExpr, MirScalarExpr};
-use mz_ore::collections::CollectionExt;
-use mz_ore::stack::maybe_grow;
-use mz_repr::*;
 
 mod variadic_left;
 
@@ -184,8 +186,8 @@ impl HirRelationExpr {
             }
             mut other => {
                 let mut id_gen = mz_ore::id_gen::IdGen::default();
-                transform_hir::split_subquery_predicates(&mut other);
-                transform_hir::try_simplify_quantified_comparisons(&mut other);
+                transform_hir::split_subquery_predicates(&mut other)?;
+                transform_hir::try_simplify_quantified_comparisons(&mut other)?;
                 transform_hir::fuse_window_functions(&mut other, &context)?;
                 MirRelationExpr::constant(vec![vec![]], RelationType::new(vec![])).let_in(
                     &mut id_gen,
@@ -1525,7 +1527,6 @@ impl HirScalarExpr {
             let mut subqueries = Vec::new();
             let distinct_inner = get_inner.clone().distinct();
             for expr in exprs.iter() {
-                #[allow(deprecated)]
                 expr.visit_pre_post(
                     &mut |e| match e {
                         // For simplicity, subqueries within a conditional statement will be
@@ -1565,7 +1566,7 @@ impl HirScalarExpr {
                         }
                         _ => {}
                     },
-                );
+                )?;
             }
 
             if subqueries.is_empty() {

diff --git a/src/sql/src/plan/query.rs b/src/sql/src/plan/query.rs
@@ -855,15 +855,16 @@ fn handle_mutation_using_clause(
         // those to the right of `using_rel_expr`) to instead be correlated to
         // the outer relation, i.e. `get`.
         let using_rel_arity = qcx.relation_type(&using_rel_expr).arity();
-        #[allow(deprecated)]
-        expr.visit_mut(&mut |e| {
+        // local import to not get confused with `mz_sql_parser::ast::visit::Visit`
+        use mz_expr::visit::Visit;
+        expr.visit_mut_post(&mut |e| {
             if let HirScalarExpr::Column(c) = e {
                 if c.column >= using_rel_arity {
                     c.level += 1;
                     c.column -= using_rel_arity;
                 };
             }
-        });
+        })?;
 
         // Filter `USING` tables like `<using_rel_expr> WHERE <expr>`. Note that
         // this filters the `USING` tables, _not_ the joined `USING..., FROM`