pitrou commented on code in PR #51093:
URL: https://github.com/apache/arrow/pull/51093#discussion_r3892682370
##########
cpp/src/arrow/array/array_nested.cc:
##########
@@ -896,13 +896,19 @@ Status MapArray::ValidateChildData(
if (pair_data->type->id() != Type::STRUCT) {
return Status::Invalid("Map array child array should have struct type");
}
- if (pair_data->MayHaveNulls()) {
+ // Use GetNullCount() rather than MayHaveNulls() so that an all-valid
+ // validity bitmap (null_count == kUnknownNullCount but every bit set)
+ // is not mistakenly treated as containing nulls. MayHaveNulls() returns
+ // true whenever a bitmap is present and null_count != 0, which includes
+ // kUnknownNullCount (-1); GetNullCount() actually counts the bits and
+ // caches the result. See GH-51029.
+ if (pair_data->GetNullCount() != 0) {
Review Comment:
I don't think there's a need for this lengthy comment, let's just remove it?
##########
.gitignore:
##########
@@ -32,6 +32,7 @@ dependency-reduced-pom.xml
MANIFEST
compile_commands.json
build.ninja
+build/
Review Comment:
This change is unrelated, can you revert it?
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -3842,6 +3842,49 @@ TEST(Cast, CastMap) {
Cast(src, dst_type));
}
+// GH-51029: An all-valid validity bitmap with an unknown null count must
+// not be treated as containing nulls when validating MapArray keys.
+TEST(Cast, CastMapWithAllValidKeysBitmap) {
Review Comment:
This is a rather complicated way of exercising this bug.
Why not just set `null_count` to `kUnknownNullCount` directly?
(also then it can be moved to `array_list_test.cc` because it won't depend
on compute anymore)
--
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]
