xiangfu0 commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3532886963
##########
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));
+ if (expectedNumHashFunctions != existingNumHashFunctions) {
Review Comment:
This only compares numHashFunctions, but fpp also changes Guava's bit-array
size. For example, fpp 0.03 and 0.025 both use k=5 for a fixed cardinality
while the backing bit array grows, so Pinot would keep serving the old bloom
filter and silently ignore the stricter config. Compare the serialized
bit-array length/effective numBits as well, or persist/compare the full
effective bloom config, and add a same-k fpp regression test.
--
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]
