xiangfu0 commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3526868111
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java:
##########
@@ -85,6 +97,61 @@ public boolean needUpdateIndices(SegmentDirectory.Reader
segmentReader) {
return false;
}
+ /**
+ * Checks whether the fpp (false positive probability) config has changed by
comparing the number of hash functions
+ * in the existing bloom filter with what the new config would produce.
Accepts both Reader and Writer since
+ * Writer extends Reader, allowing this method to be called from both
needUpdateIndices and updateIndices.
+ */
+ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String
segmentName, String column) {
+ ColumnMetadata columnMetadata =
_segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+ if (columnMetadata == null) {
+ return false;
+ }
+ int existingNumHashFunctions;
+ try {
+ PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column,
StandardIndexes.bloomFilter());
+ int version = dataBuffer.getInt(VERSION_OFFSET);
+ if (version != OnHeapGuavaBloomFilterCreator.VERSION) {
+ LOGGER.warn("Unexpected bloom filter version {} for segment: {},
column: {}; skipping fpp check", version,
+ segmentName, column);
+ return false;
+ }
+ existingNumHashFunctions = dataBuffer.getByte(NUM_HASH_FUNCTIONS_OFFSET)
& 0xFF;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to read existing bloom filter for segment: {},
column: {}", segmentName, column, e);
+ return false;
+ }
+ int expectedNumHashFunctions =
computeExpectedNumHashFunctions(columnMetadata,
_bloomFilterConfigs.get(column));
Review Comment:
**Major:** This only compares the stored `numHashFunctions`, but Guava's
serialized bloom filter also includes the bit-array length. Different fpp
values can round to the same hash-function count while producing a different
bit-array size, so Pinot would skip a needed rebuild.
For example, with cardinality 1000, fpp `0.05` and `0.045` both use `k=4`,
but Guava would allocate different underlying long-array sizes. The old bloom
filter would remain in place even though the configured fpp changed.
Can we compare both `numHashFunctions` and the stored number of longs /
bit-array size? `BaseGuavaBloomFilterReader` already reads that at Guava
payload offset 2. It would also be good to add a regression test for a same-`k`
fpp transition such as `0.05 -> 0.045`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
