alamb commented on code in PR #8520:
URL: https://github.com/apache/arrow-datafusion/pull/8520#discussion_r1424440141
##########
datafusion/sqllogictest/test_files/functions.slt:
##########
@@ -995,3 +995,8 @@ query ?
SELECT find_in_set(NULL, NULL)
----
NULL
+
+query B
Review Comment:
Providing context about what this test is doing might be helpful for the
future. Something like
```suggestion
# Verify that multiple calls to volatile functions like `random()` are not
combined / optimized away
query B
```
##########
datafusion/expr/src/expr.rs:
##########
@@ -1692,6 +1692,24 @@ fn create_names(exprs: &[Expr]) -> Result<String> {
.join(", "))
}
+/// Whether the given expression is volatile, i.e. whether it can return
different results
+/// when evaluated multiple times with the same input.
+#[allow(clippy::match_like_matches_macro)]
+pub fn is_volatile(expr: &Expr) -> bool {
+ match expr {
+ Expr::ScalarFunction(func) => match func.func_def {
+ ScalarFunctionDefinition::BuiltIn(func) => match func {
Review Comment:
I suggest moving this logic into a method like
`ScalarFunctionDefinition::volatility` directly ?
https://github.com/apache/arrow-datafusion/blob/7f312c8a1d82b19f03c640e4a5d7d6437b2ee835/datafusion/expr/src/expr.rs#L339-L348
##########
datafusion/expr/src/expr.rs:
##########
@@ -1692,6 +1692,24 @@ fn create_names(exprs: &[Expr]) -> Result<String> {
.join(", "))
}
+/// Whether the given expression is volatile, i.e. whether it can return
different results
Review Comment:
Volatility is already captured as part of the function signature. I think
you can access it like `func.volatility()`:
https://github.com/apache/arrow-datafusion/blob/7f312c8a1d82b19f03c640e4a5d7d6437b2ee835/datafusion/expr/src/built_in_function.rs#L360-L363
##########
datafusion/expr/src/expr.rs:
##########
@@ -1692,6 +1692,24 @@ fn create_names(exprs: &[Expr]) -> Result<String> {
.join(", "))
}
+/// Whether the given expression is volatile, i.e. whether it can return
different results
+/// when evaluated multiple times with the same input.
+#[allow(clippy::match_like_matches_macro)]
+pub fn is_volatile(expr: &Expr) -> bool {
+ match expr {
+ Expr::ScalarFunction(func) => match func.func_def {
+ ScalarFunctionDefinition::BuiltIn(func) => match func {
+ BuiltinScalarFunction::Random => true,
+ _ => false,
+ },
+ // TODO: Add volatile flag to UDFs
+ ScalarFunctionDefinition::UDF(_) => false,
Review Comment:
I think they can have volatile set as part of their signature:
https://docs.rs/datafusion/latest/datafusion/logical_expr/struct.ScalarUDF.html#structfield.signature
(and a field on Signature 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
