Uh oh!
There was an error while loading. Please reload this page.
HBASE-29842 Add Ribbon Filter as an alternative to Bloom Filter - #7653
HBASE-29842 Add Ribbon Filter as an alternative to Bloom Filter#7653Jaehui-Lee wants to merge 9 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Apache9
left a comment
There was a problem hiding this comment.
For me I prefer we introduce a new configuration which indicates the bloom filter implementation? The current BloomType just indicates that how we want to apply the bloom filter, not the implementation of the bloom filter itself.
Jaehui-Lee
commented
Jan 26, 2026
Thank you for the review! I've added a new |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Jaehui-Lee
commented
Feb 2, 2026
Hi @Apache9, friendly bump. Please let me know if there are any further changes needed. Thanks! |
Apache9
left a comment
There was a problem hiding this comment.
So the ribbon filter implementation in this PR is implemented from scratch?
| if (bloomType == BloomType.ROWCOL) { | ||
| err = (float) (1 - Math.sqrt(1 - err)); | ||
| // Check if Ribbon filter is requested | ||
| if (bloomImpl == BloomFilterImpl.RIBBON) { |
There was a problem hiding this comment.
Better use switch case here? we could have other bloom implementation in the future.
| return createRibbonFilterAtWrite(conf, cacheConf, bloomType, writer); | ||
| } | ||
| float err = (float) getAdjustedErrorRate(conf, bloomType); |
There was a problem hiding this comment.
Also extract the below code to a method called createTraditionalBloomFilter?
| family, region.getManagedKeyDataCache(), region.getSystemKeyCache())) | ||
| .withBloomType(family.getBloomFilterType()).withCacheConfig(createCacheConf(family)) | ||
| .withBloomType(family.getBloomFilterType()) | ||
| .withBloomFilterImpl(BloomFilterFactory.getBloomFilterImpl(conf, family)) |
There was a problem hiding this comment.
Why here we can not call family.getBloomFilterImpl directly, like the above for BloomType?
@Apache9 Thank you for the review! I've addressed your feedback:
Yes, this is implemented from scratch based on: |
This comment has been minimized.
This comment has been minimized.
Apache9
commented
Feb 2, 2026
Although we do not use the code directly, I still think we'd better mention this reference implementation at least in the javadoc of our implementation class. The reference implementation is licensed under Apache 2.0, which should be fine. |
Jaehui-Lee
commented
Feb 2, 2026
6dabb24 Added the reference implementation link to the ribbon package javadoc. Thanks! |
This comment has been minimized.
This comment has been minimized.
Apache-HBase
commented
Feb 2, 2026
🎊 +1 overall
This message was automatically generated. |
…l bloom filter methods
https://issues.apache.org/jira/browse/HBASE-29842