DeviousCardi commented on code in PR #25561:
URL: https://github.com/apache/datafusion/pull/25561#discussion_r4369737388
##########
datafusion/functions/src/core/coalesce.rs:
##########
@@ -29,7 +32,7 @@ use itertools::Itertools;
#[user_doc(
doc_section(label = "Conditional Functions"),
- description = "Returns the first of its arguments that is not _null_.
Returns _null_ if all arguments are _null_. This function is often used to
substitute a default value for _null_ values.",
+ description = "Returns the first of its arguments that is not _null_.
Returns _null_ if all arguments are _null_. Arguments after the first
non-_null_ one are normally not evaluated, but when any argument other than the
last is volatile every argument is evaluated, so that each is evaluated exactly
once. This function is often used to substitute a default value for _null_
values.",
Review Comment:
Agreed, removed in 3289dc434. The coalesce and nvl descriptions (and the
generated scalar_functions.md) are back to their original text.
##########
datafusion/functions/src/core/coalesce.rs:
##########
@@ -146,3 +223,211 @@ impl ScalarUDFImpl for CoalesceFunc {
self.doc()
}
}
+
+#[cfg(test)]
+mod tests {
+ use std::sync::Arc;
+
+ use arrow::array::{Array, ArrayRef, Int32Array, StringArray};
+ use arrow::datatypes::DataType;
+ use datafusion_common::ScalarValue;
+ use datafusion_common::config::ConfigOptions;
+ use datafusion_expr::{ColumnarValue, Expr, ScalarUDFImpl, lit};
+
+ use super::*;
+
+ fn invoke(args: Vec<ColumnarValue>, number_rows: usize) -> ColumnarValue {
Review Comment:
Done in 3289dc434. The Rust unit tests are gone and the coverage is now in
coalesce.slt. To make the volatile path deterministic the tests use `case when
random() >= 0 then x end`, which is volatile but always equals `x`, so they can
check the kernel against exact expected rows (array args, a scalar fallback,
all-null input, strings, and a volatile middle argument). The 100k-row
`random()` query that reproduces the original bug is still there, now as a
single query covering coalesce and nvl. With the volatility guard removed,
three of the new queries fail.
##########
docs/source/library-user-guide/upgrading/56.0.0.md:
##########
@@ -1306,3 +1306,29 @@ without declaring one, use
`FileScanConfigBuilder::with_preserve_order(true)`.
Code that reads `FileScanConfig::preserve_order` to decide whether files may be
reordered should call `FileScanConfig::preserves_file_order()` instead, which
also accounts for the output ordering.
+
+### A volatile `coalesce` or `nvl` argument is now evaluated eagerly
Review Comment:
Makes sense, removed in 3289dc434.
##########
datafusion/functions/src/core/coalesce.rs:
##########
@@ -117,8 +139,63 @@ impl ScalarUDFImpl for CoalesceFunc {
}
/// coalesce evaluates to the first value which is not NULL
- fn invoke_with_args(&self, _args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
- internal_err!("coalesce should have been simplified to case")
+ ///
+ /// This kernel is only reached when [`Self::simplify`] declined to
rewrite the
Review Comment:
Thanks, I looked into this. With the lambda support from #21679 the rewrite
you describe can be expressed, and I think it is the better long-term shape.
`v` would be evaluated once, `b` and `c` would stay lazy because the CASE
inside the body only evaluates its ELSE on the remaining rows, and it would
reuse the fast CASE implementation instead of a separate kernel.
It is not a small change today, though:
- There is no scalar `apply`/`let` higher-order function yet. All of the
existing ones are list functions in functions-nested, and those are only
registered when the `nested_expressions` feature is enabled. So we would need a
new function in core that is always registered, either public or hidden.
- Lambda variables are normally resolved by the SQL planner. `simplify` runs
after that, so it would have to build an already-resolved `LambdaVariable` and
the captures for `b` and `c` itself.
- Substrait has no lambda support, so an optimized plan with a volatile
coalesce would stop being serializable there. Proto deserialization would also
need the function to be in the registry.
In the meantime, 3289dc434 narrows the special case so the slow path covers
as little as possible. The CASE rewrite now runs up to the first volatile
non-final argument, and only the rest goes to the kernel: `coalesce(a, v, c)`
becomes `CASE WHEN a IS NOT NULL THEN a ELSE coalesce(v, c) END`. Calls with no
volatile non-final argument plan exactly as before and never reach the kernel.
The kernel only runs when a volatile value has to be checked and returned, and
in that case it is the only correct option we have right now.
If you are OK with it, I would keep this narrow fix for the bug and open a
follow-up issue for a general scalar binding function (`apply(v, x -> ...)`).
Once that exists, this kernel could be replaced, and any other rewrite that
duplicates an argument could use the same function.
--
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]
