istvan-fodor commented on code in PR #5541:
URL: https://github.com/apache/arrow-rs/pull/5541#discussion_r1549918256
##########
arrow-array/src/builder/fixed_size_list_builder.rs:
##########
@@ -248,9 +325,37 @@ mod tests {
}
#[test]
- fn test_fixed_size_list_array_builder_finish_cloned() {
+ fn test_fixed_size_list_array_builder_with_field() {
+ let builder = make_list_builder(false, false);
Review Comment:
@alamb , I had a question about this test in particular. With the new
capability to set the field as a non-nullable we have the situation where the
builder has a null element built like this:
```rust
builder.values().append_null();
builder.values().append_null();
builder.values().append_null();
builder.append(false);
```
If the field is set to non-nullable, this element will fail the null check.
I wanted to check with you if this is logically right or not? Are we making a
distinction between nulls in the values builder, where let's say 1 of the 3
elements of a particular fixed size list is null (which would be a valid fail
on non-nullability) or nulls in the main builder (which is built up as 3 nulls
+ a `append(false)` in the values builder). To me the second one is
questionable and comes down to how you want this to work. If this test should
not fail with the second complete null case, then I have to go back and revise
the null check of the values builder and somehow control for null elements of
the main builder.
--
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]
