jcsherin commented on issue #11433:
URL: https://github.com/apache/datafusion/issues/11433#issuecomment-2237184057
I tested this out in many more ways. Each time making sure it's multi-phase
grouping. So that state fields are being used. This is the understanding I've
reached about the correct `nullable` value for both <ins>the list</ins> and
<ins>the items in the list</ins>:
1. It is impossible for the intermediate state of `values` (the list) to
ever be null. It can be empty, but never null. So the current setting of
`nullable: false` for the list of values is correct.
2. The `nullable` for items in the list need not be configurable(like I
initially thought). The computation of nth row does not depend on the
`nullable` defined in schema of the input column expression. It should remain
set to `true` to allow `null` value to exist as an item in the list. Adding
this field to `StateFieldArgs` for the sake of `nth_value` is unnecessary in my
opinion.
```rust
let mut fields = vec![Field::new_list(
format_state_name(self.name(), "nth_value"),
Field::new("item", args.input_type.clone(), true), // See [2]
false, // See [1]
)];
```
It will be beneficial to future readers of this code to add some comments
here. It took me more time than I think was necessary to reach the conclusions
stated above because of how they are now implicit in code.
But other than that the good news is no code changes are required.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
