alamb commented on code in PR #11265:
URL: https://github.com/apache/datafusion/pull/11265#discussion_r1666987160
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -917,27 +912,36 @@ struct ExprIdentifierVisitor<'a, 'n> {
/// Record item that used when traversing an expression tree.
enum VisitRecord<'n> {
- /// Contains the post-order index assigned in during the first, visiting
traversal and
- /// a boolean flag to indicate if the record marks an expression subtree
(not just a
- /// single node).
+ /// Marks the beginning of expression. It contains:
+ /// - The post-order index assigned during the first, visiting traversal.
+ /// - A boolean flag if the record marks an expression subtree (not just a
single
+ /// node).
EnterMark(usize, bool),
- /// Accumulated identifier of sub expression.
- ExprItem(Identifier<'n>),
+
+ /// Marks an accumulated subexpression tree. It contains:
+ /// - The accumulated identifier of a subexpression.
+ /// - A boolean flag if the expression is valid for subexpression
elimination.
+ /// The flag is propagated up from children to parent. (E.g. volatile
expressions
+ /// are not valid and can't be extracted, but non-volatile children of
volatile
+ /// expressions can be extracted.)
+ ExprItem(Identifier<'n>, bool),
}
impl<'n> ExprIdentifierVisitor<'_, 'n> {
/// Find the first `EnterMark` in the stack, and accumulates every
`ExprItem`
/// before it.
Review Comment:
Could you possibly update this doc string to explain the 4-tuple that is now
returned? Specifically document the two different `bool` values?
--
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]
