feat: deprecate AggregateUDF: :is_nullable

[codetyri0n]

Which issue does this PR close?

• ClosesDeprecate AggregateUDFImpl::is_nullable in favour of return_field #18882

Rationale for this change

• deprecating is_nullable in favour of return_field as it is already encoded within return_field

Are these changes tested?

• yes

Are there any user-facing changes?

• Deprecated method

[Jefffrey]

Surprisingly fewer changes than I expected

[martin-g]

• https://github.com/codetyri0n/datafusion/blob/810cae5c0471a4050e2a9b4ac8580eaf5f039906/datafusion/ffi/src/udaf/mod.rs#L347 is not annotated with #[expect(deprecated)]
• https://github.com/codetyri0n/datafusion/blob/810cae5c0471a4050e2a9b4ac8580eaf5f039906/datafusion/functions-aggregate/src/count.rs#L299 - should something be done for overrides like this one ?

[martin-g]

Should this method be deprecated too ?

[codetyri0n]

yep thanks!

[codetyri0n]

• https://github.com/codetyri0n/datafusion/blob/810cae5c0471a4050e2a9b4ac8580eaf5f039906/datafusion/ffi/src/udaf/mod.rs#L347 is not annotated with #[expect(deprecated)]
• https://github.com/codetyri0n/datafusion/blob/810cae5c0471a4050e2a9b4ac8580eaf5f039906/datafusion/functions-aggregate/src/count.rs#L299 - should something be done for overrides like this one ?

thanks for catching the mod.rs miss @martin-g , i had left the count.rs file untouched as i dont see the signature or the implementation getting altered much in the near future

[codetyri0n]

can we merge this?

[Jefffrey]

Sorry I haven't gotten around to re-reviewing this after the latest changes

New changes

[Jefffrey]

I think the changes look good in achieving what we want for the issue.

Some notes:

• We should look at UDAFs that implement is_nullable and ensure they implement return_field in line with the deprecation; I think this only occurs for Count
  • Unfortunately it seems Rust can't flag this for us (implementations of deprecated methods)
• I think this PR doesn't introduce behaviour changes so long as any downstream UDAFs that implement is_nullable either don't implement return_field (in which case the default behaviour still uses is_nullable), or if they do implement return_field hopefully it is consistent with is_nullable otherwise behaviour may change
  • That latter case seems like a bug in the downstream anyway, though not sure how to communicate this 🤔

One more thing is I wonder how we should handle this for FFI; for example we have is_nullable as part of the FFI API I believe:

datafusion/datafusion/ffi/src/udaf/mod.rs

Lines 80 to 81 in a1bb74b

	/// FFI equivalent to the `is_nullable` of a [`AggregateUDF`]
	pubis_nullable:bool,

Maybe @timsaucer can weigh in regarding FFI?

Apart from that, it would be good if could get some other opinions on if we want to proceed with this deprecation; I know for the UDF is_nullable deprecation there was some concern, see discussions on #14094 and #17074. cc @alamb if you have time

• Maybe we should include a note in the upgrade guide to make this more visible since Rust can't flag the implementation of a deprecated trait method 🤔

[timsaucer]

Two things:

• The FFI portion looks correct. When we remove the API we can just remove these pieces of code.
• I think we need a note in the upgrade guide.

[github-actions]

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.