Bloom filter implementation

[joacoc]

Rationale for this change

This is part of a bigger issue around skip indexes but only focuses on the Bloom filter implementation:

• Tracking Issue: Skipping Indexes on ZonedLayout #8901

What changes are included in this PR?

This is a continuation of a previous PR that already implements a simpler version of a bloom filter: #8933.

Most of the changes included in this PR are replacing the hand-rolled bloom-filter from the base PR with the split block bloom filter, and expanding support for more canonicals and constants. So far it supports bool, primitives, decimals, varbinview, and extensions, and this PR leaves out composable types out of the implementation (struct, list and so on).

About the code

The Bloom filter implementation is a translation from the paper source code, given that it is very short I thought it wouldn't be a big deal. The code doesn't use arch-y fn calls, but relies on the compiler optimizer for vectorizing/calling the correct assembler expression, which from my checks on godbolt, seems to get translated well. I could have gone into specializing the code into different architectures but keeping it as is seemed better for maintenance.

Accumulators

Bloom filter's accumulators for canonicals are very similar to those of the min/max aggregation fn. I used those implementations as a reference for manipulating each case, with some slight variations.

Invalid values are left out of Bloom filters. Only valid values apply. Accumulating an invalid value will just skip it, but asking for a scalar that is invalid will raise a VortexError.

Options/tuning

The Bloom filter has a single tuning option called blocks_count. The more blocks, the more memory/space usage (per zone), but the fewer false positives. Defaults to 256 (8KiB p/zone), derived from the default amount of 8192 rows in a zone. The paper suggests keeping between 20 and 52 distinct hash values per block. Assuming all 8,192 values are distinct, choosing 32 values per block gives 8,192 / 32 = 256 blocks (low false positive rate: ~3%).

Another approach could be to use statistics from a subset of the data, as is done with encoders, to select the best block count. I think this approach aligns with how Vortex encoders work, but it shouldn't be part of this PR, as it may depend on how the skip index writer is designed.

Hasher crate

This PR also includes a new crate twox-hash that implements XxHash, similar to how Parquet uses the same hasher for the same use case, but it relies on vendored code. Happy to change or look for alternatives.

Pending tasks

• Microbenchmarks
• Goldenfile
• Salt-order check
• Define hashing fn (XxHash3_64 vs XXH64)
• More docs in source

[joacoc]

The current implementation has no benchmarks, and I saw divan used in the encoders. It would be great to have some, at least for BloomPartial, but I'd leave that for a follow-up PR (if that's ok).

[connortsui20]

A few things on this first pass that I saw, I haven't actually grokked the algorithm myself either so in the next round I'll review the actual filter algorithm.

