fix(9678): populate visited stack for short-circuited expressions, du…

…ring the common-expr elimination optimization

datafusion/optimizer/src/common_subexpr_eliminate.rs

@@ -582,46 +582,46 @@ impl ExprIdentifierVisitor<'_> {
 
     /// Find the first `EnterMark` in the stack, and accumulates every `ExprItem`
     /// before it.
-    fn pop_enter_mark(&mut self) -> Option<(usize, Identifier)> {
+    fn pop_enter_mark(&mut self) -> (usize, Identifier) {
         let mut desc = String::new();
 
         while let Some(item) = self.visit_stack.pop() {
             match item {
                 VisitRecord::EnterMark(idx) => {
-                    return Some((idx, desc));
+                    return (idx, desc);
                 }
                 VisitRecord::ExprItem(s) => {
                     desc.push_str(&s);
                 }
             }
         }
-        None
+        unreachable!("Enter mark should paired with node number");
     }
 }
 
 impl TreeNodeVisitor for ExprIdentifierVisitor<'_> {
     type Node = Expr;
 
     fn f_down(&mut self, expr: &Expr) -> Result<TreeNodeRecursion> {
-        // related to https://github.com/apache/arrow-datafusion/issues/8814
-        // If the expr contain volatile expression or is a short-circuit expression, skip it.
-        if expr.short_circuits() || is_volatile_expression(expr)? {
-            return Ok(TreeNodeRecursion::Jump);
-        }
         self.visit_stack
             .push(VisitRecord::EnterMark(self.node_count));
         self.node_count += 1;
         // put placeholder
         self.id_array.push((0, "".to_string()));
+
+        // related to https://github.com/apache/arrow-datafusion/issues/8814
+        // If the expr contain volatile expression or is a short-circuit expression, skip it.
+        if expr.short_circuits() || is_volatile_expression(expr)? {
+            return Ok(TreeNodeRecursion::Jump);
+        }
         Ok(TreeNodeRecursion::Continue)
     }
 
     fn f_up(&mut self, expr: &Expr) -> Result<TreeNodeRecursion> {
         self.series_number += 1;
 
-        let Some((idx, sub_expr_desc)) = self.pop_enter_mark() else {
-            return Ok(TreeNodeRecursion::Continue);
-        };
+        let (idx, sub_expr_desc) = self.pop_enter_mark();
+
         // skip exprs should not be recognize.
         if self.expr_mask.ignores(expr) {
             self.id_array[idx].0 = self.series_number;

datafusion/sqllogictest/test_files/common_subexpr_eliminate.slt

@@ -38,16 +38,14 @@ select f64, coalesce(1.0 / f64, 0.0), acos(coalesce(1.0 / f64, 0.0)) from double
 ----
 10.1 0.09900990099 1.471623942989
 
-# BUG: should be 1.471623942989
 # common subexpr with coalesce (short-circuited) and alias
 query RRR rowsort
 select f64, coalesce(1.0 / f64, 0.0) as f64_1, acos(coalesce(1.0 / f64, 0.0)) from doubles;
 ----
-10.1 0.09900990099 0.09900990099
+10.1 0.09900990099 1.471623942989
 
-# BUG: should be 1.471623942989
 # common subexpr with case (short-circuited)
 query RRR rowsort
 select f64, case when f64 > 0 then 1.0 / f64 else null end, acos(case when f64 > 0 then 1.0 / f64 else null end) from doubles;
 ----
-10.1 0.09900990099 0.09900990099
+10.1 0.09900990099 1.471623942989