-
Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-50515: [C++][Compute] Respect parent validity bitmap when casting nested structs with non-nullable fields #50546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
379ee41
5fe5a33
7232ac3
da10b42
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| #include "arrow/compute/kernels/common_internal.h" | ||
| #include "arrow/compute/kernels/scalar_cast_internal.h" | ||
| #include "arrow/util/bitmap_ops.h" | ||
| #include "arrow/util/bit_block_counter.h" | ||
| #include "arrow/util/int_util.h" | ||
| #include "arrow/util/logging_internal.h" | ||
|
|
||
|
|
@@ -384,12 +385,48 @@ struct CastStruct { | |
| const auto& in_field = in_type.field(in_field_index); | ||
| const auto& in_values = (in_array.child_data[in_field_index].ToArrayData()->Slice( | ||
| in_array.offset, in_array.length)); | ||
|
|
||
| if (in_field->nullable() && !out_field->nullable() && | ||
| in_values->GetNullCount() > 0) { | ||
| return Status::Invalid( | ||
| "field '", in_field->name(), "' of type ", in_field->type()->ToString(), | ||
| " has nulls. Can't cast to non-nullable field '", out_field->name(), | ||
| "' of type ", out_field_type->ToString()); | ||
| bool has_nulls = false; | ||
| const uint8_t* parent_bitmap = in_array.buffers[0].data; | ||
| const uint8_t* child_bitmap = in_values->buffers.empty() ? nullptr : (in_values->buffers[0] ? in_values->buffers[0]->data() : nullptr); | ||
|
|
||
| if (parent_bitmap == nullptr) { | ||
| // Parent has no nulls. Since child has nulls, they are unmasked. | ||
| has_nulls = true; | ||
| } else if (child_bitmap == nullptr) { | ||
| // 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. | ||
| for (int64_t i = 0; i < in_array.length; ++i) { | ||
| if (arrow::bit_util::GetBit(parent_bitmap, in_array.offset + i) && | ||
| in_values->IsNull(i)) { | ||
| has_nulls = true; | ||
| break; | ||
| } | ||
| } | ||
| } else { | ||
| // Both parent and child have bitmaps. Check if parent is valid AND child is null. | ||
| arrow::internal::BinaryBitBlockCounter bit_counter( | ||
| parent_bitmap, in_array.offset, | ||
| child_bitmap, in_values->offset, in_array.length); | ||
| int64_t position = 0; | ||
| while (position < in_array.length) { | ||
| arrow::internal::BitBlockCount block = bit_counter.NextAndNotWord(); | ||
| if (block.popcount > 0) { | ||
| has_nulls = true; | ||
| break; | ||
| } | ||
| position += block.length; | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
|
|
||
| if (has_nulls) { | ||
| return Status::Invalid( | ||
| "field '", in_field->name(), "' of type ", in_field->type()->ToString(), | ||
| " has nulls. Can't cast to non-nullable field '", out_field->name(), | ||
| "' of type ", out_field_type->ToString()); | ||
| } | ||
| } | ||
| ARROW_ASSIGN_OR_RAISE(Datum cast_values, Cast(in_values, out_field_type, options, | ||
| ctx->exec_context())); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4145,6 +4145,103 @@ TEST(Cast, StructToStructSubsetWithNulls) { | |
| CheckStructToStructSubsetWithNulls(NumericTypes()); | ||
| } | ||
|
|
||
| TEST(Cast, StructNestedNullabilityAbsentParent) { | ||
| auto inner_type_dest = struct_({field("a", int32(), /*nullable=*/false)}); | ||
| auto outer_type_dest = struct_({field("inner", inner_type_dest)}); | ||
| auto inner_type_src = struct_({field("a", int32())}); | ||
| auto outer_type_src = struct_({field("inner", inner_type_src)}); | ||
|
|
||
| auto src = ArrayFromJSON(outer_type_src, R"([ | ||
| {"inner": {"a": 1}}, | ||
| {"inner": {"a": null}} | ||
| ])"); | ||
| EXPECT_RAISES_WITH_MESSAGE_THAT( | ||
| Invalid, ::testing::HasSubstr("has nulls. Can't cast to non-nullable field"), | ||
| Cast(src, CastOptions::Safe(outer_type_dest))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also check with a zero-length input? |
||
| } | ||
|
|
||
| TEST(Cast, StructNestedNullabilityMasked) { | ||
| auto inner_type_dest = struct_({field("a", int32(), /*nullable=*/false)}); | ||
| auto outer_type_dest = struct_({field("inner", inner_type_dest)}); | ||
| auto inner_type_src = struct_({field("a", int32())}); | ||
| auto outer_type_src = struct_({field("inner", inner_type_src)}); | ||
|
|
||
| auto src = ArrayFromJSON(outer_type_src, R"([ | ||
| {"inner": {"a": 1}}, | ||
| null | ||
| ])"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not guarantee that the inner field will have nulls at all, it's just an implementation detail of |
||
| auto expected = ArrayFromJSON(outer_type_dest, R"([ | ||
| {"inner": {"a": 1}}, | ||
| null | ||
| ])"); | ||
| CheckCast(src, expected); | ||
| } | ||
|
|
||
| TEST(Cast, StructNestedNullabilitySliced) { | ||
| auto inner_type_dest = struct_({field("a", int32(), /*nullable=*/false)}); | ||
| auto outer_type_dest = struct_({field("inner", inner_type_dest)}); | ||
| auto inner_type_src = struct_({field("a", int32())}); | ||
| auto outer_type_src = struct_({field("inner", inner_type_src)}); | ||
|
|
||
| auto src = ArrayFromJSON(outer_type_src, R"([ | ||
| {"inner": {"a": 1}}, | ||
| {"inner": {"a": 2}}, | ||
| {"inner": {"a": null}}, | ||
| null, | ||
| {"inner": {"a": 5}} | ||
| ])"); | ||
| auto expected = ArrayFromJSON(outer_type_dest, R"([ | ||
| {"inner": {"a": 1}}, | ||
| {"inner": {"a": 2}}, | ||
| {"inner": {"a": null}}, | ||
| null, | ||
| {"inner": {"a": 5}} | ||
| ])"); | ||
|
|
||
| CheckCast(src->Slice(3, 2), expected->Slice(3, 2)); | ||
|
|
||
| EXPECT_RAISES_WITH_MESSAGE_THAT( | ||
| Invalid, ::testing::HasSubstr("has nulls. Can't cast to non-nullable field"), | ||
| Cast(src->Slice(2, 2), CastOptions::Safe(outer_type_dest))); | ||
| } | ||
|
|
||
| TEST(Cast, StructNestedNullabilityAbsentChild) { | ||
| auto inner_type_dest = struct_({field("a", int32(), /*nullable=*/false)}); | ||
| auto outer_type_dest = struct_({field("inner", inner_type_dest)}); | ||
| auto inner_type_src = struct_({field("a", int32())}); | ||
| auto outer_type_src = struct_({field("inner", inner_type_src)}); | ||
|
|
||
| auto src = ArrayFromJSON(outer_type_src, R"([ | ||
| {"inner": {"a": 1}}, | ||
| {"inner": {"a": 2}} | ||
| ])"); | ||
| auto expected = ArrayFromJSON(outer_type_dest, R"([ | ||
| {"inner": {"a": 1}}, | ||
| {"inner": {"a": 2}} | ||
| ])"); | ||
| CheckCast(src, expected); | ||
| } | ||
|
|
||
| TEST(Cast, StructNestedNullabilityDeep) { | ||
| auto deep_inner_dest = struct_({field("a", int32(), /*nullable=*/false)}); | ||
| auto deep_mid_dest = struct_({field("mid", deep_inner_dest)}); | ||
| auto deep_outer_dest = struct_({field("outer", deep_mid_dest)}); | ||
|
|
||
| auto deep_inner_src = struct_({field("a", int32())}); | ||
| auto deep_mid_src = struct_({field("mid", deep_inner_src)}); | ||
| auto deep_outer_src = struct_({field("outer", deep_mid_src)}); | ||
|
|
||
| auto src = ArrayFromJSON(deep_outer_src, R"([ | ||
| {"outer": {"mid": {"a": 1}}}, | ||
| null | ||
| ])"); | ||
| auto expected = ArrayFromJSON(deep_outer_dest, R"([ | ||
| {"outer": {"mid": {"a": 1}}}, | ||
| null | ||
| ])"); | ||
| CheckCast(src, expected); | ||
| } | ||
|
|
||
| TEST(Cast, StructToSameSizedButDifferentNamedStruct) { | ||
| std::vector<std::string> src_field_names = {"a", "b"}; | ||
| std::shared_ptr<Array> a, b; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
BinaryBitBlockCounterpath below does not account for logical nulls, so I don't think this one should, either.We can just hardcode an error for NullArray.