[WIP] Bloom filter implementation - #9398
Draft
joacoc wants to merge 8 commits into
Draft
Conversation
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
…ement constant/canonical accumulators.
…ion comments and fix primitive test for validation
…from bytes, and use constant for block_size
…e those cases. Also remove aggregates/min_max.rs and aggregates/mod.rs, and re-add them once the integration is done (they're currently in the writer) so the skip index integration PR should re-add them. This commit also adds a new composable type to the bloom filter (map) but yet not supported
I, Joaquin Colacci <joaquincolacci@gmail.com>, hereby add my Signed-off-by to this commit: a9e0550 I, Joaquin Colacci <joaquincolacci@gmail.com>, hereby add my Signed-off-by to this commit: 8d81b78 I, Joaquin Colacci <joaquincolacci@gmail.com>, hereby add my Signed-off-by to this commit: 7d0495f I, Joaquin Colacci <joaquincolacci@gmail.com>, hereby add my Signed-off-by to this commit: 43b064d I, Joaquin Colacci <joaquincolacci@gmail.com>, hereby add my Signed-off-by to this commit: f6ee474 Signed-off-by: Joaquin Colacci <joaquincolacci@gmail.com>
…so, replace T: Hash with AsRef[u8], given that this is what the oneshot expects. Primitive types translate to the new tra it, but other DTypes require to convert into bytes. Having a macro/trait over DType to convert into the underlying bytes could be useful if more agg. fns need this convertion. Also made the partial struct more practical to avoid having to expose blocks into the super BloomFilter struct. Performance improvement: removed branch inside find_hash :) Signed-off-by: Joaquin Colacci <joaquincolacci@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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
This is part of a bigger issue around skip indexes but only focuses on the Bloom filter implementation:
ZonedLayout#8901What 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-hashthat 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.