feat: deprecate AggregateUDF: :is_nullable

[codetyri0n]

Which issue does this PR close?

• Closes Deprecate 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`]
	pub is_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.