J-HowHuang commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3581823158
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java:
##########
@@ -85,6 +100,120 @@ public boolean needUpdateIndices(SegmentDirectory.Reader
segmentReader) {
return false;
}
+ /**
+ * Checks whether the effective fpp config has changed for an existing bloom
filter index.
+ *
+ * <p>V2 segments (VERSION_V2 = 2) store the effective fpp explicitly in the
header; the comparison is exact.
+ * V2 segments were created during a short window before the format was
rolled back for backward compatibility;
+ * V2 reading is retained so those segments are not permanently stuck.
+ *
+ * <p>V1 segments (VERSION = 1, the current write format) do not store fpp
in the header. FPP change is
+ * detected structurally: the stored {@code numHashFunctions} and {@code
numLongs} in the Guava payload
+ * are compared to the values that would be produced by the configured fpp +
cardinality. These two fields
+ * jointly encode the filter capacity and are stable across Guava versions.
+ *
+ * <p>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) {
+ try {
+ PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column,
StandardIndexes.bloomFilter());
+ int version = dataBuffer.getInt(VERSION_OFFSET);
+ BloomFilterConfig config = _bloomFilterConfigs.get(column);
+ ColumnMetadata columnMetadata =
_segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+
+ if (version == OnHeapGuavaBloomFilterCreator.VERSION_V2) {
+ // V2 header stores the effective fpp explicitly.
+ double storedFpp =
dataBuffer.getDouble(OnHeapGuavaBloomFilterCreator.FPP_OFFSET);
+ double expectedFpp = computeEffectiveFpp(columnMetadata, config);
+ if (Double.compare(storedFpp, expectedFpp) != 0) {
+ LOGGER.info("Bloom filter fpp config changed for segment: {},
column: {}, stored fpp: {}, "
+ + "expected fpp: {}. Index needs to be rebuilt.",
+ segmentName, column, storedFpp, expectedFpp);
+ return true;
+ }
+ return false;
+ }
+
+ if (version != OnHeapGuavaBloomFilterCreator.VERSION) {
+ // Unrecognized version — force rebuild rather than silently reading
bytes at wrong offsets.
+ LOGGER.warn("Unrecognized bloom filter version {} for segment: {},
column: {}; forcing rebuild.",
+ version, segmentName, column);
+ return true;
+ }
+ // V1 format: detect fpp change via numHashFunctions + numLongs in the
Guava payload.
+ // These values are at fixed offsets (see V1_NUM_HASH_FUNCTIONS_OFFSET /
V1_NUM_LONGS_OFFSET).
+ int storedNumHashFunctions =
dataBuffer.getByte(V1_NUM_HASH_FUNCTIONS_OFFSET) & 0xFF;
+ int storedNumLongs = dataBuffer.getInt(V1_NUM_LONGS_OFFSET);
+ int[] expected =
computeExpectedNumHashFunctionsAndNumLongs(columnMetadata, config);
+ int expectedNumHashFunctions = expected[0];
+ int expectedNumLongs = expected[1];
+ if (storedNumHashFunctions != expectedNumHashFunctions || storedNumLongs
!= expectedNumLongs) {
+ LOGGER.info("Bloom filter fpp config changed for segment: {}, column:
{}, "
+ + "stored numHashFunctions: {}, expected: {}, stored numLongs:
{}, expected: {}. "
+ + "Index needs to be rebuilt.",
+ segmentName, column, storedNumHashFunctions,
expectedNumHashFunctions, storedNumLongs, expectedNumLongs);
+ return true;
+ }
+ return false;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to read existing bloom filter for segment: {},
column: {}", segmentName, column, e);
+ return false;
+ }
+ }
Review Comment:
Why are we adding this back? Is there any good reason to directly read into
Guava's raw bytes?
--
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]
