kosiew commented on code in PR #24029:
URL: https://github.com/apache/datafusion/pull/24029#discussion_r3711747526
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -1059,7 +1059,7 @@ impl Accumulator for DistinctArrayAggAccumulator {
.map(|i| ScalarValue::try_from_array(decoded.as_ref(), i))
.collect::<Result<_>>()?;
- let arr = ScalarValue::new_list(&values, &self.datatype, true);
+ let arr = ScalarValue::new_list(&values, decoded.data_type(), true);
Review Comment:
Could we keep using `self.datatype` here?
`ARRAY_AGG(DISTINCT ...)` needs to return the declared element type because
that is the type used by `return_type` and the state fields. Deriving the list
child type from `decoded` makes the result depend on the runtime column or row
decoder schema, which can preserve different nested field nullability.
That would bring back the declared-versus-produced schema mismatch this PR
is fixing, specifically on the DISTINCT path. Please pass `&self.datatype` to
`new_list` and update the test builder or input schema rather than bypassing
the constructor's normalization.
Could you also add a nested struct nullability regression that asserts the
accumulator result type exactly?
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -1429,14 +1429,15 @@ impl Accumulator for OrderSensitiveArrayAggAccumulator {
}
let values = self.values.clone();
+ let values_data_type = values[0].data_type();
Review Comment:
Could we use `&self.datatypes[0]` for both the empty and non-empty cases
here?
Selecting `values[0].data_type()` makes the ordered `ARRAY_AGG` output
schema depend on the first runtime scalar, while the empty case still uses the
declared type. With nested values, that can make field nullability differ
between empty and non-empty groups and violate the aggregate return and state
schema.
The new list cast should normalize the scalar values to the declared type,
so the declared datatype should remain the source of truth. Please also add
coverage for a non-empty ordered aggregate where a nested field's runtime
nullability differs from its declared nullability.
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -11544,4 +11547,28 @@ mod tests {
run_tests::<Decimal128Type>();
run_tests::<Decimal256Type>();
}
+
+ #[test]
+ fn test_new_list_nested_nullability_mismatch_issue_24022() {
Review Comment:
It would be helpful to extend this regression to `new_list_from_iter` and
`new_large_list`, since all three updated constructors share the same
normalization invariant.
Could the tests also assert the child array values, rather than only
checking `list.data_type()`? That should make it easier to catch any future
divergence between these constructors.
--
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]
