GH-48206 Fix Statistics logic to enable Parquet DB support on s390x #48207
Conversation
|
|
github-actions
bot
added
Component: Parquet
Component: C++
awaiting review
Vishwanatha-HD
force-pushed
the
fixStatistics
branch
from
69807c0 to
fc2f62c
Compare
| // Fallback: use encoder for other types | ||
| auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, false, descr_, pool_); | ||
| encoder->Put(&src, 1); | ||
| auto buffer = encoder->FlushValues(); | ||
| dst->assign(reinterpret_cast<const char*>(buffer->data()), | ||
| static_cast<size_t>(buffer->size())); |
There was a problem hiding this comment.
Can we reuse the implementation in ARROW_LITTLE_ENDIAN for this?
#if !ARROW_LITTLE_ENDIAN
if constexprt (...) {
...
} else if ... {
...
}
#endif
auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, false, descr_, pool_);
encoder->Put(&src, 1);
auto buffer = encoder->FlushValues();
auto ptr = reinterpret_cast<const char*>(buffer->data());
dst->assign(ptr, static_cast<size_t>(buffer->size()));| if constexpr (std::is_same_v<DType, Int32Type>) { | ||
| uint32_t u; | ||
| std::memcpy(&u, &src, sizeof(u)); | ||
| u = ::arrow::bit_util::ToLittleEndian(u); | ||
| dst->assign(reinterpret_cast<const char*>(&u), sizeof(u)); | ||
| return; | ||
| } else if constexpr (std::is_same_v<DType, Int64Type>) { | ||
| uint64_t u; | ||
| std::memcpy(&u, &src, sizeof(u)); | ||
| u = ::arrow::bit_util::ToLittleEndian(u); | ||
| dst->assign(reinterpret_cast<const char*>(&u), sizeof(u)); | ||
| return; | ||
| } else if constexpr (std::is_same_v<DType, FloatType>) { | ||
| uint32_t u; | ||
| static_assert(sizeof(u) == sizeof(float), "size"); | ||
| std::memcpy(&u, &src, sizeof(u)); | ||
| u = ::arrow::bit_util::ToLittleEndian(u); | ||
| dst->assign(reinterpret_cast<const char*>(&u), sizeof(u)); | ||
| return; | ||
| } else if constexpr (std::is_same_v<DType, DoubleType>) { | ||
| uint64_t u; | ||
| static_assert(sizeof(u) == sizeof(double), "size"); | ||
| std::memcpy(&u, &src, sizeof(u)); | ||
| u = ::arrow::bit_util::ToLittleEndian(u); | ||
| dst->assign(reinterpret_cast<const char*>(&u), sizeof(u)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can we do this in XXXEncoder::Put() instead of here?
|
Statistics tend to surface all sorts of subtle endian quirks, so it’s always interesting to see how different approaches handle those edge cases. Running things on s390x, I’ve found that the most stable behavior usually comes from treating every numeric value—whether it’s a 32-bit int, a float, or the three-limb INT96—as if it should be serialized in LE form no matter what the host is doing. Once everything passes through that single normalization step, the defaults, comparisons, and encoder paths all line up cleanly across architectures. Here, the explicit BE branches for Int32/Int64/Float/Double make the intention clear and should work fine, though it does mean LE and BE end up taking two quite different routes through the code. That can occasionally lead to tiny differences across platforms, especially when stats pages mix types or include INT96 timestamps. Not raising an issue with the logic—just sharing patterns that have helped keep stats round-trips consistent across hosts. |
3 participants
Rationale for this change
This PR is intended to enable Parquet DB support on Big-endian (s390x) systems. The fix in this PR fixes the Statistics logic.
What changes are included in this PR?
The fix includes changes to following file:
cpp/src/parquet/statistics.cc
Are these changes tested?
Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.
Are there any user-facing changes?
No
GitHub main Issue link: #48151