From 83e279d340cf77fa9bf3a9276c05b980495e82f2 Mon Sep 17 00:00:00 2001 From: Yu-Chuan Hung Date: Sun, 25 Oct 2026 00:19:35 +0800 Subject: [PATCH] feat: deprecate AggregateUDFImpl::is_nullable in favour of return_field --- datafusion/expr/src/test/function_stub.rs | 7 +++--- datafusion/expr/src/udaf.rs | 5 +++++ datafusion/ffi/src/udaf/mod.rs | 1 + .../src/approx_distinct.rs | 4 ++-- datafusion/functions-aggregate/src/count.rs | 4 ++-- datafusion/functions-aggregate/src/regr.rs | 10 +++++++-- datafusion/physical-expr/src/aggregate.rs | 2 +- .../library-user-guide/upgrading/56.0.0.md | 22 +++++++++++++++++++ 8 files changed, 45 insertions(+), 10 deletions(-) diff --git a/datafusion/expr/src/test/function_stub.rs b/datafusion/expr/src/test/function_stub.rs index a1f29b649b2f8..e1de6c9bab462 100644 --- a/datafusion/expr/src/test/function_stub.rs +++ b/datafusion/expr/src/test/function_stub.rs @@ -22,8 +22,9 @@ use arrow::datatypes::{ DECIMAL32_MAX_PRECISION, DECIMAL32_MAX_SCALE, DECIMAL64_MAX_PRECISION, DECIMAL64_MAX_SCALE, DECIMAL128_MAX_PRECISION, DECIMAL128_MAX_SCALE, - DECIMAL256_MAX_PRECISION, DECIMAL256_MAX_SCALE, DataType, FieldRef, + DECIMAL256_MAX_PRECISION, DECIMAL256_MAX_SCALE, DataType, Field, FieldRef, }; +use std::sync::Arc; use datafusion_common::plan_err; use datafusion_common::{Result, exec_err, not_impl_err, utils::take_function_args}; @@ -252,8 +253,8 @@ impl AggregateUDFImpl for Count { Ok(DataType::Int64) } - fn is_nullable(&self) -> bool { - false + fn return_field(&self, _arg_fields: &[FieldRef]) -> Result { + Ok(Arc::new(Field::new(self.name(), DataType::Int64, false))) } fn state_fields(&self, _args: StateFieldsArgs) -> Result> { diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index 5bfc76242cc0f..f1c9e0e7ed727 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -208,7 +208,9 @@ impl AggregateUDF { self.inner.window_function_display_name(params) } + #[deprecated(since = "56.0.0", note = "Use `AggregateUDF::return_field` instead.")] pub fn is_nullable(&self) -> bool { + #[expect(deprecated)] self.inner.is_nullable() } @@ -570,6 +572,7 @@ pub trait AggregateUDFImpl: Debug + DynEq + DynHash + Send + Sync + Any { /// For example, aggregate functions like `COUNT` always return a non null value /// but others like `MIN` will return `NULL` if there is nullable input. /// Note that if the function is declared as *not* nullable, make sure the [`AggregateUDFImpl::default_value`] is `non-null` + #[deprecated(since = "56.0.0", note = "Use `return_field` instead.")] fn is_nullable(&self) -> bool { true } @@ -1514,6 +1517,7 @@ pub fn udaf_default_return_field( Ok(Arc::new(Field::new( func.name(), data_type, + #[expect(deprecated)] func.is_nullable(), ))) } @@ -1678,6 +1682,7 @@ impl AggregateUDFImpl for AliasedAggregateUDFImpl { } fn is_nullable(&self) -> bool { + #[expect(deprecated)] self.inner.is_nullable() } diff --git a/datafusion/ffi/src/udaf/mod.rs b/datafusion/ffi/src/udaf/mod.rs index df27ec78bac80..e0a592718359a 100644 --- a/datafusion/ffi/src/udaf/mod.rs +++ b/datafusion/ffi/src/udaf/mod.rs @@ -399,6 +399,7 @@ impl From> for FFI_AggregateUDF { let name = udaf.name().into(); let aliases = udaf.aliases().iter().map(|a| a.to_owned().into()).collect(); + #[expect(deprecated)] let is_nullable = udaf.is_nullable(); let volatility = udaf.signature().volatility.into(); diff --git a/datafusion/functions-aggregate/src/approx_distinct.rs b/datafusion/functions-aggregate/src/approx_distinct.rs index 7da68c47bcd2a..93233cd9e4cf4 100644 --- a/datafusion/functions-aggregate/src/approx_distinct.rs +++ b/datafusion/functions-aggregate/src/approx_distinct.rs @@ -742,8 +742,8 @@ impl AggregateUDFImpl for ApproxDistinct { Ok(ScalarValue::UInt64(Some(0))) } - fn is_nullable(&self) -> bool { - false + fn return_field(&self, _arg_fields: &[FieldRef]) -> Result { + Ok(Arc::new(Field::new(self.name(), DataType::UInt64, false))) } fn state_fields(&self, args: StateFieldsArgs) -> Result> { diff --git a/datafusion/functions-aggregate/src/count.rs b/datafusion/functions-aggregate/src/count.rs index 7c3bfdceefdf7..fa191a01dfdf5 100644 --- a/datafusion/functions-aggregate/src/count.rs +++ b/datafusion/functions-aggregate/src/count.rs @@ -293,8 +293,8 @@ impl AggregateUDFImpl for Count { Ok(DataType::Int64) } - fn is_nullable(&self) -> bool { - false + fn return_field(&self, _arg_fields: &[FieldRef]) -> Result { + Ok(Arc::new(Field::new(self.name(), DataType::Int64, false))) } fn state_fields(&self, args: StateFieldsArgs) -> Result> { diff --git a/datafusion/functions-aggregate/src/regr.rs b/datafusion/functions-aggregate/src/regr.rs index b6b4f2a7e4f5e..c510b6d1f6eb2 100644 --- a/datafusion/functions-aggregate/src/regr.rs +++ b/datafusion/functions-aggregate/src/regr.rs @@ -465,8 +465,14 @@ impl AggregateUDFImpl for Regr { } } - fn is_nullable(&self) -> bool { - self.regr_type != RegrType::Count + fn return_field(&self, arg_fields: &[FieldRef]) -> Result { + let arg_types: Vec<_> = + arg_fields.iter().map(|f| f.data_type()).cloned().collect(); + Ok(Arc::new(Field::new( + self.name(), + self.return_type(&arg_types)?, + self.regr_type != RegrType::Count, + ))) } fn accumulator(&self, acc_args: AccumulatorArgs) -> Result> { diff --git a/datafusion/physical-expr/src/aggregate.rs b/datafusion/physical-expr/src/aggregate.rs index 71150d330ac29..df583cb5a427b 100644 --- a/datafusion/physical-expr/src/aggregate.rs +++ b/datafusion/physical-expr/src/aggregate.rs @@ -301,7 +301,7 @@ impl AggregateExprBuilder { if let Some(output_metadata) = output_metadata { return_field = output_metadata.add_to_field_ref(return_field); } - let is_nullable = fun.is_nullable(); + let is_nullable = return_field.is_nullable(); let Some(name) = alias else { return internal_err!( "AggregateExprBuilder::alias must be provided prior to calling build" diff --git a/docs/source/library-user-guide/upgrading/56.0.0.md b/docs/source/library-user-guide/upgrading/56.0.0.md index e927d3ec4ae0e..67293f09e023d 100644 --- a/docs/source/library-user-guide/upgrading/56.0.0.md +++ b/docs/source/library-user-guide/upgrading/56.0.0.md @@ -1342,3 +1342,25 @@ 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. + +### `AggregateUDFImpl::is_nullable` is deprecated + +`AggregateUDFImpl::is_nullable` is deprecated; set the nullability in the field +returned by `AggregateUDFImpl::return_field` instead. The compiler does not +warn about existing overrides. + +**Migration guide:** + +```rust,ignore +// Before +fn is_nullable(&self) -> bool { + false +} + +// After +fn return_field(&self, _arg_fields: &[FieldRef]) -> Result { + Ok(Arc::new(Field::new(self.name(), DataType::Int64, false))) +} +``` + +See [issue #18882](https://github.com/apache/datafusion/issues/18882) for details.