fix: drop a deep BinaryExpr chain iteratively to avoid a stack overflow

datafusion-examples/examples/query_planning/expr_api.rs

@@ -591,9 +591,10 @@ fn type_coercion_demo() -> Result<()> {
             if let Expr::Column(ref col_expr) = *e.left {
                 let field = df_schema.field_with_name(None, col_expr.name())?;
                 let cast_to_type = field.data_type();
-                let coerced_right = e.right.cast_to(cast_to_type, &df_schema)?;
+                let coerced_right =
+                    (*e.right.into_inner()).cast_to(cast_to_type, &df_schema)?;
                 Ok(Transformed::yes(Expr::BinaryExpr(BinaryExpr::new(
-                    e.left,
+                    e.left.into_inner(),
                     e.op,
                     Box::new(coerced_right),
                 ))))

datafusion-examples/examples/query_planning/optimizer_rule.rs

@@ -150,7 +150,7 @@ impl MyOptimizerRule {
                     let BinaryExpr { left, op: _, right } = binary_expr;
                     // rewrite to `my_eq(left, right)`
                     let udf = ScalarUDF::new_from_impl(MyEq::new());
-                    let call = udf.call(vec![*left, *right]);
+                    let call = udf.call(vec![*left.into_inner(), *right.into_inner()]);
                     Ok(Transformed::yes(call))
                 }
                 _ => Ok(Transformed::no(expr)),

datafusion/core/tests/optimizer/mod.rs

@@ -321,19 +321,19 @@ fn test_inequalities_non_null_bounded() {
         (col("x").not_between(lit(0), lit(5)), false),
         (col("x").not_between(lit(5), lit(10)), true),
         (
-            Expr::BinaryExpr(BinaryExpr {
-                left: Box::new(col("x")),
-                op: Operator::IsDistinctFrom,
-                right: Box::new(lit(ScalarValue::Null)),
-            }),
+            Expr::BinaryExpr(BinaryExpr::new(
+                Box::new(col("x")),
+                Operator::IsDistinctFrom,
+                Box::new(lit(ScalarValue::Null)),
+            )),
             true,
         ),
         (
-            Expr::BinaryExpr(BinaryExpr {
-                left: Box::new(col("x")),
-                op: Operator::IsDistinctFrom,
-                right: Box::new(lit(5)),
-            }),
+            Expr::BinaryExpr(BinaryExpr::new(
+                Box::new(col("x")),
+                Operator::IsDistinctFrom,
+                Box::new(lit(5)),
+            )),
             true,
         ),
     ];

datafusion/core/tests/user_defined/expr_planner.rs

@@ -41,18 +41,18 @@ impl ExprPlanner for MyCustomPlanner {
     ) -> Result<PlannerResult<RawBinaryExpr>> {
         match &expr.op {
             BinaryOperator::Arrow => {
-                Ok(PlannerResult::Planned(Expr::BinaryExpr(BinaryExpr {
-                    left: Box::new(expr.left.clone()),
-                    right: Box::new(expr.right.clone()),
-                    op: Operator::StringConcat,
-                })))
+                Ok(PlannerResult::Planned(Expr::BinaryExpr(BinaryExpr::new(
+                    Box::new(expr.left.clone()),
+                    Operator::StringConcat,
+                    Box::new(expr.right.clone()),
+                ))))
             }
             BinaryOperator::LongArrow => {
-                Ok(PlannerResult::Planned(Expr::BinaryExpr(BinaryExpr {
-                    left: Box::new(expr.left.clone()),
-                    right: Box::new(expr.right.clone()),
-                    op: Operator::Plus,
-                })))
+                Ok(PlannerResult::Planned(Expr::BinaryExpr(BinaryExpr::new(
+                    Box::new(expr.left.clone()),
+                    Operator::Plus,
+                    Box::new(expr.right.clone()),
+                ))))
             }
             BinaryOperator::Question => {
                 Ok(PlannerResult::Planned(Expr::Alias(Alias::new(

datafusion/expr/src/expr.rs

@@ -22,6 +22,7 @@ use std::collections::HashSet;
 use std::fmt::{self, Display, Formatter, Write};
 use std::hash::{Hash, Hasher};
 use std::mem;
+use std::ops::{Deref, DerefMut};
 use std::sync::Arc;
 
 use crate::expr_fn::binary_expr;
@@ -767,21 +768,112 @@ impl Alias {
     }
 }
 
+/// A `Box<Expr>` for the recursive children of [`BinaryExpr`]. A long binary-operator chain
+/// (e.g. `a OR b OR c OR ...`) builds an `Expr` as deep as the chain, and dropping it
+/// recursively would overflow the stack. This wrapper's `Drop` tears the chain down iteratively.
+#[derive(Clone, PartialEq, Eq, PartialOrd, Hash)]
+pub struct BoxedExpr(Box<Expr>);
+
+impl fmt::Debug for BoxedExpr {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        // Stay transparent like the `Box<Expr>` this replaced, so a `BinaryExpr`'s debug
+        // output keeps showing its children directly rather than wrapped.
+        fmt::Debug::fmt(&self.0, f)
+    }
+}
+
+impl BoxedExpr {
+    pub fn new(expr: Expr) -> Self {
+        Self(Box::new(expr))
+    }
+
+    /// Take the inner `Box<Expr>` out without running the iterative `Drop`.
+    pub fn into_inner(mut self) -> Box<Expr> {
+        let inner = mem::replace(
+            &mut self.0,
+            Box::new(Expr::Literal(ScalarValue::Null, None)),
+        );
+        // `self` now holds a leaf and drops in O(1).
+        inner
+    }
+}
+
+impl From<Box<Expr>> for BoxedExpr {
+    fn from(boxed: Box<Expr>) -> Self {
+        Self(boxed)
+    }
+}
+
+impl From<Expr> for BoxedExpr {
+    fn from(expr: Expr) -> Self {
+        Self(Box::new(expr))
+    }
+}
+
+impl Deref for BoxedExpr {
+    type Target = Expr;
+    fn deref(&self) -> &Expr {
+        &self.0
+    }
+}
+
+impl DerefMut for BoxedExpr {
+    fn deref_mut(&mut self) -> &mut Expr {
+        &mut self.0
+    }
+}
+
+impl AsRef<Expr> for BoxedExpr {
+    fn as_ref(&self) -> &Expr {
+        &self.0
+    }
+}
+
+impl Drop for BoxedExpr {
+    fn drop(&mut self) {
+        // Detach each `BinaryExpr`'s children into a heap stack before the node drops, so an
+        // arbitrarily deep chain is torn down iteratively instead of recursing.
+        fn detach(expr: &mut Expr, stack: &mut Vec<Expr>) {
+            if let Expr::BinaryExpr(b) = expr {
+                stack.push(mem::replace(
+                    b.left.0.as_mut(),
+                    Expr::Literal(ScalarValue::Null, None),
+                ));
+                stack.push(mem::replace(
+                    b.right.0.as_mut(),
+                    Expr::Literal(ScalarValue::Null, None),
+                ));
+            }
+        }
+
+        let mut stack: Vec<Expr> = Vec::new();
+        detach(self.0.as_mut(), &mut stack);
+        while let Some(mut expr) = stack.pop() {
+            detach(&mut expr, &mut stack);
+            // `expr` drops here; its children were detached, so the drop is shallow.
+        }
+    }
+}
+
 /// Binary expression for [`Expr::BinaryExpr`]
 #[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
 pub struct BinaryExpr {
     /// Left-hand side of the expression
-    pub left: Box<Expr>,
+    pub left: BoxedExpr,
     /// The comparison operator
     pub op: Operator,
     /// Right-hand side of the expression
-    pub right: Box<Expr>,
+    pub right: BoxedExpr,
 }
 
 impl BinaryExpr {
     /// Create a new binary expression
     pub fn new(left: Box<Expr>, op: Operator, right: Box<Expr>) -> Self {
-        Self { left, op, right }
+        Self {
+            left: BoxedExpr(left),
+            op,
+            right: BoxedExpr(right),
+        }
     }
 }
 
@@ -2165,8 +2257,8 @@ impl Expr {
             match &mut expr {
                 // Default to assuming the arguments are the same type
                 Expr::BinaryExpr(BinaryExpr { left, op: _, right }) => {
-                    rewrite_placeholder(left.as_mut(), right.as_ref(), schema)?;
-                    rewrite_placeholder(right.as_mut(), left.as_ref(), schema)?;
+                    rewrite_placeholder(left, right.as_ref(), schema)?;
+                    rewrite_placeholder(right, left.as_ref(), schema)?;
                 }
                 Expr::Between(Between {
                     expr,
@@ -3789,6 +3881,22 @@ mod test {
     use sqlparser::ast;
     use sqlparser::ast::{Ident, IdentWithAlias};
 
+    #[test]
+    fn deep_binary_expr_chain_drops_without_overflowing() {
+        // Build a left-deep `OR` chain far deeper than a recursive drop could unwind, then let it
+        // drop on the normal test stack. The iterative `Drop` on `BoxedExpr` must keep it from
+        // overflowing.
+        let mut expr = Expr::Literal(ScalarValue::Null, None);
+        for _ in 0..200_000 {
+            expr = Expr::BinaryExpr(BinaryExpr::new(
+                Box::new(expr),
+                Operator::Or,
+                Box::new(Expr::Literal(ScalarValue::Null, None)),
+            ));
+        }
+        drop(expr);
+    }
+
     #[test]
     fn infer_placeholder_in_clause() {
         // SELECT * FROM employees WHERE department_id IN ($1, $2, $3);
@@ -4011,7 +4119,7 @@ mod test {
 
         let (inferred_expr, _) = expr.infer_placeholder_types(&df_schema).unwrap();
         match inferred_expr {
-            Expr::BinaryExpr(BinaryExpr { right, .. }) => match *right {
+            Expr::BinaryExpr(BinaryExpr { right, .. }) => match *right.into_inner() {
                 Expr::Placeholder(placeholder) => {
                     assert_eq!(
                         placeholder.field.as_ref().unwrap().data_type(),

datafusion/expr/src/expr_rewriter/guarantees.rs

@@ -489,19 +489,19 @@ mod tests {
                 true,
             ),
             (
-                Expr::BinaryExpr(BinaryExpr {
-                    left: Box::new(col("x")),
-                    op: Operator::IsDistinctFrom,
-                    right: Box::new(lit(ScalarValue::Null)),
-                }),
+                Expr::BinaryExpr(BinaryExpr::new(
+                    Box::new(col("x")),
+                    Operator::IsDistinctFrom,
+                    Box::new(lit(ScalarValue::Null)),
+                )),
                 true,
             ),
             (
-                Expr::BinaryExpr(BinaryExpr {
-                    left: Box::new(col("x")),
-                    op: Operator::IsDistinctFrom,
-                    right: Box::new(lit(ScalarValue::Date32(Some(17000)))),
-                }),
+                Expr::BinaryExpr(BinaryExpr::new(
+                    Box::new(col("x")),
+                    Operator::IsDistinctFrom,
+                    Box::new(lit(ScalarValue::Date32(Some(17000)))),
+                )),
                 true,
             ),
         ];
@@ -547,19 +547,19 @@ mod tests {
         // (original_expr, expected_simplification)
         let simplified_cases = &[
             (
-                Expr::BinaryExpr(BinaryExpr {
-                    left: Box::new(col("x")),
-                    op: Operator::IsDistinctFrom,
-                    right: Box::new(lit("z")),
-                }),
+                Expr::BinaryExpr(BinaryExpr::new(
+                    Box::new(col("x")),
+                    Operator::IsDistinctFrom,
+                    Box::new(lit("z")),
+                )),
                 true,
             ),
             (
-                Expr::BinaryExpr(BinaryExpr {
-                    left: Box::new(col("x")),
-                    op: Operator::IsNotDistinctFrom,
-                    right: Box::new(lit("z")),
-                }),
+                Expr::BinaryExpr(BinaryExpr::new(
+                    Box::new(col("x")),
+                    Operator::IsNotDistinctFrom,
+                    Box::new(lit("z")),
+                )),
                 false,
             ),
         ];
@@ -575,11 +575,11 @@ mod tests {
             col("x").not_eq(lit("a")),
             col("x").between(lit("a"), lit("z")),
             col("x").not_between(lit("a"), lit("z")),
-            Expr::BinaryExpr(BinaryExpr {
-                left: Box::new(col("x")),
-                op: Operator::IsDistinctFrom,
-                right: Box::new(lit(ScalarValue::Null)),
-            }),
+            Expr::BinaryExpr(BinaryExpr::new(
+                Box::new(col("x")),
+                Operator::IsDistinctFrom,
+                Box::new(lit(ScalarValue::Null)),
+            )),
         ];
 
         validate_unchanged_cases(&guarantees, unchanged_cases);

datafusion/expr/src/tree_node.rs

@@ -85,7 +85,7 @@ impl TreeNode for Expr {
             | Expr::Placeholder(_)
             | Expr::LambdaVariable(_) => Ok(TreeNodeRecursion::Continue),
             Expr::BinaryExpr(BinaryExpr { left, right, .. }) => {
-                (left, right).apply_ref_elements(f)
+                (left.as_ref(), right.as_ref()).apply_ref_elements(f)
             }
             Expr::Like(Like { expr, pattern, .. })
             | Expr::SimilarTo(Like { expr, pattern, .. }) => {
@@ -173,11 +173,13 @@ impl TreeNode for Expr {
             }) => expr.map_elements(f)?.update_data(|be| {
                 Expr::InSubquery(InSubquery::new(be, subquery, negated))
             }),
-            Expr::BinaryExpr(BinaryExpr { left, op, right }) => (left, right)
-                .map_elements(f)?
-                .update_data(|(new_left, new_right)| {
-                    Expr::BinaryExpr(BinaryExpr::new(new_left, op, new_right))
-                }),
+            Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
+                (left.into_inner(), right.into_inner())
+                    .map_elements(f)?
+                    .update_data(|(new_left, new_right)| {
+                        Expr::BinaryExpr(BinaryExpr::new(new_left, op, new_right))
+                    })
+            }
             Expr::Like(Like {
                 negated,
                 expr,

datafusion/expr/src/utils.rs

@@ -1075,8 +1075,8 @@ pub fn iter_conjunction_owned(expr: Expr) -> impl Iterator<Item = Expr> {
                     op: Operator::And,
                     left,
                 }) => {
-                    stack.push(*right);
-                    stack.push(*left);
+                    stack.push(*right.into_inner());
+                    stack.push(*left.into_inner());
                 }
                 Expr::Alias(Alias { expr, .. }) => stack.push(*expr),
                 other => return Some(other),
@@ -1138,8 +1138,8 @@ fn split_binary_owned_impl(
 ) -> Vec<Expr> {
     match expr {
         Expr::BinaryExpr(BinaryExpr { right, op, left }) if op == operator => {
-            let exprs = split_binary_owned_impl(*left, operator, exprs);
-            split_binary_owned_impl(*right, operator, exprs)
+            let exprs = split_binary_owned_impl(*left.into_inner(), operator, exprs);
+            split_binary_owned_impl(*right.into_inner(), operator, exprs)
         }
         Expr::Alias(Alias { expr, .. }) => {
             split_binary_owned_impl(*expr, operator, exprs)

datafusion/optimizer/src/analyzer/type_coercion.rs

@@ -614,8 +614,13 @@ impl TreeNodeRewriter for TypeCoercionRewriter<'_> {
                 ))))
             }
             Expr::BinaryExpr(BinaryExpr { left, op, right }) => {
-                let (left, right) =
-                    self.coerce_binary_op(*left, self.schema, op, *right, self.schema)?;
+                let (left, right) = self.coerce_binary_op(
+                    *left.into_inner(),
+                    self.schema,
+                    op,
+                    *right.into_inner(),
+                    self.schema,
+                )?;
                 Ok(Transformed::yes(Expr::BinaryExpr(BinaryExpr::new(
                     Box::new(left),
                     op,