alamb commented on code in PR #25561:
URL: https://github.com/apache/datafusion/pull/25561#discussion_r4366822535
##########
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:
this seems like an unecessary level of detail for user documentation -- it
follows from the definition of volatile functions I think
##########
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:
Can we please use slt tests here? The rust tests are pretty verbose and I
think slt would offer better coverage?
##########
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:
I am worried that this path will almost never be invoked and it is quite a
bit slower than the fast CASE implementation we already have.
##########
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:
What do you think about doing something with lambda expressions so we can
evaluate once and then use the binding? for example, something like this?
```sql
coalesce(v, b, c)
--> apply(v, x -> CASE WHEN x IS NOT NULL THEN x ELSE coalesce(b, c) END)
```
--
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]
