apache / apache/arrow

[C++] Don't assume `!MayHaveNulls()` implies no logical nulls for all types

Open
#35,207 0 comments 0 reactions 0 assignees View on GitHub
Component: C++ Type: bug
Dominant language
C++
Stars
17.1k
Forks
4.3k
Avg merge
3d 18h
Merged PRs (30d)
91

Description

### Describe the bug, including details regarding any error messages, version, and platform.

The assumption that an array without a validity bitmap has no logical nulls breaks for some types that are not `NA` `RUN_END_ENCODED`, `DENSE_UNION`, `SPARSE_UNION`.

*We need to review usages of `MayHaveNulls` case by case and use more focused functions -- `HasValidityBitmap` and/or `MayHaveLogicalNulls`.*

The docstring in `ArrayData::MayHaveLogicalNulls` describes the situation:

```cpp
/// \brief Return true if the validity bitmap may have 0's in it, or if the
/// child arrays (in the case of types without a validity bitmap) may have
/// nulls
///
/// This is not a drop-in replacement for MayHaveNulls, as historically
/// MayHaveNulls() has been used to check for the presence of a validity
/// bitmap that needs to be checked.
///
/// Code that previously used MayHaveNulls() and then dealt with the validity
/// bitmap directly can be fixed to handle all types correctly without
/// performance degradation when handling most types by adopting
/// HasValidityBitmap and MayHaveLogicalNulls.
///
/// Before:
///
/// uint8_t* validity = array.MayHaveNulls() ? array.buffers[0].data : NULLPTR;
/// for (int64_t i = 0; i < array.length; ++i) {
/// if (validity && !bit_util::GetBit(validity, i)) {
/// continue; // skip a NULL
/// }
/// ...
/// }
///
/// After:
///
/// bool all_valid = !array.MayHaveLogicalNulls();
/// uint8_t* validity = array.HasValidityBitmap() ? array.buffers[0].data : NULLPTR;
/// for (int64_t i = 0; i < array.length; ++i) {
/// bool is_valid = all_valid ||
/// (validity && bit_util::GetBit(validity, i)) ||
/// array.IsValid(i);
/// if (!is_valid) {
/// continue; // skip a NULL
/// }
/// ...
/// }
bool MayHaveLogicalNulls() const {
if (buffers[0] != NULLPTR) {
return null_count.load() != 0;
}
const auto t = type->id();
if (t == Type::SPARSE_UNION || t == Type::DENSE_UNION) {
return internal::UnionMayHaveLogicalNulls(*this);
}
if (t == Type::RUN_END_ENCODED) {
return internal::RunEndEncodedMayHaveLogicalNulls(*this);
}
return null_count.load() != 0;
}
```

List of specific issues:

- [x] #35059
- [ ] ...

### Component(s)

C++

Contributor guide

Open the contributing guide

Research direction

Start by reviewing the C++ usages of ArrayData::MayHaveNulls described in the issue, comparing them with HasValidityBitmap and MayHaveLogicalNulls. Check each usage against the listed union and run-end-encoded cases; done means the affected checks handle logical nulls without incorrectly assuming that a missing validity bitmap means all values are valid.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
data-engineering
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.