GH-41114: [C++] Add is_validity_defined_by_bitmap() predicate

[felipecrv]

Rationale for this change

To make it easier to find bugs that are very likely to be silent in the codebas because users rarely use unions and REE types.

What changes are included in this PR?

Adding the type predicate and two usages in builder_nested.h.

Are these changes tested?

By the compilation process, since they are both static_asserts.

• GitHub Issue: [C++] A way to assert that arrays of a certain type have the validity of the values defined by the validity bitmap #41114

[github-actions]

⚠️ GitHub issue #41114 has been automatically assigned in GitHub to PR creator.

[pitrou]

Is this conceptually different from internal::HasValidityBitmap?

[felipecrv]

Is this conceptually different from internal::HasValidityBitmap?

The definition is the same, but the naming is unclear and ambiguous:

internal::HasValidityBitmap(type) = true iff array instances of this type might have bitmap buffers
Array(Data|Span)?::HasValidityBitmap() = true iff this specific array instance has a bitmap buffer

This reads as if the array.HasValidityBitmap() check is redundant:

static_assert(internal::HasValidityBitmap(ArrayType::type_id));
const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;

vs

static_assert(internal::is_validity_defined_by_bitmap(ArrayType::type_id));
const uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;

[mapleFU]

+1 for this, so it replace MayHaveLogicalNulls() and validity buffer check to just reject the type like "NA", and just check the validity buffer when it might contains physical null?

[felipecrv]

+1 for this, so it replace MayHaveLogicalNulls() and validity buffer check to just reject the type like "NA", and just check the validity buffer when it might contains physical null?

In that case, it's better to call MayHaveLogicalNulls() directly. But if you're writing kernels to a subset of types and want to have a local guarantee that accessing the bitmap is enough, you use this check, access the bitmap directly, and never even call IsValid, MayHaveLogicalNulls etc.

[mapleFU]

+1 for me

[pitrou]

The definition is the same, but the naming is unclear and ambiguous:

Regardless of the name, we shouldn't have two functions doing the same thing. So at least this should be renaming the existing function (but I find your proposition rather wordy).

[felipecrv]

The definition is the same, but the naming is unclear and ambiguous:

Regardless of the name, we shouldn't have two functions doing the same thing.

They could evolve in different ways in the future with the addition of new types. It's a coincidence that the intention of both matches today's implementations.

So at least this should be renaming the existing function (but I find your proposition rather wordy).

I also think it's a long name, but I couldn't come up with a shorter one, do you have a name suggestion?

[pitrou]

They could evolve in different ways in the future with the addition of new types.

How come? Nobody has ever suggested a different validity representation than the validity bitmap.

I also think it's a long name, but I couldn't come up with a shorter one, do you have a name suggestion?

I would suggest can_have_validity_bitmap or may_have_validity_bitmap.

[felipecrv]

I'm going to merge this based on the +1 from @mapleFU and the fact that I acted on all the feedback posted by @pitrou.

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit a78760f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 31 possible false positives for unstable benchmarks that are known to sometimes produce them.