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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