I was also doing some digging into how this works, and I believe that your impl follows the parquet rust impl, but that's actually different from the paper slightly? Maybe it is fine. I will also eventually start looking at the autovectorization and LLVM IR to verify if the compiler is doing it's job (because sometimes it just doesn't)

Also, please fix the lints and other CI issues. Feel free to ping me so I can approve the workflows to run.

[connortsui20]

We probably also want to have a goldenfile for this? To make sure we don't break things accidentally in the future

[joacoc]

Thanks for the quick review Connor.

I was also doing some digging into how this works, and I believe that your impl follows the parquet rust impl, but that's actually different from the paper slightly? Maybe it is fine. I will also eventually start looking at the autovectorization and LLVM IR to verify if the compiler is doing it's job (because sometimes it just doesn't)

At first, I translated the implementation from the paper into AVX2 and NEON. Then I switched to a single portable implementation (Codex convinced me), and I wouldn’t be surprised if it led to an implementation similar to the Parquet Rust impl.

Next, I compared both implementations on Godbolt (this link contains both implementations), and I was happy with the result.

[connortsui20]

@joacoc that makes sense. Do you know if this vectorizes for avx512 as well?

Regardless I think we want to have some sort of goldenfile that we can use for backcompat checks. Not sure what the best way to do that is...

In the meantime could you resolve the merge conflicts and other comments I had? Then once it's cleaned up I can do a deeper review

[joacoc]

Just checked, and avx512 vectorizes ok. It uses the same instructions as AVX2. The implementation targets 256-bit vectors, so that makes sense.

I agree about the golden-file test. The current smoke test is clearly not enough. I’ll come up with something. The filter can already serialize to bytes, so it should be ok.

I’ll update the PR details so we can track everything there.

[joseph-isaacs]

Can you add a comment of the stability of the bloom filter w.r.t. serialisation.

Do you see us using other hash function or serialisation of the buckets going forwards?

Should we add somewhere in the ID or Options the format of the bloom filter?

[joacoc]

@joseph-isaacs Yes. I see the serialization of the buckets to stay stable. If we change the bucket layout itself, I think that should be a new filter type, rather than an evolution of this one.

On the other hand, changing the hash function seems much more plausible. I think making the hash function an option with an id, and storing that id in the serialized options/metadata, would make the filter more flexible and future-proof. Does that make sense?

[joseph-isaacs]

Can we also include further docs explaining how this work in source with a link to more detail if needed.

I see the serialization of the buckets to stay stable.
True for sbbf bloom filters.

I think making the hash function an option.
Please do this, lets have a golden file for each hash (if not already done).

What about the numbers of hash functions k and seed. Are these fix or should we include these?

[joacoc]

Can we also include further docs explaining how this work in source with a link to more detail if needed.

Yep. I’ll add that to the pending tasks.

What about the numbers of hash functions k and seed. Are these fix or should we include these?

If hash functions k means let rehash: [u32; 8] = [ 0x47b6137b, ... ], then those are fixed and should stay as is (the length is to fit all lanes).

The hash seed is an interesting case, but after thinking about it, I would keep it fixed. I don't have a clear concrete case (just speculations) that would justify a custom seed for this implementation, or why a future hash function would make it a more important parameter that a user would need to change it. Even though the implementation and the seed storage size would be really small.

The format I barely tried out is as follows:

pub struct BloomOptions {
    blocks_count: NonZeroU32,
    hash_fn: HashFn
}

enum HashFn {
    XxHash3_64, // ID = ...
    XxHash64,   // ID = ...
    ...
}

[joacoc]

So, added configurable hashing and would love to highlight two things:

1. I would love to assert inside combine_partials(&self, partial: &mut Self::Partial, other: Scalar) which options, particularly the hash function, were used in other. Given that it is just a Scalar, I can't truly verify that the scalar was built using the same hash function as Self::Partial, so I have to rely on the caller for this. This is probably already within some bounds or implicit during aggregation acc., but an opinion from someone more familiar with the aggregation flow would be really helpful here.
2. The performance drops a bit, but I think it could be improved. I do the hash function selection in the hot path, so it shows in the current benchmarks.

Benchmark

Spec: mac m1

Match/case

Operation	Blocks	Fastest	Slowest	Median	Mean	Median throughput
contains_absent	256	20.66 µs	32.04 µs	20.79 µs	21.83 µs	394.0 Mitem/s
contains_absent	32,768	21.79 µs	30.04 µs	21.95 µs	22.32 µs	373.0 Mitem/s
contains_absent	1,048,576	22.83 µs	205.40 µs	23.62 µs	26.10 µs	346.7 Mitem/s
contains_present	256	20.74 µs	40.74 µs	20.83 µs	21.12 µs	393.2 Mitem/s
contains_present	32,768	21.70 µs	53.54 µs	21.99 µs	23.13 µs	372.3 Mitem/s
contains_present	1,048,576	23.33 µs	100.00 µs	23.70 µs	24.80 µs	345.5 Mitem/s
insert	256	17.16 µs	19.29 µs	17.24 µs	17.26 µs	474.9 Mitem/s
insert	32,768	17.49 µs	28.62 µs	17.79 µs	17.87 µs	460.4 Mitem/s
insert	1,048,576	18.91 µs	92.33 µs	20.08 µs	22.59 µs	407.9 Mitem/s

Without match/case (just a default)

Operation	Blocks	Fastest	Slowest	Median	Mean	Median throughput
contains_absent	256	17.91 µs	48.87 µs	18.62 µs	19.63 µs	439.8 Mitem/s
contains_absent	32,768	18.49 µs	59.08 µs	18.95 µs	20.93 µs	432.1 Mitem/s
contains_absent	1,048,576	19.66 µs	219.20 µs	20.56 µs	24.66 µs	398.4 Mitem/s
contains_present	256	17.79 µs	28.91 µs	18.10 µs	18.90 µs	452.4 Mitem/s
contains_present	32,768	18.54 µs	42.74 µs	18.93 µs	19.24 µs	432.5 Mitem/s
contains_present	1,048,576	19.66 µs	23.29 µs	20.33 µs	20.38 µs	402.8 Mitem/s
insert	256	15.74 µs	54.08 µs	16.49 µs	17.88 µs	496.4 Mitem/s
insert	32,768	16.83 µs	58.79 µs	17.37 µs	18.30 µs	471.4 Mitem/s
insert	1,048,576	17.45 µs	113.60 µs	18.52 µs	21.86 µs	442.3 Mitem/s

[joseph-isaacs]

This is great Lets keep this as a single hash function for now!

Want to fixup CI and then I can do a final pass!

[joacoc]

@joseph-isaacs I changed it. I was not sure if you wanted HashFn to remain as a parameter/option or to be removed, so I kept it. It only contains one function and gets serialized into the layout. If that is not the case, I will remove it completely.

[codspeed]

Merging this PR will regress 2 benchmarks

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 4 improved benchmarks
❌ 2 regressed benchmarks
✅ 2166 untouched benchmarks
🆕 9 new benchmarks
⏩ 206 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
❌	WallTime	words_gather_scalar_avx2[65536]	8.3 µs	9.4 µs	-11.84%
❌	WallTime	mul_u32_nonnull_avx512	5.6 µs	6.3 µs	-10.79%
⚡	WallTime	arrow_checked_add_u32_neon[16384]	20.5 µs	12.8 µs	+60.57%
⚡	Simulation	decompress[u64, (4000, 1024)]	85.6 µs	70.4 µs	+21.71%
⚡	WallTime	arrow_checked_add_u32_avx2[16384]	21.3 µs	17.7 µs	+20.56%
⚡	Simulation	allocate_drop_arrow[0]	456.9 ns	402.7 ns	+13.45%
🆕	Simulation	contains_absent[1048576]	N/A	776 µs	N/A
🆕	Simulation	contains_absent[256]	N/A	342.7 µs	N/A
🆕	Simulation	contains_absent[32768]	N/A	701.5 µs	N/A
🆕	Simulation	contains_present[1048576]	N/A	777.2 µs	N/A
🆕	Simulation	contains_present[256]	N/A	342.7 µs	N/A
🆕	Simulation	contains_present[32768]	N/A	690.9 µs	N/A
🆕	Simulation	insert[1048576]	N/A	783.2 µs	N/A
🆕	Simulation	insert[256]	N/A	347 µs	N/A
🆕	Simulation	insert[32768]	N/A	700.2 µs	N/A

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

---

_{Comparing joacoc:bloom-index (67fae73) with develop (792cef7)}

Footnotes

1. 206 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[connortsui20]

Generally looks good to me, but left some nits.

I verified the codegen and everything seems to vectorize correctly.

I'll let @joseph-isaacs give a final review to merge.

Thanks for the contribution!

[joacoc]

Sorry to bring this late, but I just wanted to call it out before merging. There are two things I noticed when I jumped into the writer PR to wire everything together:

1. Is that I should include impl ScalarFnVTable for BloomContains in this PR and leave the PR for the writer clean from that.
2. BloomOptions::blocks_count is an initialization parameter. It is not needed to interpret a serialized filter, but I could not find a clean way to avoid storing it in the layout without making the metadata/options structure look awkward. So it is, in a sense, redundant metadata. If I could separate between initialization options from serialization options, that would be great.

[joseph-isaacs]

could at least have a debug assert?

[joacoc]

I can’t do that unless I add HashFn to the partial serialization as a header

[joacoc]

@joseph-isaacs A similar scenario happens here. The difference is that blocks_count can be derived from the serialized length.

It makes me wonder if the Options of AggregateFnVTable are a contract/source of truth for every partial/zone, or if a partial could be also self-describing, with the options acting as a contract to determine whether that partial is compatible or not? The latter could mean adding a header [blocks_count, hash_fn] (8 bytes) to the serialized partial and validating it against the options.

[joseph-isaacs]

I think the problem here is on our side.

That method should also be handled options, the option shouldn't need to be duplicated between the partial and options.

[codecov]

Codecov Report

❌ Patch coverage is 92.02614% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.25%. Comparing base (59a056d) to head (becd29f).
⚠️ Report is 8 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ts/zoned/aggregates/bloom_filter/partial/scalar.rs	68.85%	19 Missing ⚠️
...t/src/layouts/zoned/aggregates/bloom_filter/mod.rs	91.94%	17 Missing ⚠️
...ned/aggregates/bloom_filter/canonical/primitive.rs	85.71%	13 Missing ⚠️
...youts/zoned/aggregates/bloom_filter/partial/mod.rs	97.10%	5 Missing ⚠️
...uts/zoned/aggregates/bloom_filter/canonical/mod.rs	80.00%	3 Missing ⚠️
...ned/aggregates/bloom_filter/canonical/extension.rs	96.07%	2 Missing ⚠️
...ts/zoned/aggregates/bloom_filter/canonical/bool.rs	95.83%	1 Missing ⚠️
.../zoned/aggregates/bloom_filter/canonical/varbin.rs	98.38%	1 Missing ⚠️

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

stale

[connortsui20]

I think this is fine. At risk of nitpicking this forever, I think we should just merge this: there are some outstanding issues here but I don't think it is the fault of the bloom filter implementation, rather some of our abstractions.

Realistically, we are going to improve this over time (given we will likely want to use this a lot in the future), and since we haven't even figured out the writer interface (we really need to start looking at #9413) it is less important that there are small issues here.

@joseph-isaacs if you have any other concerns I'll let you review again, but let's lean towards merging.

Thanks for the contribution @joacoc!