GH-48210: [C++][Parquet] Fix Bloom Filter logic to enable Parquet DB support on s390x#48211
GH-48210: [C++][Parquet] Fix Bloom Filter logic to enable Parquet DB support on s390x#48211Vishwanatha-HD wants to merge 1 commit into
Conversation
|
|
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
Vishwanatha-HD
force-pushed
the
fixBloomFilters
branch
from
6f6199c to
7dbe358
Compare
kou
changed the title
|
Bloom filters are one of those parts of Parquet where tiny byte-order details end up mattering way more than you’d expect, so it’s good to see attention landing here. Something I ran into on s390x is that the xxhash input/output tends to stay a lot more predictable if the bitset words are kept in a single canonical order (LE in our case) and the reader/writer treat them as such. In my own experiments I normalized the bitset once at the boundary and let the rest of the logic operate on native values. In this patch, the per-word Not calling this out as a problem — the behavior you’re targeting here lines up with what I’ve seen on s390x, especially around making sure the mask checks behave the same across BE/LE hosts. Just sharing observations in case it’s useful while these pieces get polished. |
Vishwanatha-HD
force-pushed
the
fixBloomFilters
branch
from
7dbe358 to
70dd0c1
Compare
|
@pitrou @kou @kiszk @zanmato1984 |
|
Could you rebase on main? |
There was a problem hiding this comment.
Pull request overview
This pull request fixes Parquet Bloom Filter bitset access so Bloom filters are interpreted/stored in little-endian word order, enabling correct behavior on big-endian architectures (notably s390x) while preserving existing behavior on little-endian systems.
Changes:
- Read Bloom filter bitset words using
arrow::bit_util::FromLittleEndian()during lookups. - Write Bloom filter bitset words using a read/modify/write cycle with
FromLittleEndian()/ToLittleEndian()during inserts. - Add the required Arrow endian utilities include.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint32_t* raw_bitset32 = reinterpret_cast<uint32_t*>(data_->mutable_data()); | ||
|
|
||
| for (int i = 0; i < kBitsSetPerBlock; i++) { | ||
| const int word_index = bucket_index * kBitsSetPerBlock + i; |
Vishwanatha-HD
force-pushed
the
fixBloomFilters
branch
from
70dd0c1 to
41e672e
Compare
Hi @kou.. |
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 Bloom Filter logic.
What changes are included in this PR?
The fix includes changes to following file:
cpp/src/parquet/bloom_filter.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