jayzhan211 commented on code in PR #26746:
URL: https://github.com/apache/datafusion/pull/26746#discussion_r4412203475
##########
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<FieldRef> {
Review Comment:
@CuteChuanChuan thanks for picking this up! Because the `is_nullable`
override here was replaced rather than kept, the deprecated
`AggregateUDF::is_nullable()` now falls back to the trait default. It returns
`true` for `count`, `approx_distinct` and `regr_count` while `return_field`
says non-null. The FFI export at `ffi/src/udaf/mod.rs:403` also sends `true`
now. I checked on this branch:
```
count is_nullable()=true return_field.nullable=false
approx_distinct is_nullable()=true return_field.nullable=false
regr_count is_nullable()=true return_field.nullable=false
```
Could you keep the old overrides next to the new `return_field` until the
method is removed? Overriding a deprecated trait method doesn't trigger a
warning, so no `#[expect]` is needed. The same applies to `approx_distinct.rs`,
`regr.rs` and the `Count` stub in `function_stub.rs`:
```rs
// Keep the deprecated method consistent with `return_field` until it is
removed
fn is_nullable(&self) -> bool {
false
}
```
A small test would keep the two in sync:
```rs
#[test]
#[expect(deprecated)]
fn is_nullable_matches_return_field() {
let udf = count_udaf();
let field = udf
.return_field(&[Arc::new(Field::new("a", DataType::Int64, true))])
.unwrap();
assert_eq!(udf.is_nullable(), field.is_nullable());
}
```
##########
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();
Review Comment:
Optional cleanup: now that `is_nullable` is just
`return_field.is_nullable()`, the stored field could go away so the two can't
disagree. Remove the `is_nullable: bool` field and its copies in the struct
literals, and change the getter to:
```rs
pub fn is_nullable(&self) -> bool {
self.return_field.is_nullable()
}
```
I tried this locally and the physical-expr tests pass.
--
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]
