[C++] Don't assume `!MayHaveNulls()` implies no logical nulls for all types
- 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
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