alamb commented on code in PR #11265:
URL: https://github.com/apache/datafusion/pull/11265#discussion_r1666985733
##########
datafusion/expr/src/expr.rs:
##########
@@ -1413,12 +1413,16 @@ impl Expr {
.unwrap()
}
+ /// Returns true if the expression node is volatile, i.e. whether it can
return
+ /// different results when evaluated multiple times with the same input.
+ pub fn is_volatile_node(&self) -> bool {
Review Comment:
I think it would help to clarify in the docs the difference between
`is_volatile` and `is_volatile_node`
Perhaps something like
```suggestion
///
/// Note: unlike [`Self::is_volatile`], this function does not consider
inputs:
/// - `rand()` returns `true`,
/// - `a + rand()` returns `false`
pub fn is_volatile_node(&self) -> bool {
```
(and something similar for `is_volatile`)
##########
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.
- fn pop_enter_mark(&mut self) -> (usize, bool, Option<Identifier<'n>>) {
+ fn pop_enter_mark(&mut self) -> (usize, bool, Option<Identifier<'n>>,
bool) {
let mut expr_id = None;
+ let mut is_valid = true;
Review Comment:
Could you please document what "valid" means in this context? I think it
means "is valid for CSE" as in "this sub expression could potentially be
removed via CSE" but I am not quite sure
--
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]
