kosiew commented on code in PR #18191:
URL: https://github.com/apache/datafusion/pull/18191#discussion_r2450406532
##########
datafusion/functions/src/core/nvl2.rs:
##########
@@ -95,8 +96,71 @@ impl ScalarUDFImpl for NVL2Func {
Ok(arg_types[1].clone())
}
+ fn return_field_from_args(&self, args: ReturnFieldArgs) ->
Result<FieldRef> {
+ let nullable =
+ args.arg_fields[1].is_nullable() ||
args.arg_fields[2].is_nullable();
+ let return_type = args.arg_fields[1].data_type().clone();
+ Ok(Field::new(self.name(), return_type, nullable).into())
+ }
+
fn invoke_with_args(&self, args: ScalarFunctionArgs) ->
Result<ColumnarValue> {
- nvl2_func(&args.args)
+ let [test, if_non_null, if_null] = take_function_args(self.name(),
args.args)?;
Review Comment:
> make invoke_with_args return an internal error instead of retaining the
implementation.
The new fallback evaluator is exercised directly in
test_create_physical_expr_nvl2, which builds physical expressions without
running the simplifier. Returning an error here would regress those
non-simplified code paths.
It isn’t just about satisfying a unit test, it's about preserving a
supported API surface.
[SessionContext::create_physical_expr](https://github.com/apache/datafusion/blob/531af8e/datafusion/core/src/execution/session_state.rs#L674-L706)
explicitly states that it performs coercion and rewrite passes but does not
run the expression simplifier, so any expression handed directly to that API
must still execute correctly without being rewritten to a CASE statement first.
The test_create_physical_expr_nvl2 fixture exercises exactly that public
workflow by building a physical expression through
SessionContext::create_physical_expr and evaluating it without simplification.
If we changed invoke_with_args to return an error, that flow would regress
for library users in the same way it would fail for the test.
Rather than removing or rewriting the test, I think we should keep it to
guard this behavior; it’s effectively documenting that nvl2 continues to work
for consumers who rely on the non-simplifying physical-expr builder, which the
function implementation currently supports.
I recommend keeping the implementation so those tests—and any downstream
consumers that bypass simplification—continue to 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: [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]
