GH-47995: [C++][Parquet] Fix empty string min/max statistics being lost during merge #48717
+260
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing lost during merge Prior to this change, CleanStatistic() for ByteArray rejected statistics where ptr == nullptr. However, empty strings can have ptr == nullptr with len == 0, causing valid statistics to be discarded when the minimum value is an empty string. The fix introduces a sentinel pointer (kNoValueSentinel) distinct from nullptr to mark "no value" in ByteArray statistics. This allows CleanStatistic to distinguish between "no min/max computed" (sentinel) and "min/max is empty string" (nullptr with len=0). FLBA is unchanged since it has fixed length and no "empty" concept.
|
|
github-actions
bot
added
Component: Parquet
Component: C++
awaiting review
Member
wgtmac
reviewed
Jan 6, 2026
| static T DefaultMax() { return T{0, kNoValueSentinel}; } | ||
|
|
||
| static T Min(int type_length, const T& a, const T& b) { | ||
| if (a.ptr == kNoValueSentinel) return b; |
Member
There was a problem hiding this comment.
Not related to this PR. Since it allows ptr to be nullptr for empty string, following Copy may be a UB if min is an empty string with nullptr.
template <>
inline void TypedStatisticsImpl<ByteArrayType>::Copy(const ByteArray& src, ByteArray* dst,
ResizableBuffer* buffer) {
if (dst->ptr == src.ptr) return;
PARQUET_THROW_NOT_OK(buffer->Resize(src.len, false));
std::memcpy(buffer->mutable_data(), src.ptr, src.len);
*dst = ByteArray(src.len, buffer->data());
}
Author
There was a problem hiding this comment.
good point, I will make a separate PR to fix that
This was referenced Jan 6, 2026
HuaHuaY
approved these changes
Jan 7, 2026
github-actions
bot
added
awaiting committer review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Fixes #47995
When merging ByteArray statistics, empty string min/max values were incorrectly discarded. This happened because
CleanStatistic()rejected statistics whereptr == nullptr, but empty strings can legitimately haveptr == nullptrwithlen == 0.What changes are included in this PR?
Introduces a sentinel pointer (
kNoValueSentinel) distinct fromnullptrto mark "no value" in ByteArray statistics. This allowsCleanStatisticto distinguish between:FLBA is unchanged since it has fixed length and no "empty" concept.
Are these changes tested?
Yes. Added comprehensive tests covering all combinations of:
Are there any user-facing changes?
No API changes. This is a bug fix that preserves empty string statistics correctly during merge operations.