Akanksha-kedia commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3535392531
##########
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:
Addressed. Instead of reading Guava internals at all, the effective fpp is
now stored explicitly in a new Pinot v2 header so the comparison is clean and
format-independent.
**New bloom filter file format (v2):**
```
[TYPE_VALUE_V2=2 (int)][VERSION (int)][effective_fpp (double, 8 B)][Guava
bytes...]
```
Legacy v1 format (`TYPE_VALUE=1`) is preserved unchanged for backward
compatibility.
**Changes:**
- `OnHeapGuavaBloomFilterCreator.seal()` writes the v2 header with the
effective fpp (after `maxSizeInBytes` cap) before the Guava payload.
- `BloomFilterReaderFactory` dispatches on `TYPE_VALUE`: v2 Guava payload
starts at offset 16, v1 at offset 8.
- `BloomFilterHandler.isFppChanged()` reads `storedFpp` directly from the v2
header at byte offset 8. For legacy v1 segments it skips fpp detection — those
upgrade to v2 on next rebuild.
- `BloomFilterHandlerTest` rewritten for v2: legacy v1 (skip detection), v2
same fpp (no rebuild), v2 different fpp (rebuild).
--
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]
