GH-50515: [C++][Compute] Respect parent validity bitmap when casting nested structs with non-nullable fields#50546
Conversation
…ct cast When casting a struct with a nullable->non-nullable field change, CastStruct::Exec checks GetNullCount() on the child array without considering the parent struct's validity bitmap. This rejects casts where the child's physical nulls are fully masked by parent-level nulls. Intersect the parent and child validity bitmaps before deciding whether unmasked nulls exist, using BinaryBitBlockCounter for the common case and explicit fallbacks for absent bitmaps.
|
|
You can use |
| } | ||
| position += block.length; | ||
| } | ||
| } |
There was a problem hiding this comment.
If we get here, I think we should make sure that the final casted child does not have a null bitmap, otherwise it might violate expectations for a nullable field (which are not well specified, unfortunately).
| // Child has nulls but no bitmap (e.g. NullArray, RunEndEncoded, Union). | ||
| // We must semantically check if any valid parent element corresponds to a null child. |
There was a problem hiding this comment.
The BinaryBitBlockCounter path below does not account for logical nulls, so I don't think this one should, either.
We can just hardcode an error for NullArray.
|
@aaron-seq @davlee1972 thanks for the issue and PR. This is a bit tricky as we have never formalized what the nullable flag means for non-trivial types, so I've started a discussion on the dev ML to make sure we don't go into the wrong direction here: |
|
I understand the complexity and for pure performance reasons I don't think you want to add Nulls in a non nullable array to begin with, but I'm forced to even out array lengths between parent records and child records. I added a function called fill_nulls_with_dummy() as a workaround to populate any non nullable array with zeros, empty string, 1970 dates, 0 duration, etc.. Maybe what we really need is a pyarrow.notnull() type, an option in pyarrow.compute.fill_null() to use a dummy fill value based on the values type and a pyarrow compute non_null_scalar() function that takes a type and returns back a dummy scalar. |
|
I actually like how assembling list arrays work with a pure non nullable array with an offsets array which has None for null positions without having to pollute the data array with None values. It would be a major change to support an offsets array for struct arrays.. [0,1,2,None,None,3]. For a struct array with only four non nullable array values but contains two additional null entries. |
Rationale for this change
When casting a nested struct whose inner field changes from nullable to
non-nullable,
CastStruct::Execcurrently callsin_values->GetNullCount() > 0without considering the parent struct'svalidity bitmap. This causes a spurious
ArrowInvaliderror when thechild's physical nulls are fully masked by null parent entries.
Reported in #50515.
What changes are included in this PR?
In
scalar_cast_nested.cc, replace the unconditional rejection with athree-way check:
BinaryBitBlockCounter::NextAndNotWord()to detect parent-valid-AND-child-null positions without heap allocation.
Are these changes tested?
Five new C++ test cases in
scalar_cast_test.cc:StructNestedNullabilityAbsentParent— true violation, must rejectStructNestedNullabilityMasked— masked null, must succeedStructNestedNullabilitySliced— offset correctness for both outcomesStructNestedNullabilityAbsentChild— no nulls, must succeedStructNestedNullabilityDeep— 3-level nesting with masked nullOne new Python test in
test_compute.py:test_cast_struct_nested_nullability— end-to-end PyArrow validationcovering success, rejection, and sliced array cases
Are there any user-facing changes?
Struct casts that were previously rejected with
ArrowInvalid: field '...' has nullswill now succeed when the nullsare masked by parent-level nulls. This is a correctness fix, not a
behavior change — the previous behavior was incorrect per the Arrow
columnar format specification.