You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2025/08/28 20:53:14 UTC
[PR] Add PhysicalExpr::is_volatile [datafusion]
adriangb opened a new pull request, #17351:
URL: https://github.com/apache/datafusion/pull/17351
Closes https://github.com/apache/datafusion/issues/17318
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2324658328
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -560,3 +573,26 @@ pub fn is_dynamic_physical_expr(expr: &Arc<dyn PhysicalExpr>) -> bool {
// If the generation is non-zero, then this `PhysicalExpr` is dynamic.
snapshot_generation(expr) != 0
}
+
+/// Returns true if the expression is volatile, i.e. whether it can return different
+/// results when evaluated multiple times with the same input.
+///
+/// For example the function call `RANDOM()` is volatile as each call will
+/// return a different value.
+///
+/// This method recursively checks if any sub-expression is volatile, for example
+/// `1 + RANDOM()` will return `true`.
+pub fn is_volatile(expr: &Arc<dyn PhysicalExpr>) -> bool {
+ let mut is_volatile = expr.is_volatile_node();
+ expr.apply(|e| {
+ is_volatile = is_volatile || e.is_volatile_node();
Review Comment:
You mean check the root node before recursing?
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3247256961
Functions are the only ones that I know of
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3258493991
Small followup: https://github.com/apache/datafusion/pull/17443
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2317444574
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
Review Comment:
I think it makes sense to give the option to implement for a couple releases before we force it. It means a less aggressive upgrade cycle.
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3247250413
I think we only add one simple test to show that it picks up the volatile node.
Now that you mention it I don't know which nodes are currently volatile that arent just the functions. Were you planning on marking nodes as volatile or is this more of a preemptive pr?
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2317357983
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
Review Comment:
Should we not give a default and have every expression implement this. I could see this being missed + difficult for reviewers to look at for this when new physical expressions are added. Could do as follow up.
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2308522201
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,40 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`Self::is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile.
Review Comment:
```suggestion
/// By default, expressions are not volatile to avoid imposing API churn on implementers.
/// It is highly recommended that volatile expressions implement this method and return `true`.
```
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,40 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`Self::is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile.
+ fn is_volatile_node(&self) -> bool {
+ false
+ }
+
+ /// Returns true if the expression is volatile, i.e. whether it can return different
+ /// results when evaluated multiple times with the same input.
+ ///
+ /// For example the function call `RANDOM()` is volatile as each call will
+ /// return a different value.
+ ///
+ /// This method recursively checks if any sub-expression is volatile.
Review Comment:
```suggestion
/// This method recursively checks if any sub-expression is volatile, for example
/// `1 + RANDOM()` will return `true.
```
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2324657452
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
+ false
Review Comment:
I agree this is not a safe default long term. I can add a comment about it.
Do you think it's better to just eat the cost of the breaking change now? My thought was that we've lived with a world where if you have a custom volatile expression you can't teach DataFusion about it for a long time now. This just means that you can fix it in your code instead of ours. But having no default means breaking changes for a lot of people.
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3247261016
FWIW my reasoning behind this proposal is that:
1. We already had it in DataFusion for logical expressions.
2. We already were doing downcasting and the same logic just privately.
3. My team needed that same code so instead of copying it again I thought I'd make it a public API.
4. Doing this via downcast matching precludes third party crates, etc from marking themselves as volatile.
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2324687435
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -560,3 +573,26 @@ pub fn is_dynamic_physical_expr(expr: &Arc<dyn PhysicalExpr>) -> bool {
// If the generation is non-zero, then this `PhysicalExpr` is dynamic.
snapshot_generation(expr) != 0
}
+
+/// Returns true if the expression is volatile, i.e. whether it can return different
+/// results when evaluated multiple times with the same input.
+///
+/// For example the function call `RANDOM()` is volatile as each call will
+/// return a different value.
+///
+/// This method recursively checks if any sub-expression is volatile, for example
+/// `1 + RANDOM()` will return `true`.
+pub fn is_volatile(expr: &Arc<dyn PhysicalExpr>) -> bool {
+ let mut is_volatile = expr.is_volatile_node();
+ expr.apply(|e| {
+ is_volatile = is_volatile || e.is_volatile_node();
Review Comment:
I thought about sth like
```rust
pub fn is_volatile(expr: &Arc<dyn PhysicalExpr>) -> bool {
if expr.is_volatile_node() {
return true;
}
let mut is_volatile = false;
expr.apply(|e| {
if e.is_volatile_node() {
is_volatile = true;
Ok(TreeNodeRecursion::Stop)
} else {
Ok(TreeNodeRecursion::Continue)
}
})
.expect("infallible closure should not fail");
is_volatile
}
```
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3257918095
Thank you for the review folks. I plan to merge this once CI passes.
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2317357983
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
Review Comment:
Should we not give a default and have every expression implement this. I could see this being missed + difficult for reviewers to keep in mind when new physical expressions are added. Could do as follow up if we want to do this (lots of tedious work 😆 ).
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3257592974
LGTM
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2324657452
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
+ false
Review Comment:
I agree this is not a safe default long term. I can add a comment about it.
Do you think it's better to just eat the cost of the breaking change now? My thought was that we've lived with a world where if you have a custom volatile expression you can't teach DataFusion about it for a long time now. This just means that you can fix it in your code instead of ours. But having no default means breaking changes for a lot of people. That is, this change with the unsafe default is a better situation than the current status quo for everyone, even if it isn't perfect. Making the breaking changes is arguably a worse situation for some people (harder to upgrade). And if we put the default in there we can always remove it later, but not the other way around.
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3255965541
@findepi could you take a look at this please?
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2317361993
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
Review Comment:
Also maybe `is_volatile` is enough
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "xudong963 (via GitHub)" <gi...@apache.org>.
xudong963 commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2324227931
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
Review Comment:
```suggestion
/// - `a + random()` returns `false` (becuase the operation `+` itself is not volatile.)
```
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2324681354
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
+ false
Review Comment:
> My thought was that we've lived with a world where if you have a custom volatile expression you can't teach DataFusion about it for a long time now.
good point
let's leave it as is
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2324504677
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
Review Comment:
> Also maybe `is_volatile` is enough
`is_volatile` should return whether the "expression is volatile".
it would be enough, but it needs to traverse the expr tree
> I could see this being missed + difficult for reviewers to keep in mind when new physical expressions are added.
The default impl is not a safe default. What happens when someone has existing volatile `PhysicalExpr` implementation and doesn't override this method?
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
+ false
Review Comment:
This is likely not a safe default. What happens when someone has existing volatile PhysicalExpr implementation and doesn't override this method?
Today the answer is "nothing", as these functions are not used for anything. But as soon as they are used for something (eg some optimization pass), this not safe a default may lead to correctness bugs (like constant-folding a rand call).
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -560,3 +573,26 @@ pub fn is_dynamic_physical_expr(expr: &Arc<dyn PhysicalExpr>) -> bool {
// If the generation is non-zero, then this `PhysicalExpr` is dynamic.
snapshot_generation(expr) != 0
}
+
+/// Returns true if the expression is volatile, i.e. whether it can return different
+/// results when evaluated multiple times with the same input.
+///
+/// For example the function call `RANDOM()` is volatile as each call will
+/// return a different value.
+///
+/// This method recursively checks if any sub-expression is volatile, for example
+/// `1 + RANDOM()` will return `true`.
+pub fn is_volatile(expr: &Arc<dyn PhysicalExpr>) -> bool {
+ let mut is_volatile = expr.is_volatile_node();
+ expr.apply(|e| {
+ is_volatile = is_volatile || e.is_volatile_node();
Review Comment:
No need to call `expr.apply` when `is_volatile` is already true. Does it matter?
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#discussion_r2317357983
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -377,6 +377,19 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
// static expressions will always return 0.
0
}
+
+ /// Returns true if the expression node is volatile, i.e. whether it can return
+ /// different results when evaluated multiple times with the same input.
+ ///
+ /// Note: unlike [`is_volatile`], this function does not consider inputs:
+ /// - `random()` returns `true`,
+ /// - `a + random()` returns `false`
+ ///
+ /// By default, expressions are not volatile to avoid imposing API churn on implementers.
+ /// It is highly recommended that volatile expressions implement this method and return `true`.
+ fn is_volatile_node(&self) -> bool {
Review Comment:
Should we not give a default and have every expression implement this. I could see this being missed + difficult for reviewers to keep in mind when new physical expressions are added. Could do as follow up.
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #17351:
URL: https://github.com/apache/datafusion/pull/17351#issuecomment-3247183043
> We can add this to existing volatile expressions in this PR + add tests, or we could do in follow up if not possible.
How many cases are there? If it's reasonable to do here let's do it, but not if it obscures the new API.
What tests do you think we need to add? For each implementation?
--
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
Re: [PR] Add PhysicalExpr::is_volatile [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #17351:
URL: https://github.com/apache/datafusion/pull/17351
--
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