wiedld commented on code in PR #9685:
URL: https://github.com/apache/arrow-datafusion/pull/9685#discussion_r1531573732
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -571,66 +580,73 @@ enum VisitRecord {
/// `usize` is the monotone increasing series number assigned in
pre_visit().
/// Starts from 0. Is used to index the identifier array `id_array` in
post_visit().
EnterMark(usize),
+ /// the node's children were skipped => jump to f_up on same node
+ JumpMark(usize),
/// Accumulated identifier of sub expression.
ExprItem(Identifier),
}
impl ExprIdentifierVisitor<'_> {
- fn desc_expr(expr: &Expr) -> String {
+ fn node_identifier(expr: &Expr) -> String {
format!("{expr}")
}
/// 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));
+ VisitRecord::EnterMark(idx) | VisitRecord::JumpMark(idx) => {
+ return (idx, desc);
}
- VisitRecord::ExprItem(s) => {
- desc.push_str(&s);
+ VisitRecord::ExprItem(id) => {
+ desc.push_str(&id);
}
}
}
- 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> {
+ // put placeholder, sets the proper array length
+ 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);
+ self.visit_stack
+ .push(VisitRecord::JumpMark(self.node_count));
+ return Ok(TreeNodeRecursion::Jump); // go to f_up
}
+
self.visit_stack
.push(VisitRecord::EnterMark(self.node_count));
self.node_count += 1;
- // put placeholder
- self.id_array.push((0, "".to_string()));
+
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);
- };
Review Comment:
This was one of the bugs. The behavior was changed with the TreeNode
refactor. Previously, it was always returning a `sub_expr_desc` => then
executing the remaining function block.
The changes I made here was to (1) bring it back closer to the original
code, while (2) incorporating the jump contract from the consolidated visitor.
--
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]
