Skip to content

GH-50007: [C++][Parquet] Add bloom filter folding to automatically size SBBF filters - #50008

Merged
pitrou merged 7 commits into
apache:mainfrom
HuaHuaY:sbbf_filters
Jun 11, 2026
Merged

GH-50007: [C++][Parquet] Add bloom filter folding to automatically size SBBF filters#50008
pitrou merged 7 commits into
apache:mainfrom
HuaHuaY:sbbf_filters

Conversation

@HuaHuaY

@HuaHuaYHuaHuaY commented May 21, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

This PR follows apache/arrow-rs#9628. It supports optimizing the disk usage of the Bloom filter. So specifying an ndv value larger than the actual value will not affect disk usage.

Bloom filters now support folding mode: allocate a conservatively large filter (sized for worst-case NDV), insert all values during writing, then fold down at flush time to meet a target FPP. This eliminates the need to guess NDV upfront and produces optimally-sized filters automatically.

What changes are included in this PR?

BloomFilterBuilder will try to fold the bloom filter before writing it to the output stream.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

The type of ndv in BloomFilterOptions is changed from int32_t to std::optional<int64_t>. And the argument type of OptimalNumOfBytes and OptimalNumOfBits in BlockSplitBloomFilter is changed from uint32_t ndv to uint64_t ndv.

Add a new field fold in BloomFilterOptions and default value is true.

@HuaHuaY
HuaHuaY requested a review from wgtmac as a code ownerMay 21, 2026 10:19
@github-actionsgithub-actionsBot added the awaiting review Awaiting review label May 21, 2026
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50007has been automatically assigned in GitHub to PR creator.

Comment threadcpp/src/parquet/bloom_filter_writer.cc Outdated
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 21, 2026
@HuaHuaY

Copy link
Copy Markdown
ContributorAuthor

@wgtmac@alamb@etseidl@emkornfield Please take a look.

@alamb

Copy link
Copy Markdown
Contributor

I am not likely to have time to review C++ code in the arrow repository unfortunately

@wgtmacwgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HuaHuaY for adding this quickly!

Comment threadcpp/src/parquet/properties.h
Comment threadcpp/src/parquet/properties.h Outdated
std::to_string(bloom_filter_options.fpp));
}
if (bloom_filter_options.ndv.has_value() && bloom_filter_options.ndv.value() < 0) {
throw ParquetException("Bloom filter number of distinct values must be >= 0, got " +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected behavior of 0?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will create a smallest bloom filter.

Comment threadcpp/src/parquet/bloom_filter.h
Comment threadcpp/src/parquet/bloom_filter.h Outdated
Comment threadcpp/src/parquet/bloom_filter.cc Outdated
Comment threadcpp/src/parquet/bloom_filter.cc
Comment threadcpp/src/parquet/bloom_filter_reader_writer_test.cc Outdated
@wgtmac

Copy link
Copy Markdown
Member

cc @mapleFU@adamreeve

@wgtmacwgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. I left some nits.

Comment threadcpp/src/parquet/properties.h Outdated
Comment threadcpp/src/parquet/bloom_filter.cc Outdated
Comment threadcpp/src/parquet/properties.h

@mapleFUmapleFU left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM

Comment threadcpp/src/parquet/bloom_filter_writer.cc Outdated
Comment threadcpp/src/parquet/bloom_filter.cc Outdated
@HuaHuaY

Copy link
Copy Markdown
ContributorAuthor

@pitrou@mapleFU Please take a look.

Comment threadcpp/src/parquet/bloom_filter.cc Outdated
Comment threadcpp/src/parquet/bloom_filter.cc
Comment threadcpp/src/parquet/bloom_filter.cc
Comment threadcpp/src/parquet/bloom_filter.cc
Comment threadcpp/src/parquet/bloom_filter_writer.cc Outdated
Comment threadcpp/src/parquet/bloom_filter_reader_writer_test.cc
Comment threadcpp/src/parquet/bloom_filter.cc Outdated
(static_cast<double>(num_blocks) * kBytesPerFilterBlock * 8);
const auto max_folds = static_cast<uint32_t>(std::countr_zero(num_blocks));

if (avg_fill == 0.0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I little bit forgot would this really happens when writing a parquet file?

@HuaHuaYHuaHuaYJun 4, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all values in a column chunk are null, avg_fill will be 0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it still need a BF or fold in this scenerio? Or this path would lead to zero cost folding?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are differences between "not have a bloom filter" and "bloom filter has no values". The latter can filter every not null values. And there is currently no way to indicate that a bloom filter exists but has no value through metadata.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about
(1) without folding, just replace to a smallest one without any copying

And there is currently no way to indicate that a bloom filter exists but has no value through metadata.

In theory you're right. In production, I believe null_count == num_values works?

@HuaHuaYHuaHuaYJun 4, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without folding, just replace to a smallest one without any copying

Good idea. I will change the code soon.

I believe null_count == num_values works

I don't think it's good to mix two separate components. Also, null_count is an optional value and may not actually exist.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the commits and now only fold when total_set_bits is not equal to 0.

Comment threadcpp/src/parquet/bloom_filter.cc Outdated
const auto* bitset32 = reinterpret_cast<const uint32_t*>(data_->data());
const uint32_t num_words = num_bytes_ / static_cast<uint32_t>(sizeof(uint32_t));
for (uint32_t i = 0; i < num_words; ++i) {
total_set_bits += static_cast<uint64_t>(std::popcount(bitset32[i]));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether internal::CountSetBits easy to understand here ( though popcount is right and a bit faster)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changd to internal::CountSetBits. internal::CountSetBits may be faster because it counts once every 64 bits.

Comment threadcpp/src/parquet/bloom_filter_reader_writer_test.cc
@HuaHuaY
HuaHuaYforce-pushed the sbbf_filters branch 2 times, most recently from d26cb2e to d57601bCompareJune 4, 2026 09:34
@HuaHuaY

Copy link
Copy Markdown
ContributorAuthor

@pitrou Please take a look again.

Comment threadcpp/src/parquet/bloom_filter_reader_writer_test.cc Outdated
Comment threadcpp/src/parquet/bloom_filter.cc
Comment threadcpp/src/parquet/bloom_filter_reader_writer_test.cc
@HuaHuaY
HuaHuaY requested a review from pitrouJune 11, 2026 07:45

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @HuaHuaY !

@pitrou
pitrou merged commit 108240f into apache:mainJun 11, 2026
52 checks passed
@pitroupitrou removed the awaiting committer review Awaiting committer review label Jun 11, 2026
@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Jun 11, 2026
@HuaHuaY
HuaHuaY deleted the sbbf_filters branch June 11, 2026 08:42
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 108240f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

pitrou pushed a commit that referenced this pull request Jun 18, 2026
…als-test on test-conda-cpp-valgrind (#50213)
### Rationale for this change
Fix#50212 `Test #89: parquet-internals-test .......................***Failed 14.96 sec`
### What changes are included in this PR?
Add custom `PrintTo` for parameter `BloomFilterBuilderFoldingTestCase` introduced with pr #50008, so gtest prints fields instead of raw bytes (error `Use of uninitialised value`).
Similar to the past solution in commit 1b7e396 but with additional field names/debug output.
### Are these changes tested?
Yes, by CI.
### Are there any user-facing changes?
No.
* GitHub Issue: #50212
Authored-by: Tadeja Kadunc <tadeja.kadunc@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@HuaHuaY@alamb@wgtmac@pitrou@mapleFU