You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AnuragRaut08 (via GitHub)" <gi...@apache.org> on 2026/08/31 06:25:25 UTC
[PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
AnuragRaut08 opened a new pull request, #51093:
URL: https://github.com/apache/arrow/pull/51093
### Rationale for this change
`MapArray` validation uses `MayHaveNulls()` to check that the map child and keys contain no nulls. When an all-valid validity bitmap is present with an unknown null count, `MayHaveNulls()` can report that the array may contain nulls even though all values are valid.
During a cast of a `MapArray`, this can cause validation to return an invalid status which is passed to `ARROW_CHECK_OK`, resulting in a process abort instead of a recoverable error.
### What changes are included in this PR?
- Use `GetNullCount()` instead of `MayHaveNulls()` when validating the `MapArray` child and keys.
- Add a regression test covering an all-valid validity bitmap with `kUnknownNullCount`.
- Cover both the `filter()` → `cast()` path and an explicitly constructed all-valid keys bitmap.
### Are these changes tested?
Yes.
Added and passed:
- `Cast.CastMapWithAllValidKeysBitmap`
- Existing `Cast.CastMap` test
The complete `arrow-array-test` suite was also run successfully with 1060 tests passed and 1 skipped.
### Are there any user-facing changes?
Yes. Casting affected `MapArray` values no longer aborts the process when the map keys have an all-valid validity bitmap with an unknown null count.
### This PR contains a "Critical Fix".
This fixes a bug that can cause a process crash when casting a valid `MapArray` whose keys carry an all-valid validity bitmap with an unknown null count.
Fixes GH-51029.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #51093:
URL: https://github.com/apache/arrow/pull/51093
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "AnuragRaut08 (via GitHub)" <gi...@apache.org>.
AnuragRaut08 commented on PR #51093:
URL: https://github.com/apache/arrow/pull/51093#issuecomment-5496631003
Done, ran clang-format on the affected C++ files with pre-commit.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #51093:
URL: https://github.com/apache/arrow/pull/51093#discussion_r3914183100
##########
.gitignore:
##########
@@ -32,6 +32,7 @@ dependency-reduced-pom.xml
MANIFEST
compile_commands.json
build.ninja
+build/
Review Comment:
```suggestion
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "AnuragRaut08 (via GitHub)" <gi...@apache.org>.
AnuragRaut08 commented on code in PR #51093:
URL: https://github.com/apache/arrow/pull/51093#discussion_r3905873729
##########
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:
Done, removed the lengthy 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #51093:
URL: https://github.com/apache/arrow/pull/51093#issuecomment-5496707523
> @pitrou I checked the other MayHaveNulls() uses in array_nested.cc. They are used for validating nullable list/map offsets when constructing arrays, rather than validating MapArray keys, so I left them unchanged.
But the same issue applies: `MayHaveNulls` does not tell whether there are really nulls. So we should strengthen those checks as well.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "AnuragRaut08 (via GitHub)" <gi...@apache.org>.
AnuragRaut08 commented on code in PR #51093:
URL: https://github.com/apache/arrow/pull/51093#discussion_r3905878294
##########
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:
Done. I simplified the regression test by setting the null count to kUnknownNullCount directly and moved it to array_list_test.cc.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "AnuragRaut08 (via GitHub)" <gi...@apache.org>.
AnuragRaut08 commented on PR #51093:
URL: https://github.com/apache/arrow/pull/51093#issuecomment-5496995389
Thanks, addressed. I have updated the remaining `MayHaveNulls()` checks in `array_nested.cc` to use `GetNullCount() != 0` and added regression coverage for an all-valid offsets bitmap with `kUnknownNullCount`. The `TestMapArray.*` tests pass.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "AnuragRaut08 (via GitHub)" <gi...@apache.org>.
AnuragRaut08 commented on code in PR #51093:
URL: https://github.com/apache/arrow/pull/51093#discussion_r3905881068
##########
.gitignore:
##########
@@ -32,6 +32,7 @@ dependency-reduced-pom.xml
MANIFEST
compile_commands.json
build.ninja
+build/
Review Comment:
Done, reverted the unrelated .gitignore change.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #51093:
URL: https://github.com/apache/arrow/pull/51093#issuecomment-5510226578
@AlenkaF FYI
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #51093:
URL: https://github.com/apache/arrow/pull/51093#issuecomment-5474586737
:warning: GitHub issue #51029 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51029: [C++] Fix MapArray validation with unknown null count [arrow]
Posted by "AnuragRaut08 (via GitHub)" <gi...@apache.org>.
AnuragRaut08 commented on PR #51093:
URL: https://github.com/apache/arrow/pull/51093#issuecomment-5496652587
@pitrou I checked the other MayHaveNulls() uses in array_nested.cc. They are used for validating nullable list/map offsets when constructing arrays, rather than validating MapArray keys, so I left them unchanged.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org