Re: [PR] fix: drop a deep `BinaryExpr` chain iteratively to avoid a stack overflow [datafusion]

["mdashti (via GitHub)" <gi...@apache.org>]

mdashti commented on code in PR #23198:
URL: https://github.com/apache/datafusion/pull/23198#discussion_r3707379997


##########
datafusion/expr/src/expr.rs:
##########
@@ -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.

Review Comment:
   Thanks @alamb. Agreed the real fix is the representation change; `ops: Vec<Expr>` reshapes every consumer, so I'd rather it land through #23264. On the drop-without-API-change idea: once `Expr` or `BinaryExpr` implements `Drop`, moving fields out of them is rejected (E0509), and by-value destructures like `BinaryExpr { left, op, right }` are everywhere in rewrites. The newtype on the two recursive edges was the smallest shape I found that keeps those moves compiling; documented on `BoxedExpr` in 024f059aa. The same commit removes the hot-path cost @xudong963 raised: a leaf-only binary now drops with no writes and no `Vec`, and `into_inner` hands the same box through rewrites instead of allocating a placeholder. I will close this PR for now, as it seems the consensus is on the `Vec` representation.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org