lidavidm commented on a change in pull request #12706:
URL: https://github.com/apache/arrow/pull/12706#discussion_r838951164
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -285,7 +286,7 @@ struct ValidateArrayImpl {
const auto& field_data = *data.child_data[i];
Review comment:
Line 275 above (FixedSizeListType) could also use validation (in the
`RecurseInto` call)
##########
File path: cpp/src/arrow/array/array_struct_test.cc
##########
@@ -196,6 +196,22 @@ TEST(StructArray, Validate) {
ASSERT_RAISES(Invalid, arr->ValidateFull());
}
+TEST(StructArray, ValidateFullNullable) {
+ auto type = struct_({field("a", int32(), /*nullable =*/false),
+ field("b", utf8(), /*nullable =*/false),
+ field("c", list(boolean()), /*nullable =*/false)});
+
+ auto struct_arr = std::static_pointer_cast<StructArray>(ArrayFromJSON(
+ type, R"([[1, "a", [null, false]], [null, "bc", []], [2, null,
null]])"));
+
+ auto struct_arr_nonull = std::static_pointer_cast<StructArray>(ArrayFromJSON(
+ type, R"([[1, "a", [true, false]], [6, "bc", []], [2, "bcj", [true,
true]]])"));
Review comment:
Note that there's no need to cast the result, since ValidateFull is a
method of the base class.
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -509,22 +510,23 @@ struct ValidateArrayImpl {
}
if (full_validation) {
- if (data.null_count != kUnknownNullCount) {
- int64_t actual_null_count;
- if (HasValidityBitmap(data.type->id()) && data.buffers[0]) {
- // Do not call GetNullCount() as it would also set the `null_count`
member
- actual_null_count = data.length -
CountSetBits(data.buffers[0]->data(),
- data.offset,
data.length);
- } else if (data.type->storage_id() == Type::NA) {
- actual_null_count = data.length;
- } else {
- actual_null_count = 0;
- }
- if (actual_null_count != data.null_count) {
- return Status::Invalid("null_count value (", data.null_count,
- ") doesn't match actual number of nulls in
array (",
- actual_null_count, ")");
- }
+ int64_t actual_null_count;
+ if (HasValidityBitmap(data.type->id()) && data.buffers[0]) {
+ // Do not call GetNullCount() as it would also set the `null_count`
member
+ actual_null_count =
+ data.length - CountSetBits(data.buffers[0]->data(), data.offset,
data.length);
+ } else if (data.type->storage_id() == Type::NA) {
+ actual_null_count = data.length;
+ } else {
+ actual_null_count = 0;
+ }
+ if (data.null_count != kUnknownNullCount && actual_null_count !=
data.null_count) {
+ return Status::Invalid("null_count value (", data.null_count,
+ ") doesn't match actual number of nulls in
array (",
+ actual_null_count, ")");
+ }
+ if (!nullable && actual_null_count > 0) {
+ return Status::Invalid("Null found but field is not nullable");
}
}
return Status::OK();
Review comment:
Also, line 585 below (in ValidateListLike) has a RecurseInto call that
should be updated
--
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]
