refactor and fix bugs

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

@@ -1682,11 +1682,10 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for Simplifier<'a, S> {
 fn has_common_conjunction(lhs: &Expr, rhs: &Expr) -> Result<bool, DataFusionError> {
     let lhs: HashSet<&Expr> = iter_conjunction(lhs).collect();
     iter_conjunction(rhs).try_fold(false, |acc, e| {
-        if lhs.contains(&e) {
-            let count = count_volatile_calls(e)?;
-            Ok::<bool, DataFusionError>(acc || count == 0)
+        if lhs.contains(&e) && e.is_volatile()? {
+            Ok(false)
         } else {
-            Ok(acc)
+            Ok(acc || lhs.contains(&e))
         }
     })
 }

datafusion/optimizer/src/simplify_expressions/utils.rs

@@ -83,10 +83,7 @@ fn expr_contains_inner(expr: &Expr, needle: &Expr, search_op: Operator) -> bool
 
 /// check volatile calls and return if expr contains needle
 pub fn expr_contains(expr: &Expr, needle: &Expr, search_op: Operator) -> Result<bool> {
-    Ok(
-        expr_contains_inner(expr, needle, search_op)
-            && count_volatile_calls(needle)? == 0,
-    )
+    Ok(expr_contains_inner(expr, needle, search_op) && !needle.is_volatile()?)
 }
 
 /// Deletes all 'needles' or remains one 'needle' that are found in a chain of xor
@@ -219,7 +216,7 @@ pub fn is_false(expr: &Expr) -> bool {
 /// returns true if `haystack` looks like (needle OP X) or (X OP needle)
 pub fn is_op_with(target_op: Operator, haystack: &Expr, needle: &Expr) -> Result<bool> {
     Ok(
-        matches!(haystack, Expr::BinaryExpr(BinaryExpr { left, op, right }) if op == &target_op && (needle == left.as_ref() || needle == right.as_ref()) && count_volatile_calls(needle)? == 0),
+        matches!(haystack, Expr::BinaryExpr(BinaryExpr { left, op, right }) if op == &target_op && (needle == left.as_ref() || needle == right.as_ref()) && !needle.is_volatile()?),
     )
 }
 
@@ -355,49 +352,3 @@ pub fn distribute_negation(expr: Expr) -> Expr {
         _ => Expr::Negative(Box::new(expr)),
     }
 }
-
-struct VolatileFunctionCounter {
-    counter: usize,
-}
-
-impl VolatileFunctionCounter {
-    pub fn get_count(&self) -> usize {
-        self.counter
-    }
-
-    pub fn new() -> Self {
-        Self { counter: 0 }
-    }
-}
-
-impl<'n> TreeNodeVisitor<'n> for VolatileFunctionCounter {
-    type Node = Expr;
-    fn f_up(
-        &mut self,
-        expr: &'n Self::Node,
-    ) -> Result<datafusion_common::tree_node::TreeNodeRecursion> {
-        match expr {
-            Expr::ScalarFunction(func)
-                if matches!(func.func.signature().volatility, Volatility::Volatile) =>
-            {
-                self.counter += 1;
-            }
-            _ => {}
-        }
-        Ok(datafusion_common::tree_node::TreeNodeRecursion::Continue)
-    }
-
-    fn f_down(
-        &mut self,
-        _node: &'n Self::Node,
-    ) -> Result<datafusion_common::tree_node::TreeNodeRecursion> {
-        Ok(datafusion_common::tree_node::TreeNodeRecursion::Continue)
-    }
-}
-
-// get the number of volatile call in a expression
-pub fn count_volatile_calls(expr: &Expr) -> Result<usize> {
-    let mut volatile_visitor = VolatileFunctionCounter::new();
-    expr.visit(&mut volatile_visitor)?;
-    Ok(volatile_visitor.get_count())
-}

datafusion/sqllogictest/test_files/explain.slt

@@ -436,3 +436,18 @@ physical_plan
 01)CoalesceBatchesExec: target_batch_size=8192
 02)--FilterExec: column1@0 = 2
 03)----ValuesExec
+
+
+query TT
+explain select * from VALUES (1), (2) where column1 = 2 AND random() = 0 OR column1 = 2 AND random() = 0;
+----
+logical_plan
+01)Projection: column1
+02)--Filter: __common_expr_4 AND random() = Float64(0) OR __common_expr_4 AND random() = Float64(0)
+03)----Projection: column1 = Int64(2) AS __common_expr_4, column1
+04)------Values: (Int64(1)), (Int64(2))
+physical_plan
+01)CoalesceBatchesExec: target_batch_size=8192
+02)--FilterExec: __common_expr_4@0 AND random() = 0 OR __common_expr_4@0 AND random() = 0, projection=[column1@1]
+03)----ProjectionExec: expr=[column1@0 = 2 as __common_expr_4, column1@0 as column1]
+04)------ValuesExec