Ensure StructArrays check nullability of fields #3205
Conversation
|
I was under the impression that child fields of a StructArray must have their values respect their nullability (given the linked issue), however given how many tests I needed to fix (particularly MapBuilder) I'm a bit more uncertain on this now... Would appreciate some clarification on expected behaviour. |
| @@ -916,7 +916,7 @@ mod tests { | |||
| #[test] | |||
| fn test_null_struct() { | |||
| let struct_type = | |||
| DataType::Struct(vec![Field::new("data", DataType::Int64, false)]); | |||
| DataType::Struct(vec![Field::new("data", DataType::Int64, true)]); | |||
There was a problem hiding this comment.
This actually highlights a somewhat fun issue that I'm also running into with the row format (#3159), namely how to create a null StructArray with children that aren't nullable. I think new_null_array should discard the null mask for its children, but I'm not 100% sure. I need to think a bit more on this
| @@ -286,8 +286,8 @@ mod tests { | |||
|
|
|||
| let string_builder = builder.keys(); | |||
| string_builder.append_value("joe"); | |||
| string_builder.append_null(); | |||
| string_builder.append_null(); | |||
| string_builder.append_value("n1"); | |||
There was a problem hiding this comment.
We should probably add an error to MapArrayBuilder so that it returns a meaningful error if you try to create a keys array with nulls. I believe this may overlap somewhat with #1697
There was a problem hiding this comment.
I can give this a shot, but probably won't get much progress until the weekend, so can open a separate PR if needed
|
Benchmark runs are scheduled for baseline = 4926bad and contender = 1d6b5ab. 1d6b5ab is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #1167
Rationale for this change
What changes are included in this PR?
Main fix: when building using
StructArray::from, check that if child fields are set as non-nullable, then their values respect this; if they don't then will panicOther fixes to
StructArray::from:[0]to get length to compare)Are there any user-facing changes?