doc: why nullable of list item is set to true

[jcsherin]

Which issue does this PR close?

Closes #11625.

Rationale for this change

When working on issues related to #8708 there have been multiple PRs which dealt with the nullability of list item in accumulator state. This doc patch makes the reasoning of existing code explicit.

What changes are included in this PR?

Only doc comments are added. There are no code changes.

Aggregate functions which use data type of first argument:

1. ArrayAgg
2. NthValueAgg
3. Count

Aggregate functions which use data type of returned value:

1. BitwiseOperation
2. Sum

Are these changes tested?

Are there any user-facing changes?

[jcsherin]

Notes:

1. I felt inline comments made more sense here rather than adding this as part of docs for state_fields. This is primarily intended for the reader of code rather than the user of the library.
2. There is repetition of comment text in aggregate functions.

Please recommend changes to copy or critique this approach.

[comphead]

tbh I would be putting this to trait object instead of copying through methods.

[alamb]

tbh I would be putting this to trait object instead of copying through methods.

Do you mean add the documentation to https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.AggregateUDFImpl.html#method.state_fields ?

[comphead]

tbh I would be putting this to trait object instead of copying through methods.

Do you mean add the documentation to https://docs.rs/datafusion/latest/datafusion/logical_expr/trait.AggregateUDFImpl.html#method.state_fields ?

yep, it might be a better place imho. Especially if it related to all function implementations

review again

[jcsherin]

tbh I would be putting this to trait object instead of copying through methods.

This does not belong in trait object because this affects only a few aggregate functions, not all of them.

For some aggregate functions the intermediate accumulator state often has:

1. a list of items and,
2. the type of the item is same as the first argument or the returned value

Field::new_list( 
    format_state_name(args.name, "distinct_array_agg"), 
    Field::new("item", args.input_type.clone(), true), // [1] should always be true
    true,  // [2] or false
)

At first glance it looked like nullable of the list item should be configurable. There were also multiple PRs in this direction before we realized it was all unnecessary.

To get rid of the comment duplication, I'll instead move them to a markdown doc within functions-aggregate and link to it from the comments.

[alamb]

Looks like an improvement to me -- thank you @jcsherin and @jayzhan211 and @comphead

I also marked this PR ready for review as it looks good to me

[alamb]

👍

[alamb]

Is another rationale that the intermediate results need to be able to represent "saw no rows" (e.g that partition had no values)?

[jcsherin]

For nth_value accumulator, when now rows are present in the partition, then no values are added to the intermediate state.

I haven't checked the other aggregates though. So I don't know for certain if this is the case always. I'll verify and make a follow-on PR if any differences exist. I think we've only looked deeper into nth_value and array_agg (by @jayzhan211) at the moment.

[alamb]

Makes sense -- I vaguely remember that the null was needed in one of the aggregators to distinguish between

1. only empty lists had been seen []
2. No lists at all had been seen NULL

[jcsherin]

@alamb Thanks for the pointer. I'll keep this in mind while making pass through the aggregates next time.

[jcsherin]

I made a minor copy change to disambiguate that the "Computing Intermediate State" section is talking about the nullability of the list item rather than the nullability of the list container.

Sorry for the confusion. I was not clear earlier.

[jcsherin]

I pushed a minor copy edit and CI failed. Looking at the error logs it looks to me like it is not related to this change.

[jcsherin]

I pushed a minor copy edit and CI failed. Looking at the error logs it looks to me like it is not related to this change.

The clippy errors in CI are being tracked here - #11651.

[alamb]

I merged up from main to get the fix for the clippy errors

[jcsherin]

I merged up from main to get the fix for the clippy errors

@alamb Thank you.

[alamb]

Thanks @jcsherin

[alamb]

Thanks again - we can iterate on the docs in follow on PRs if there is more to do

[jcsherin]

Thanks for the review feedback - @alamb, @comphead and for prior discussions @jayzhan211.