pitrou commented on code in PR #50546:
URL: https://github.com/apache/arrow/pull/50546#discussion_r3658281256
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -4145,6 +4145,128 @@ 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)));
+}
+
+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
+ ])");
Review Comment:
This does not guarantee that the inner field will have nulls at all, it's
just an implementation detail of `ArrayFromJSON`. Can you instead use
`StructArray::Make` to construct the outer array from its inner child array?
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -384,17 +384,58 @@ 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());
+ const uint8_t* parent_bitmap = in_array.buffers[0].data;
+ const uint8_t* child_bitmap =
+ in_values->buffers.empty() || !in_values->buffers[0]
+ ? nullptr
+ : in_values->buffers[0]->data();
+
+ int64_t unmasked_null_count;
+ if (parent_bitmap == nullptr) {
+ // Parent has no nulls, so any child null is unmasked.
+ unmasked_null_count = in_values->GetNullCount();
+ } else if (child_bitmap == nullptr) {
+ // Child reports nulls but has no validity buffer of its own (e.g.
+ // NullArray, RunEndEncoded, Union). There is no cheap,
bitmap-only way
Review Comment:
This will only apply to NullArray. RunEndEncoded and Union should return 0
for `GetNullCount` (which should not be confused with
`ComputeLogicalNullCount`).
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -384,17 +384,58 @@ 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());
+ const uint8_t* parent_bitmap = in_array.buffers[0].data;
+ const uint8_t* child_bitmap =
+ in_values->buffers.empty() || !in_values->buffers[0]
+ ? nullptr
+ : in_values->buffers[0]->data();
+
+ int64_t unmasked_null_count;
+ if (parent_bitmap == nullptr) {
+ // Parent has no nulls, so any child null is unmasked.
+ unmasked_null_count = in_values->GetNullCount();
+ } else if (child_bitmap == nullptr) {
+ // Child reports nulls but has no validity buffer of its own (e.g.
+ // NullArray, RunEndEncoded, Union). There is no cheap,
bitmap-only way
+ // to tell which of those nulls are masked by the parent, so
+ // conservatively reject rather than reason about logical nulls
(which
+ // would be inconsistent with the bitmap-only check below).
+ unmasked_null_count = in_values->GetNullCount();
+ } else {
+ // Both parent and child have bitmaps: an unmasked null is a
position
+ // where the parent is valid but the child is null, i.e. parent
bits
+ // set that aren't also set in the child.
+ unmasked_null_count = arrow::internal::CountSetBits(
+ parent_bitmap, in_array.offset,
in_array.length) -
+ arrow::internal::CountAndSetBits(
+ parent_bitmap, in_array.offset,
child_bitmap,
+ in_values->offset, in_array.length);
Review Comment:
Can we instead add a function `CountAndNotSetBits` in
`arrow/util/bitmap_ops.h` and use it directly here?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -4145,6 +4145,128 @@ 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)));
+}
+
+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
+ ])");
+ 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, StructNestedNullabilityNoChildNulls) {
+ 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, StructNestedNullabilityNoChildBitmapConservativelyRejected) {
+ // NullType (and other child types without a validity buffer of their own,
+ // e.g. RunEndEncoded/Union) have no bitmap to distinguish masked from
+ // unmasked nulls, so any reported null is conservatively rejected -- even
+ // when the parent row is itself null and the value would otherwise be
+ // masked.
+ auto inner_type_dest = struct_({field("a", null(), /*nullable=*/false)});
+ auto outer_type_dest = struct_({field("inner", inner_type_dest)});
+ auto inner_type_src = struct_({field("a", null())});
+ auto outer_type_src = struct_({field("inner", inner_type_src)});
+
+ auto src = ArrayFromJSON(outer_type_src, R"([
+ {"inner": {"a": null}},
+ 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)));
+
+ // Slicing to only the masked (fully-null outer) row is still rejected.
+ EXPECT_RAISES_WITH_MESSAGE_THAT(
+ Invalid, ::testing::HasSubstr("has nulls. Can't cast to non-nullable
field"),
+ Cast(src->Slice(1, 1), CastOptions::Safe(outer_type_dest)));
+}
Review Comment:
Can you also check with a zero-length input? (not sure whether it should
succeed or fail)
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -4145,6 +4145,128 @@ 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)));
Review Comment:
Can you also check with a zero-length input?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -4145,6 +4145,128 @@ 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)));
+}
+
+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
+ ])");
+ 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, StructNestedNullabilityNoChildNulls) {
+ 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, StructNestedNullabilityNoChildBitmapConservativelyRejected) {
+ // NullType (and other child types without a validity buffer of their own,
+ // e.g. RunEndEncoded/Union) have no bitmap to distinguish masked from
+ // unmasked nulls, so any reported null is conservatively rejected -- even
Review Comment:
I don't think REE or Union would be rejected, can you fix the comment?
--
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]
