bkietz commented on a change in pull request #10871:
URL: https://github.com/apache/arrow/pull/10871#discussion_r685360313
##########
File path: cpp/src/arrow/array/array_test.cc
##########
@@ -407,6 +407,35 @@ TEST_F(TestArray, TestMakeArrayOfNullUnion) {
}
}
+TEST_F(TestArray, TestValidateNullCount) {
+ Int32Builder builder(pool_);
+ ASSERT_OK(builder.Append(5));
+ ASSERT_OK(builder.Append(42));
+ ASSERT_OK(builder.AppendNull());
+ ASSERT_OK_AND_ASSIGN(auto array, builder.Finish());
+
+ ArrayData* data = array->data().get();
+ data->null_count = -1;
Review comment:
also here:
```suggestion
data->null_count = kUnknownNullCount;
```
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -637,6 +638,23 @@ struct ValidateArrayFullImpl {
ARROW_EXPORT
Status ValidateArrayFull(const ArrayData& data) {
+ if (data.null_count != -1) {
+ int64_t actual_null_count;
+ if (HasValidityBitmap(data.type->id()) && data.buffers[0]) {
Review comment:
I notice ValidateArray doesn't check for `data.type == nullptr`, is that
worth adding in this PR?
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -637,6 +638,23 @@ struct ValidateArrayFullImpl {
ARROW_EXPORT
Status ValidateArrayFull(const ArrayData& data) {
+ if (data.null_count != -1) {
+ 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->id() == Type::NA) {
+ actual_null_count = data.length;
Review comment:
This is only enforced by `arrow::NullArray::SetData`; it's possible that
`data.null_count != data.length` which should be an error for `Type::NA`. Going
a step further, we might want to just always require that `data.null_count ==
data.length` for `Type::NA` instead of also allowing `kUnknownNullCount`
##########
File path: cpp/src/arrow/array/validate.cc
##########
@@ -637,6 +638,23 @@ struct ValidateArrayFullImpl {
ARROW_EXPORT
Status ValidateArrayFull(const ArrayData& data) {
+ if (data.null_count != -1) {
Review comment:
Nit:
```suggestion
if (data.null_count != kUnknownNullCount) {
```
--
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]
