GH-51029: [C++] Fix MapArray validation with unknown null count - #51093
GH-51029: [C++] Fix MapArray validation with unknown null count#51093AnuragRaut08 wants to merge 2 commits into
Conversation
|
|
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for doing this @AnuragRaut08 ! See comments below.
Also, there are other uses of MayHaveNulls in array_nested.cc, shouldn't we fix them as well?
| 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) { |
There was a problem hiding this comment.
I don't think there's a need for this lengthy comment, let's just remove it?
| MANIFEST | ||
| compile_commands.json | ||
| build.ninja | ||
| build/ |
There was a problem hiding this comment.
This change is unrelated, can you revert it?
| // 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) { |
There was a problem hiding this 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)
github-actions
Bot
added
awaiting committer review
Reranko05
left a comment
Reranko05
left a comment
There was a problem hiding this comment.
Also fix the lint by using:-
python -m pre_commit run clang-format --files <file_path>
3 participants
Rationale for this change
MapArrayvalidation usesMayHaveNulls()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 toARROW_CHECK_OK, resulting in a process abort instead of a recoverable error.What changes are included in this PR?
GetNullCount()instead ofMayHaveNulls()when validating theMapArraychild and keys.kUnknownNullCount.filter()→cast()path and an explicitly constructed all-valid keys bitmap.Are these changes tested?
Yes.
Added and passed:
Cast.CastMapWithAllValidKeysBitmapCast.CastMaptestThe complete
arrow-array-testsuite was also run successfully with 1060 tests passed and 1 skipped.Are there any user-facing changes?
Yes. Casting affected
MapArrayvalues 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
MapArraywhose keys carry an all-valid validity bitmap with an unknown null count.Fixes GH-51029.