From 45d4095d14f0aa667c2887a1f876c1805eefff62 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Wed, 1 Jul 2026 14:13:14 +0530 Subject: [PATCH 01/10] Rebuild bloom filter index when fpp config changes (#17137) When a user changes the fpp (false positive probability) config for a bloom filter index, Pinot now detects the change and rebuilds the index on segment reload. Previously, users had to remove the bloom filter config, reload, re-add with the new fpp, and reload again. The detection works by comparing the number of hash functions stored in the existing bloom filter with the expected number computed from the new fpp config and column cardinality. If they differ, the bloom filter is removed and recreated with the updated config. --- .../bloomfilter/BloomFilterHandler.java | 84 +++++++++++++++++++ .../index/loader/SegmentPreProcessorTest.java | 39 +++++++++ 2 files changed, 123 insertions(+) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java index d95f509ef325..b233221ccb39 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java @@ -27,6 +27,7 @@ import org.apache.pinot.segment.local.segment.index.dictionary.DictionaryIndexType; import org.apache.pinot.segment.local.segment.index.loader.BaseIndexHandler; import org.apache.pinot.segment.local.segment.index.loader.LoaderUtils; +import org.apache.pinot.segment.local.segment.index.readers.bloom.GuavaBloomFilterReaderUtils; import org.apache.pinot.segment.spi.ColumnMetadata; import org.apache.pinot.segment.spi.V1Constants; import org.apache.pinot.segment.spi.creator.IndexCreationContext; @@ -54,6 +55,10 @@ public class BloomFilterHandler extends BaseIndexHandler { private static final Logger LOGGER = LoggerFactory.getLogger(BloomFilterHandler.class); + // Offsets within the bloom filter file: + // [0..3] TYPE_VALUE (int), [4..7] VERSION (int), [8] strategy ordinal, [9] numHashFunctions + private static final int NUM_HASH_FUNCTIONS_OFFSET = 9; + private final Map _bloomFilterConfigs; public BloomFilterHandler(SegmentDirectory segmentDirectory, Map fieldIndexConfigs, @@ -72,6 +77,11 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) { if (!columnsToAddBF.remove(column)) { LOGGER.info("Need to remove existing bloom filter from segment: {}, column: {}", segmentName, column); return true; + } else { + // Bloom filter exists for this column; check if fpp config changed by comparing numHashFunctions + if (isFppChanged(segmentReader, segmentName, column)) { + return true; + } } } // Check if any new bloomfilter need to be added. @@ -85,6 +95,54 @@ 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. + */ + 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()); + 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) { + LOGGER.info("Bloom filter fpp config changed for segment: {}, column: {}, existing numHashFunctions: {}, " + + "expected numHashFunctions: {}. Index needs to be rebuilt.", + segmentName, column, existingNumHashFunctions, expectedNumHashFunctions); + return true; + } + return false; + } + + /** + * Computes the expected number of hash functions that would be used for a bloom filter created with the given + * column metadata and bloom filter config. This mirrors the logic in Guava's BloomFilter.create(). + */ + private int computeExpectedNumHashFunctions(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { + int cardinality = columnMetadata.getCardinality(); + if (cardinality <= 0) { + cardinality = columnMetadata.getTotalNumberOfEntries(); + } + double fpp = bloomFilterConfig.getFpp(); + int maxSizeInBytes = bloomFilterConfig.getMaxSizeInBytes(); + if (maxSizeInBytes > 0) { + double minFpp = GuavaBloomFilterReaderUtils.computeFPP(maxSizeInBytes, cardinality); + fpp = Math.max(fpp, minFpp); + } + // Formula from Guava BloomFilter: optimalNumOfBits = (long) (-n * ln(p) / (ln2)^2) + long optimalNumOfBits = (long) (-cardinality * Math.log(fpp) / (Math.log(2) * Math.log(2))); + // Formula from Guava BloomFilter: optimalNumOfHashFunctions = max(1, round(m/n * ln2)) + return Math.max(1, (int) Math.round((double) optimalNumOfBits / cardinality * Math.log(2))); + } + @Override public void updateIndices(SegmentDirectory.Writer segmentWriter) throws Exception { @@ -97,6 +155,32 @@ public void updateIndices(SegmentDirectory.Writer segmentWriter) LOGGER.info("Removing existing bloom filter from segment: {}, column: {}", segmentName, column); segmentWriter.removeIndex(column, StandardIndexes.bloomFilter()); LOGGER.info("Removed existing bloom filter from segment: {}, column: {}", segmentName, column); + } else { + // Bloom filter exists for this column; check if fpp config changed + ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); + if (columnMetadata == null) { + continue; + } + int existingNumHashFunctions; + try { + PinotDataBuffer dataBuffer = segmentWriter.getIndexFor(column, StandardIndexes.bloomFilter()); + 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); + segmentWriter.removeIndex(column, StandardIndexes.bloomFilter()); + columnsToAddBF.add(column); + continue; + } + int expectedNumHashFunctions = + computeExpectedNumHashFunctions(columnMetadata, _bloomFilterConfigs.get(column)); + if (expectedNumHashFunctions != existingNumHashFunctions) { + LOGGER.info("Bloom filter fpp config changed for segment: {}, column: {}, existing numHashFunctions: {}, " + + "expected numHashFunctions: {}. Deleting existing bloom filter before rebuilding a new one.", + segmentName, column, existingNumHashFunctions, expectedNumHashFunctions); + segmentWriter.removeIndex(column, StandardIndexes.bloomFilter()); + LOGGER.info("Removed existing bloom filter from segment: {}, column: {}", segmentName, column); + columnsToAddBF.add(column); + } } } for (String column : columnsToAddBF) { diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java index 9f1a3ad30178..c94592b9ae2a 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java @@ -1747,6 +1747,45 @@ public void testH3IndexResolutionUpdate(SegmentVersion segmentVersion) runPreProcessor(_newColumnsSchemaWithH3Json); } + @Test(dataProvider = "bothV1AndV3") + public void testBloomFilterFppUpdate(SegmentVersion segmentVersion) + throws Exception { + buildSegment(segmentVersion); + + // Create bloom filter with fpp 0.1 + _bloomFilterConfigs = Map.of("column3", new BloomFilterConfig(0.1, 0, false)); + runPreProcessor(); + + // Verify no processing needed with same config + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentPreProcessor processor = new SegmentPreProcessor(segmentDirectory, + createIndexLoadingConfig(_schema))) { + assertFalse(processor.needProcess()); + } + + // Update bloom filter fpp to 0.01 + _bloomFilterConfigs = Map.of("column3", new BloomFilterConfig(0.01, 0, false)); + + // Verify that preprocessing is needed due to fpp change + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentPreProcessor processor = new SegmentPreProcessor(segmentDirectory, + createIndexLoadingConfig(_schema))) { + assertTrue(processor.needProcess()); + processor.process(SEGMENT_OPERATIONS_THROTTLER); + } + + // Verify no more processing is needed after rebuild + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentPreProcessor processor = new SegmentPreProcessor(segmentDirectory, + createIndexLoadingConfig(_schema))) { + assertFalse(processor.needProcess()); + } + + // Remove bloom filter config + resetIndexConfigs(); + runPreProcessor(); + } + private void verifyProcessNeeded() throws Exception { try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); From d793f9794aae7d4473dbe8aae49520ffc2c6a5a7 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Thu, 2 Jul 2026 16:04:13 +0530 Subject: [PATCH 02/10] Fix computeExpectedNumHashFunctions formula, deduplicate isFppChanged, add version guard and small-cardinality test - Fix CRITICAL formula bug: replace the two-step formula (optimalNumOfBits via long-cast then k = round(m/n * ln2)) with Guava's direct formula k = max(1, round(-ln(p) / ln(2))). The old formula produced k=6 for cardinality=1 and fpp=0.01 but Guava writes k=7, causing an infinite rebuild loop on every segment reload. - Deduplicate fpp-change detection: replace the duplicated 25-line inline block in updateIndices with a call to isFppChanged (which accepts SegmentDirectory.Reader, satisfied by both Reader and Writer). - Add version guard: verify the bloom filter file version matches OnHeapGuavaBloomFilterCreator.VERSION before reading numHashFunctions at byte offset 9; log a warning and skip the fpp check for unknown versions rather than reading from an unexpected layout. - Add testBloomFilterFppUpdateSmallCardinality: exercises the formula bug with a cardinality-1 column; the final assertFalse(needProcess()) proves idempotency and would fail with the old formula. --- .../bloomfilter/BloomFilterHandler.java | 54 ++++++++----------- .../index/loader/SegmentPreProcessorTest.java | 53 ++++++++++++++++++ 2 files changed, 76 insertions(+), 31 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java index b233221ccb39..4080f8c50cb5 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java @@ -24,6 +24,7 @@ import java.util.Map; import java.util.Set; import org.apache.commons.io.FileUtils; +import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator; import org.apache.pinot.segment.local.segment.index.dictionary.DictionaryIndexType; import org.apache.pinot.segment.local.segment.index.loader.BaseIndexHandler; import org.apache.pinot.segment.local.segment.index.loader.LoaderUtils; @@ -57,6 +58,7 @@ public class BloomFilterHandler extends BaseIndexHandler { // Offsets within the bloom filter file: // [0..3] TYPE_VALUE (int), [4..7] VERSION (int), [8] strategy ordinal, [9] numHashFunctions + private static final int VERSION_OFFSET = 4; private static final int NUM_HASH_FUNCTIONS_OFFSET = 9; private final Map _bloomFilterConfigs; @@ -97,7 +99,8 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) { /** * 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. + * 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); @@ -107,6 +110,12 @@ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segme 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); @@ -123,24 +132,24 @@ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segme } /** - * Computes the expected number of hash functions that would be used for a bloom filter created with the given - * column metadata and bloom filter config. This mirrors the logic in Guava's BloomFilter.create(). + * Computes the expected number of hash functions that Guava's BloomFilter would use for the given fpp, after + * optionally adjusting fpp for the maxSizeInBytes constraint. Uses Guava's direct formula which depends only + * on fpp: {@code k = max(1, round(-ln(p) / ln(2)))}. This matches Guava's + * {@code BloomFilter.optimalNumOfHashFunctions()} exactly. */ private int computeExpectedNumHashFunctions(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { - int cardinality = columnMetadata.getCardinality(); - if (cardinality <= 0) { - cardinality = columnMetadata.getTotalNumberOfEntries(); - } double fpp = bloomFilterConfig.getFpp(); int maxSizeInBytes = bloomFilterConfig.getMaxSizeInBytes(); if (maxSizeInBytes > 0) { + int cardinality = columnMetadata.getCardinality(); + if (cardinality <= 0) { + cardinality = columnMetadata.getTotalNumberOfEntries(); + } double minFpp = GuavaBloomFilterReaderUtils.computeFPP(maxSizeInBytes, cardinality); fpp = Math.max(fpp, minFpp); } - // Formula from Guava BloomFilter: optimalNumOfBits = (long) (-n * ln(p) / (ln2)^2) - long optimalNumOfBits = (long) (-cardinality * Math.log(fpp) / (Math.log(2) * Math.log(2))); - // Formula from Guava BloomFilter: optimalNumOfHashFunctions = max(1, round(m/n * ln2)) - return Math.max(1, (int) Math.round((double) optimalNumOfBits / cardinality * Math.log(2))); + // Matches Guava BloomFilter.optimalNumOfHashFunctions() exactly + return Math.max(1, (int) Math.round(-Math.log(fpp) / Math.log(2))); } @Override @@ -157,26 +166,9 @@ public void updateIndices(SegmentDirectory.Writer segmentWriter) LOGGER.info("Removed existing bloom filter from segment: {}, column: {}", segmentName, column); } else { // Bloom filter exists for this column; check if fpp config changed - ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); - if (columnMetadata == null) { - continue; - } - int existingNumHashFunctions; - try { - PinotDataBuffer dataBuffer = segmentWriter.getIndexFor(column, StandardIndexes.bloomFilter()); - 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); - segmentWriter.removeIndex(column, StandardIndexes.bloomFilter()); - columnsToAddBF.add(column); - continue; - } - int expectedNumHashFunctions = - computeExpectedNumHashFunctions(columnMetadata, _bloomFilterConfigs.get(column)); - if (expectedNumHashFunctions != existingNumHashFunctions) { - LOGGER.info("Bloom filter fpp config changed for segment: {}, column: {}, existing numHashFunctions: {}, " - + "expected numHashFunctions: {}. Deleting existing bloom filter before rebuilding a new one.", - segmentName, column, existingNumHashFunctions, expectedNumHashFunctions); + if (isFppChanged(segmentWriter, segmentName, column)) { + LOGGER.info("Deleting existing bloom filter for segment: {}, column: {} before rebuilding.", segmentName, + column); segmentWriter.removeIndex(column, StandardIndexes.bloomFilter()); LOGGER.info("Removed existing bloom filter from segment: {}, column: {}", segmentName, column); columnsToAddBF.add(column); diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java index c94592b9ae2a..94b884f66ead 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/SegmentPreProcessorTest.java @@ -1786,6 +1786,59 @@ public void testBloomFilterFppUpdate(SegmentVersion segmentVersion) runPreProcessor(); } + /** + * Tests that the formula for computing the expected number of hash functions is correct for very small cardinalities. + * With cardinality=1 and fpp=0.01, the old two-step formula (via optimalNumOfBits with a long cast) computes k=6 + * but Guava actually writes k=7, causing an infinite rebuild loop on every segment reload. The fixed direct formula + * gives k=7, matching Guava exactly, so the idempotency check below would fail without the fix. + */ + @Test(dataProvider = "bothV1AndV3") + public void testBloomFilterFppUpdateSmallCardinality(SegmentVersion segmentVersion) + throws Exception { + // Build a segment with a cardinality-1 STRING column (single repeated value → exactly 1 unique entry in dict) + String[] singleValueArray = {"only_value", "only_value", "only_value", "only_value", "only_value"}; + long[] longValues = {1L, 2L, 3L, 4L, 5L}; + TableConfig smallCardTableConfig = + new TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME).build(); + Schema smallCardSchema = + new Schema.SchemaBuilder().addSingleValueDimension("stringCol", FieldSpec.DataType.STRING) + .addMetric("longCol", FieldSpec.DataType.LONG) + .build(); + buildTestSegment(smallCardTableConfig, smallCardSchema, singleValueArray, longValues); + + // Create bloom filter with fpp 0.1 + IndexingConfig indexingConfig = smallCardTableConfig.getIndexingConfig(); + indexingConfig.setBloomFilterConfigs(Map.of("stringCol", new BloomFilterConfig(0.1, 0, false))); + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentPreProcessor processor = new SegmentPreProcessor(segmentDirectory, + new IndexLoadingConfig(smallCardTableConfig, smallCardSchema))) { + processor.process(SEGMENT_OPERATIONS_THROTTLER); + } + + // Verify no processing needed with same fpp + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentPreProcessor processor = new SegmentPreProcessor(segmentDirectory, + new IndexLoadingConfig(smallCardTableConfig, smallCardSchema))) { + assertFalse(processor.needProcess()); + } + + // Update fpp to 0.01 — should trigger rebuild (k changes: 0.1→k=3, 0.01→k=7) + indexingConfig.setBloomFilterConfigs(Map.of("stringCol", new BloomFilterConfig(0.01, 0, false))); + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentPreProcessor processor = new SegmentPreProcessor(segmentDirectory, + new IndexLoadingConfig(smallCardTableConfig, smallCardSchema))) { + assertTrue(processor.needProcess()); + processor.process(SEGMENT_OPERATIONS_THROTTLER); + } + + // Verify no more processing needed after rebuild — proves no infinite rebuild loop (the formula bug) + try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); + SegmentPreProcessor processor = new SegmentPreProcessor(segmentDirectory, + new IndexLoadingConfig(smallCardTableConfig, smallCardSchema))) { + assertFalse(processor.needProcess()); + } + } + private void verifyProcessNeeded() throws Exception { try (SegmentDirectory segmentDirectory = new SegmentLocalFSDirectory(INDEX_DIR, ReadMode.mmap); From bd0726003fbd40f7f8305fda0d5a83e39f3ef7fc Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Tue, 7 Jul 2026 09:38:05 +0530 Subject: [PATCH 03/10] Address review: compare numLongs in addition to numHashFunctions for fpp-change detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Different fpp values can round to the same k (numHashFunctions) while producing a different bit-array size (numLongs). For example, with cardinality=1000, fpp=0.05 and fpp=0.045 both yield k=4 but Guava allocates 98 vs 101 longs respectively. The old check (numHashFunctions only) would silently keep the stale bloom filter and ignore the stricter fpp config. Changes: - Add NUM_LONGS_OFFSET = 10 constant (byte offset of numLongs in Pinot BF file) - Read existingNumLongs from the stored buffer alongside existingNumHashFunctions - Add computeExpectedNumLongs() using Guava's formula: numLongs = ceil(numBits / 64) - Extract effectiveFpp() helper to deduplicate fpp-capping logic shared by both computeExpected* methods - Trigger rebuild when either numHashFunctions OR numLongs differ - Add BloomFilterHandlerTest with same-k/different-numLongs regression test (fpp 0.05 → 0.045, cardinality=1000: k stays 4, numLongs changes 98→101) Co-Authored-By: Claude Sonnet 4.6 --- .../bloomfilter/BloomFilterHandler.java | 65 ++++-- .../bloomfilter/BloomFilterHandlerTest.java | 187 ++++++++++++++++++ 2 files changed, 237 insertions(+), 15 deletions(-) create mode 100644 pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java index 4080f8c50cb5..52c06aaa311a 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java @@ -57,9 +57,10 @@ public class BloomFilterHandler extends BaseIndexHandler { private static final Logger LOGGER = LoggerFactory.getLogger(BloomFilterHandler.class); // Offsets within the bloom filter file: - // [0..3] TYPE_VALUE (int), [4..7] VERSION (int), [8] strategy ordinal, [9] numHashFunctions + // [0..3] TYPE_VALUE (int), [4..7] VERSION (int), [8] strategy ordinal, [9] numHashFunctions, [10..13] numLongs private static final int VERSION_OFFSET = 4; private static final int NUM_HASH_FUNCTIONS_OFFSET = 9; + private static final int NUM_LONGS_OFFSET = 10; private final Map _bloomFilterConfigs; @@ -98,9 +99,16 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) { } /** - * 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. + * Checks whether the bloom filter config has changed by comparing both the number of hash functions and the + * bit-array size (numLongs) in the existing bloom filter with what the new config would produce. + * + *

Comparing only numHashFunctions is insufficient: different fpp values can round to the same k while + * producing a different bit-array size. For example, with cardinality 1000, fpp 0.05 and 0.045 both use k=4 + * but Guava allocates different underlying long-array sizes, so the old bloom filter would remain in place + * even though the configured fpp changed. + * + *

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); @@ -108,6 +116,7 @@ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segme return false; } int existingNumHashFunctions; + int existingNumLongs; try { PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column, StandardIndexes.bloomFilter()); int version = dataBuffer.getInt(VERSION_OFFSET); @@ -117,27 +126,54 @@ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segme return false; } existingNumHashFunctions = dataBuffer.getByte(NUM_HASH_FUNCTIONS_OFFSET) & 0xFF; + existingNumLongs = dataBuffer.getInt(NUM_LONGS_OFFSET); } 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) { - LOGGER.info("Bloom filter fpp config changed for segment: {}, column: {}, existing numHashFunctions: {}, " - + "expected numHashFunctions: {}. Index needs to be rebuilt.", - segmentName, column, existingNumHashFunctions, expectedNumHashFunctions); + BloomFilterConfig config = _bloomFilterConfigs.get(column); + int expectedNumHashFunctions = computeExpectedNumHashFunctions(columnMetadata, config); + int expectedNumLongs = computeExpectedNumLongs(columnMetadata, config); + if (expectedNumHashFunctions != existingNumHashFunctions || expectedNumLongs != existingNumLongs) { + LOGGER.info("Bloom filter config changed for segment: {}, column: {}, " + + "existing numHashFunctions: {}, expected numHashFunctions: {}, " + + "existing numLongs: {}, expected numLongs: {}. Index needs to be rebuilt.", + segmentName, column, existingNumHashFunctions, expectedNumHashFunctions, + existingNumLongs, expectedNumLongs); return true; } return false; } /** - * Computes the expected number of hash functions that Guava's BloomFilter would use for the given fpp, after - * optionally adjusting fpp for the maxSizeInBytes constraint. Uses Guava's direct formula which depends only - * on fpp: {@code k = max(1, round(-ln(p) / ln(2)))}. This matches Guava's - * {@code BloomFilter.optimalNumOfHashFunctions()} exactly. + * Computes the effective fpp after applying the maxSizeInBytes constraint, then returns the + * number of hash functions Guava would use: {@code k = max(1, round(-ln(p) / ln(2)))}. */ private int computeExpectedNumHashFunctions(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { + double fpp = effectiveFpp(columnMetadata, bloomFilterConfig); + // Matches Guava BloomFilter.optimalNumOfHashFunctions() exactly + return Math.max(1, (int) Math.round(-Math.log(fpp) / Math.log(2))); + } + + /** + * Computes the number of longs Guava would allocate for the bit array: + * {@code numLongs = ceil(optimalNumOfBits(n, p) / 64)}, where + * {@code optimalNumOfBits = max(1, (long)(-n * ln(p) / ln(2)^2))}. + */ + private int computeExpectedNumLongs(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { + double fpp = effectiveFpp(columnMetadata, bloomFilterConfig); + int cardinality = columnMetadata.getCardinality(); + if (cardinality <= 0) { + cardinality = columnMetadata.getTotalNumberOfEntries(); + } + long numBits = Math.max(1L, (long) (-cardinality * Math.log(fpp) / (Math.log(2) * Math.log(2)))); + return (int) ((numBits + 63) / 64); + } + + /** + * Returns the effective fpp after applying the maxSizeInBytes cap (if set). + */ + private double effectiveFpp(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { double fpp = bloomFilterConfig.getFpp(); int maxSizeInBytes = bloomFilterConfig.getMaxSizeInBytes(); if (maxSizeInBytes > 0) { @@ -148,8 +184,7 @@ private int computeExpectedNumHashFunctions(ColumnMetadata columnMetadata, Bloom double minFpp = GuavaBloomFilterReaderUtils.computeFPP(maxSizeInBytes, cardinality); fpp = Math.max(fpp, minFpp); } - // Matches Guava BloomFilter.optimalNumOfHashFunctions() exactly - return Math.max(1, (int) Math.round(-Math.log(fpp) / Math.log(2))); + return fpp; } @Override diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java new file mode 100644 index 000000000000..fcb6ab84bd9f --- /dev/null +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java @@ -0,0 +1,187 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.segment.local.segment.index.loader.bloomfilter; + +import com.google.common.hash.BloomFilter; +import com.google.common.hash.Funnels; +import java.io.DataOutputStream; +import java.io.File; +import java.io.FileOutputStream; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import java.util.Set; +import org.apache.commons.io.FileUtils; +import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator; +import org.apache.pinot.segment.spi.ColumnMetadata; +import org.apache.pinot.segment.spi.index.FieldIndexConfigs; +import org.apache.pinot.segment.spi.index.StandardIndexes; +import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl; +import org.apache.pinot.segment.spi.memory.PinotDataBuffer; +import org.apache.pinot.segment.spi.store.SegmentDirectory; +import org.apache.pinot.spi.config.table.BloomFilterConfig; +import org.apache.pinot.spi.config.table.TableConfig; +import org.apache.pinot.spi.data.Schema; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertTrue; + + +/** + * Unit tests for {@link BloomFilterHandler} fpp-change detection. + * + *

The key invariant being tested: when the bloom filter configuration changes, even if the change does NOT + * alter the number of hash functions (k), it may still change the bit-array size (numLongs). Both dimensions + * must be compared to correctly detect a rebuild requirement. + * + *

Concretely, for cardinality=1000: + *

    + *
  • fpp=0.05 → k=4, numLongs=98
    • + *
  • fpp=0.045 → k=4, numLongs=101
    • + *
+ * The old check (numHashFunctions only) silently kept the stale index; the new check catches the difference. + */ +public class BloomFilterHandlerTest { + private static final String COLUMN = "myBloomCol"; + private static final int CARDINALITY = 1000; + + // With cardinality=1000 and fpp=0.05: k=4, numLongs=98 + private static final double FPP_STORED = 0.05; + // With cardinality=1000 and fpp=0.045: k=4 (same!), numLongs=101 (different!) + private static final double FPP_NEW_SAME_K = 0.045; + // With cardinality=1000 and fpp=0.01: k=7 (different from k=4) — sanity check + private static final double FPP_NEW_DIFF_K = 0.01; + + private File _tempDir; + private File _bloomFilterFile; + + @BeforeMethod + public void setUp() + throws Exception { + _tempDir = new File(FileUtils.getTempDirectory(), "bloom-filter-handler-test-" + System.nanoTime()); + assertTrue(_tempDir.mkdirs()); + _bloomFilterFile = new File(_tempDir, "bloom.tmp"); + } + + @AfterMethod + public void tearDown() { + FileUtils.deleteQuietly(_tempDir); + } + + /** + * Regression test: fpp=0.05 → 0.045 both yield k=4 but different numLongs (98 vs 101) for cardinality=1000. + * The old check (numHashFunctions only) would return false (no rebuild), leaving a stale index. + * The new check (numHashFunctions + numLongs) must return true (rebuild needed). + */ + @Test + public void testNeedUpdateReturnsTrueWhenSameKButDifferentNumLongs() + throws Exception { + writeBloomFilter(CARDINALITY, FPP_STORED); + + BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_NEW_SAME_K, 0, false)); + SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); + + assertTrue(handler.needUpdateIndices(reader), + "Rebuild expected: fpp 0.05→0.045 keeps k=4 but changes numLongs (98→101)"); + } + + /** + * Sanity check: when fpp is unchanged the handler must not trigger a spurious rebuild. + */ + @Test + public void testNeedUpdateReturnsFalseWhenFppUnchanged() + throws Exception { + writeBloomFilter(CARDINALITY, FPP_STORED); + + BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_STORED, 0, false)); + SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); + + assertFalse(handler.needUpdateIndices(reader), + "No rebuild expected when fpp is unchanged"); + } + + /** + * Sanity check: when fpp changes enough to alter k the handler must trigger a rebuild (pre-existing behavior). + */ + @Test + public void testNeedUpdateReturnsTrueWhenKChanges() + throws Exception { + writeBloomFilter(CARDINALITY, FPP_STORED); + + BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_NEW_DIFF_K, 0, false)); + SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); + + assertTrue(handler.needUpdateIndices(reader), + "Rebuild expected: fpp 0.05→0.01 changes k from 4 to 7"); + } + + // --- helpers --- + + /** + * Writes a Pinot-format bloom filter file: [TYPE_VALUE(int)][VERSION(int)][Guava bytes]. + */ + private void writeBloomFilter(int cardinality, double fpp) + throws Exception { + BloomFilter bf = BloomFilter.create(Funnels.stringFunnel(StandardCharsets.UTF_8), cardinality, fpp); + try (DataOutputStream out = new DataOutputStream(new FileOutputStream(_bloomFilterFile))) { + out.writeInt(OnHeapGuavaBloomFilterCreator.TYPE_VALUE); + out.writeInt(OnHeapGuavaBloomFilterCreator.VERSION); + bf.writeTo(out); + } + } + + private BloomFilterHandler createHandler(String columnName, BloomFilterConfig bloomFilterConfig) { + FieldIndexConfigs fieldIndexConfigs = + new FieldIndexConfigs.Builder().add(StandardIndexes.bloomFilter(), bloomFilterConfig).build(); + + ColumnMetadata columnMetadata = mock(ColumnMetadata.class); + when(columnMetadata.getCardinality()).thenReturn(CARDINALITY); + when(columnMetadata.getTotalNumberOfEntries()).thenReturn(CARDINALITY); + + SegmentMetadataImpl segmentMetadata = mock(SegmentMetadataImpl.class); + when(segmentMetadata.getName()).thenReturn("testSegment"); + when(segmentMetadata.getColumnMetadataFor(columnName)).thenReturn(columnMetadata); + + SegmentDirectory segmentDirectory = mock(SegmentDirectory.class); + when(segmentDirectory.getSegmentMetadata()).thenReturn(segmentMetadata); + + return new BloomFilterHandler(segmentDirectory, Map.of(columnName, fieldIndexConfigs), + mock(TableConfig.class), mock(Schema.class)); + } + + private SegmentDirectory.Reader mockReaderWithBloomFilter(String columnName, File bloomFilterFile) + throws Exception { + PinotDataBuffer dataBuffer = PinotDataBuffer.loadBigEndianFile(bloomFilterFile); + + SegmentDirectory segDir = mock(SegmentDirectory.class); + when(segDir.getColumnsWithIndex(StandardIndexes.bloomFilter())).thenReturn(Set.of(columnName)); + + SegmentDirectory.Reader reader = mock(SegmentDirectory.Reader.class); + when(reader.toSegmentDirectory()).thenReturn(segDir); + when(reader.getIndexFor(eq(columnName), eq(StandardIndexes.bloomFilter()))).thenReturn(dataBuffer); + + return reader; + } +} From 9ec5fd591d777d800aabf8037ae1c164e4d0482f Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Tue, 7 Jul 2026 11:42:41 +0530 Subject: [PATCH 04/10] Address review: store effective fpp in v2 header; avoid Guava internal dependency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrites fpp-change detection per reviewer feedback to eliminate the fragile dependency on Guava's internal serialisation layout (numHashFunctions, numLongs). - OnHeapGuavaBloomFilterCreator: new TYPE_VALUE_V2=2 writes a v2 header [TYPE_VALUE_V2(int)][VERSION(int)][effective_fpp(double)][Guava bytes...] so the effective fpp (post maxSizeInBytes cap) is stored explicitly. - BloomFilterReaderFactory: dispatches on TYPE_VALUE; v2 payload starts at offset 16, legacy v1 payload at offset 8. - BloomFilterHandler.isFppChanged: reads stored fpp directly from the v2 header byte offset; skips detection gracefully for legacy v1 segments (they upgrade to v2 on next rebuild for any other reason). - BloomFilterHandlerTest: rewritten for the v2 format — three tests cover legacy v1 (skip detection), v2 same fpp (no rebuild), v2 changed fpp (rebuild). Co-Authored-By: Claude Sonnet 4.6 --- .../bloom/OnHeapGuavaBloomFilterCreator.java | 15 ++- .../bloomfilter/BloomFilterHandler.java | 91 ++++++------------- .../bloom/BloomFilterReaderFactory.java | 18 +++- .../bloomfilter/BloomFilterHandlerTest.java | 77 +++++++++------- 4 files changed, 103 insertions(+), 98 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java index b8b109431982..30db2c168221 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java @@ -43,12 +43,23 @@ public class OnHeapGuavaBloomFilterCreator implements BloomFilterCreator { private static final Logger LOGGER = LoggerFactory.getLogger(OnHeapGuavaBloomFilterCreator.class); + /** Legacy format: {@code [TYPE_VALUE=1 (int)][VERSION (int)][Guava bytes...]} — no fpp in header. */ public static final int TYPE_VALUE = 1; + /** + * V2 format: {@code [TYPE_VALUE_V2=2 (int)][VERSION (int)][effective FPP (double)][Guava bytes...]}. + * The effective fpp (after applying any {@code maxSizeInBytes} cap) is stored at byte offset 8 so that + * {@link org.apache.pinot.segment.local.segment.index.loader.bloomfilter.BloomFilterHandler} can compare it + * directly without depending on Guava's internal serialisation layout. + */ + public static final int TYPE_VALUE_V2 = 2; public static final int VERSION = 1; + /** Byte offset of the fpp field in a v2 bloom filter file (after TYPE_VALUE_V2 + VERSION). */ + public static final int FPP_OFFSET = 8; private final File _bloomFilterFile; private final BloomFilter _bloomFilter; private final FieldSpec.DataType _dataType; + private final double _effectiveFpp; // TODO: This method is here for compatibility reasons, should be removed in future PRs // exit_criteria: Not needed in Apache Pinot once #10184 is merged @@ -69,6 +80,7 @@ public OnHeapGuavaBloomFilterCreator(File indexDir, String columnName, int cardi double minFpp = GuavaBloomFilterReaderUtils.computeFPP(maxSizeInBytes, cardinality); fpp = Math.max(fpp, minFpp); } + _effectiveFpp = fpp; LOGGER.info("Creating bloom filter with cardinality: {}, fpp: {}", cardinality, fpp); _bloomFilter = BloomFilter.create(Funnels.stringFunnel(StandardCharsets.UTF_8), cardinality, fpp); } @@ -87,8 +99,9 @@ public void add(String value) { public void seal() throws IOException { try (DataOutputStream out = new DataOutputStream(new FileOutputStream(_bloomFilterFile))) { - out.writeInt(TYPE_VALUE); + out.writeInt(TYPE_VALUE_V2); out.writeInt(VERSION); + out.writeDouble(_effectiveFpp); _bloomFilter.writeTo(out); } } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java index 52c06aaa311a..6953f0bc62fc 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java @@ -56,11 +56,7 @@ public class BloomFilterHandler extends BaseIndexHandler { private static final Logger LOGGER = LoggerFactory.getLogger(BloomFilterHandler.class); - // Offsets within the bloom filter file: - // [0..3] TYPE_VALUE (int), [4..7] VERSION (int), [8] strategy ordinal, [9] numHashFunctions, [10..13] numLongs - private static final int VERSION_OFFSET = 4; - private static final int NUM_HASH_FUNCTIONS_OFFSET = 9; - private static final int NUM_LONGS_OFFSET = 10; + private static final int TYPE_VALUE_OFFSET = 0; private final Map _bloomFilterConfigs; @@ -99,84 +95,53 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) { } /** - * Checks whether the bloom filter config has changed by comparing both the number of hash functions and the - * bit-array size (numLongs) in the existing bloom filter with what the new config would produce. + * Checks whether the effective fpp config has changed by comparing the fpp stored in the v2 bloom filter + * header with the effective fpp derived from the current {@link BloomFilterConfig}. * - *

Comparing only numHashFunctions is insufficient: different fpp values can round to the same k while - * producing a different bit-array size. For example, with cardinality 1000, fpp 0.05 and 0.045 both use k=4 - * but Guava allocates different underlying long-array sizes, so the old bloom filter would remain in place - * even though the configured fpp changed. + *

The v2 format stores the effective fpp (post {@code maxSizeInBytes} cap) explicitly at byte offset + * {@link OnHeapGuavaBloomFilterCreator#FPP_OFFSET} so comparisons are exact and do not depend on Guava's + * internal serialisation layout. + * + *

Legacy v1 segments (TYPE_VALUE = 1) do not carry fpp in the header; fpp-change detection is skipped + * for those segments. They will be upgraded to v2 the next time the bloom filter is rebuilt for any reason. * *

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; - int existingNumLongs; 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); + int typeValue = dataBuffer.getInt(TYPE_VALUE_OFFSET); + if (typeValue != OnHeapGuavaBloomFilterCreator.TYPE_VALUE_V2) { + // Legacy v1 format — fpp is not stored in the header; skip fpp-change detection. + // The index will be upgraded to v2 the next time it is rebuilt. return false; } - existingNumHashFunctions = dataBuffer.getByte(NUM_HASH_FUNCTIONS_OFFSET) & 0xFF; - existingNumLongs = dataBuffer.getInt(NUM_LONGS_OFFSET); + double storedFpp = dataBuffer.getDouble(OnHeapGuavaBloomFilterCreator.FPP_OFFSET); + BloomFilterConfig config = _bloomFilterConfigs.get(column); + ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); + 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; } catch (Exception e) { LOGGER.warn("Failed to read existing bloom filter for segment: {}, column: {}", segmentName, column, e); return false; } - BloomFilterConfig config = _bloomFilterConfigs.get(column); - int expectedNumHashFunctions = computeExpectedNumHashFunctions(columnMetadata, config); - int expectedNumLongs = computeExpectedNumLongs(columnMetadata, config); - if (expectedNumHashFunctions != existingNumHashFunctions || expectedNumLongs != existingNumLongs) { - LOGGER.info("Bloom filter config changed for segment: {}, column: {}, " - + "existing numHashFunctions: {}, expected numHashFunctions: {}, " - + "existing numLongs: {}, expected numLongs: {}. Index needs to be rebuilt.", - segmentName, column, existingNumHashFunctions, expectedNumHashFunctions, - existingNumLongs, expectedNumLongs); - return true; - } - return false; - } - - /** - * Computes the effective fpp after applying the maxSizeInBytes constraint, then returns the - * number of hash functions Guava would use: {@code k = max(1, round(-ln(p) / ln(2)))}. - */ - private int computeExpectedNumHashFunctions(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { - double fpp = effectiveFpp(columnMetadata, bloomFilterConfig); - // Matches Guava BloomFilter.optimalNumOfHashFunctions() exactly - return Math.max(1, (int) Math.round(-Math.log(fpp) / Math.log(2))); - } - - /** - * Computes the number of longs Guava would allocate for the bit array: - * {@code numLongs = ceil(optimalNumOfBits(n, p) / 64)}, where - * {@code optimalNumOfBits = max(1, (long)(-n * ln(p) / ln(2)^2))}. - */ - private int computeExpectedNumLongs(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { - double fpp = effectiveFpp(columnMetadata, bloomFilterConfig); - int cardinality = columnMetadata.getCardinality(); - if (cardinality <= 0) { - cardinality = columnMetadata.getTotalNumberOfEntries(); - } - long numBits = Math.max(1L, (long) (-cardinality * Math.log(fpp) / (Math.log(2) * Math.log(2)))); - return (int) ((numBits + 63) / 64); } /** - * Returns the effective fpp after applying the maxSizeInBytes cap (if set). + * Computes the effective fpp for a new bloom filter: applies the {@code maxSizeInBytes} cap if set. + * This matches the fpp computation in {@link OnHeapGuavaBloomFilterCreator}. */ - private double effectiveFpp(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { + private double computeEffectiveFpp(ColumnMetadata columnMetadata, BloomFilterConfig bloomFilterConfig) { double fpp = bloomFilterConfig.getFpp(); int maxSizeInBytes = bloomFilterConfig.getMaxSizeInBytes(); - if (maxSizeInBytes > 0) { + if (maxSizeInBytes > 0 && columnMetadata != null) { int cardinality = columnMetadata.getCardinality(); if (cardinality <= 0) { cardinality = columnMetadata.getTotalNumberOfEntries(); diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java index 915ace5bcbbb..623068f7ec94 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java @@ -30,15 +30,29 @@ private BloomFilterReaderFactory() { private static final int TYPE_VALUE_OFFSET = 0; private static final int VERSION_OFFSET = 4; - private static final int HEADER_SIZE = 8; + /** Byte offset at which Guava bloom filter bytes start in the legacy v1 format. */ + private static final int GUAVA_PAYLOAD_OFFSET_V1 = 8; + /** + * Byte offset at which Guava bloom filter bytes start in the v2 format. + * V2 header: [TYPE_VALUE_V2 (int, 4)][VERSION (int, 4)][FPP (double, 8)] = 16 bytes. + */ + private static final int GUAVA_PAYLOAD_OFFSET_V2 = 16; public static BloomFilterReader getBloomFilterReader(PinotDataBuffer dataBuffer, boolean onHeap) { int typeValue = dataBuffer.getInt(TYPE_VALUE_OFFSET); int version = dataBuffer.getInt(VERSION_OFFSET); + if (typeValue == OnHeapGuavaBloomFilterCreator.TYPE_VALUE_V2 + && version == OnHeapGuavaBloomFilterCreator.VERSION) { + // V2 format: [TYPE_VALUE_V2][VERSION][FPP (double)][Guava bytes] + PinotDataBuffer bloomFilterDataBuffer = dataBuffer.view(GUAVA_PAYLOAD_OFFSET_V2, dataBuffer.size()); + return onHeap ? new OnHeapGuavaBloomFilterReader(bloomFilterDataBuffer) + : new OffHeapGuavaBloomFilterReader(bloomFilterDataBuffer); + } + // Legacy v1 format: [TYPE_VALUE=1][VERSION][Guava bytes] Preconditions.checkState( typeValue == OnHeapGuavaBloomFilterCreator.TYPE_VALUE && version == OnHeapGuavaBloomFilterCreator.VERSION, "Unsupported bloom filter type value: %s and version: %s", typeValue, version); - PinotDataBuffer bloomFilterDataBuffer = dataBuffer.view(HEADER_SIZE, dataBuffer.size()); + PinotDataBuffer bloomFilterDataBuffer = dataBuffer.view(GUAVA_PAYLOAD_OFFSET_V1, dataBuffer.size()); return onHeap ? new OnHeapGuavaBloomFilterReader(bloomFilterDataBuffer) : new OffHeapGuavaBloomFilterReader(bloomFilterDataBuffer); } diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java index fcb6ab84bd9f..c653c5ab7029 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java @@ -41,7 +41,6 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -52,27 +51,26 @@ /** * Unit tests for {@link BloomFilterHandler} fpp-change detection. * - *

The key invariant being tested: when the bloom filter configuration changes, even if the change does NOT - * alter the number of hash functions (k), it may still change the bit-array size (numLongs). Both dimensions - * must be compared to correctly detect a rebuild requirement. + *

The key invariant being tested: fpp-change detection reads the effective fpp stored directly + * in the v2 bloom filter header, rather than inferring it from Guava-internal fields (numHashFunctions, + * numLongs). This avoids a fragile dependency on Guava's internal serialisation layout. * - *

Concretely, for cardinality=1000: + *

Bloom filter file format versions: *

    - *
  • fpp=0.05 → k=4, numLongs=98
    • - *
  • fpp=0.045 → k=4, numLongs=101
    • + *
  • v1 (legacy): {@code [TYPE_VALUE=1 (int)][VERSION (int)][Guava bytes...]} — fpp not stored.
    • + *
  • v2: {@code [TYPE_VALUE_V2=2 (int)][VERSION (int)][FPP (double)][Guava bytes...]} — fpp stored + * explicitly at byte offset {@link OnHeapGuavaBloomFilterCreator#FPP_OFFSET}.
    • *
- * The old check (numHashFunctions only) silently kept the stale index; the new check catches the difference. + * + *

Legacy v1 segments skip fpp-change detection (return false) because the fpp is absent from the + * header. They are upgraded to v2 the next time the bloom filter is rebuilt for any other reason. */ public class BloomFilterHandlerTest { private static final String COLUMN = "myBloomCol"; private static final int CARDINALITY = 1000; - // With cardinality=1000 and fpp=0.05: k=4, numLongs=98 private static final double FPP_STORED = 0.05; - // With cardinality=1000 and fpp=0.045: k=4 (same!), numLongs=101 (different!) - private static final double FPP_NEW_SAME_K = 0.045; - // With cardinality=1000 and fpp=0.01: k=7 (different from k=4) — sanity check - private static final double FPP_NEW_DIFF_K = 0.01; + private static final double FPP_DIFFERENT = 0.01; private File _tempDir; private File _bloomFilterFile; @@ -91,58 +89,58 @@ public void tearDown() { } /** - * Regression test: fpp=0.05 → 0.045 both yield k=4 but different numLongs (98 vs 101) for cardinality=1000. - * The old check (numHashFunctions only) would return false (no rebuild), leaving a stale index. - * The new check (numHashFunctions + numLongs) must return true (rebuild needed). + * Legacy v1 segments do not carry fpp in the header; fpp-change detection must be skipped regardless + * of what the current config says. The segment will be upgraded to v2 on its next rebuild. */ @Test - public void testNeedUpdateReturnsTrueWhenSameKButDifferentNumLongs() + public void testV1LegacyFileSkipsFppChangeDetection() throws Exception { - writeBloomFilter(CARDINALITY, FPP_STORED); + writeV1BloomFilter(CARDINALITY, FPP_STORED); - BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_NEW_SAME_K, 0, false)); + // Config uses a clearly different fpp, but because the file is v1 the handler must not trigger a rebuild. + BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_DIFFERENT, 0, false)); SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); - assertTrue(handler.needUpdateIndices(reader), - "Rebuild expected: fpp 0.05→0.045 keeps k=4 but changes numLongs (98→101)"); + assertFalse(handler.needUpdateIndices(reader), + "No rebuild expected for legacy v1 segments — fpp is not stored in the header"); } /** - * Sanity check: when fpp is unchanged the handler must not trigger a spurious rebuild. + * Sanity check: when the v2 header fpp matches the current config, the handler must not trigger a rebuild. */ @Test - public void testNeedUpdateReturnsFalseWhenFppUnchanged() + public void testNeedUpdateReturnsFalseWhenFppUnchangedV2() throws Exception { - writeBloomFilter(CARDINALITY, FPP_STORED); + writeV2BloomFilter(CARDINALITY, FPP_STORED); BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_STORED, 0, false)); SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); assertFalse(handler.needUpdateIndices(reader), - "No rebuild expected when fpp is unchanged"); + "No rebuild expected when stored v2 fpp matches the current config"); } /** - * Sanity check: when fpp changes enough to alter k the handler must trigger a rebuild (pre-existing behavior). + * When the v2 header fpp differs from the current config, the handler must trigger a rebuild. */ @Test - public void testNeedUpdateReturnsTrueWhenKChanges() + public void testNeedUpdateReturnsTrueWhenFppChangedV2() throws Exception { - writeBloomFilter(CARDINALITY, FPP_STORED); + writeV2BloomFilter(CARDINALITY, FPP_STORED); - BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_NEW_DIFF_K, 0, false)); + BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_DIFFERENT, 0, false)); SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); assertTrue(handler.needUpdateIndices(reader), - "Rebuild expected: fpp 0.05→0.01 changes k from 4 to 7"); + "Rebuild expected when stored v2 fpp differs from the current config"); } // --- helpers --- /** - * Writes a Pinot-format bloom filter file: [TYPE_VALUE(int)][VERSION(int)][Guava bytes]. + * Writes a legacy v1 Pinot bloom filter file: {@code [TYPE_VALUE=1 (int)][VERSION (int)][Guava bytes]}. */ - private void writeBloomFilter(int cardinality, double fpp) + private void writeV1BloomFilter(int cardinality, double fpp) throws Exception { BloomFilter bf = BloomFilter.create(Funnels.stringFunnel(StandardCharsets.UTF_8), cardinality, fpp); try (DataOutputStream out = new DataOutputStream(new FileOutputStream(_bloomFilterFile))) { @@ -152,6 +150,21 @@ private void writeBloomFilter(int cardinality, double fpp) } } + /** + * Writes a v2 Pinot bloom filter file: + * {@code [TYPE_VALUE_V2=2 (int)][VERSION (int)][FPP (double)][Guava bytes]}. + */ + private void writeV2BloomFilter(int cardinality, double fpp) + throws Exception { + BloomFilter bf = BloomFilter.create(Funnels.stringFunnel(StandardCharsets.UTF_8), cardinality, fpp); + try (DataOutputStream out = new DataOutputStream(new FileOutputStream(_bloomFilterFile))) { + out.writeInt(OnHeapGuavaBloomFilterCreator.TYPE_VALUE_V2); + out.writeInt(OnHeapGuavaBloomFilterCreator.VERSION); + out.writeDouble(fpp); + bf.writeTo(out); + } + } + private BloomFilterHandler createHandler(String columnName, BloomFilterConfig bloomFilterConfig) { FieldIndexConfigs fieldIndexConfigs = new FieldIndexConfigs.Builder().add(StandardIndexes.bloomFilter(), bloomFilterConfig).build(); From 5d7edc527158314df1880d82e812280c450c80e1 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Tue, 7 Jul 2026 12:44:12 +0530 Subject: [PATCH 05/10] Fix BloomFilterHandlerTest: stub getTotalDocs/getAllColumns on mock BaseIndexHandler (upstream master) added a Preconditions.checkState that requires getTotalDocs() > 0. The Mockito mock returned 0 (int default), causing all three tests to fail with IllegalStateException at construction. Also stub getAllColumns() to avoid a NullPointerException in the column-map initialization loop inside BaseIndexHandler. Co-Authored-By: Claude Sonnet 4.6 --- .../index/loader/bloomfilter/BloomFilterHandlerTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java index c653c5ab7029..b12bfda4aa87 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java @@ -175,6 +175,8 @@ private BloomFilterHandler createHandler(String columnName, BloomFilterConfig bl SegmentMetadataImpl segmentMetadata = mock(SegmentMetadataImpl.class); when(segmentMetadata.getName()).thenReturn("testSegment"); + when(segmentMetadata.getTotalDocs()).thenReturn(CARDINALITY); + when(segmentMetadata.getAllColumns()).thenReturn(Set.of(columnName)); when(segmentMetadata.getColumnMetadataFor(columnName)).thenReturn(columnMetadata); SegmentDirectory segmentDirectory = mock(SegmentDirectory.class); From b3976770fd1dd7f73eb1d9f0333c1ab06a303c8e Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Tue, 7 Jul 2026 15:14:47 +0530 Subject: [PATCH 06/10] Fix BloomFilterHandlerTest: use TreeSet for getAllColumns() mock SegmentMetadataImpl.getAllColumns() returns NavigableSet in upstream master. Set.of() returns a plain Set which is not assignable to NavigableSet, causing a compilation failure. Use TreeSet instead. Co-Authored-By: Claude Sonnet 4.6 --- .../index/loader/bloomfilter/BloomFilterHandlerTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java index b12bfda4aa87..9e0329b01907 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java @@ -26,6 +26,7 @@ import java.nio.charset.StandardCharsets; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import org.apache.commons.io.FileUtils; import org.apache.pinot.segment.local.segment.creator.impl.bloom.OnHeapGuavaBloomFilterCreator; import org.apache.pinot.segment.spi.ColumnMetadata; @@ -176,7 +177,7 @@ private BloomFilterHandler createHandler(String columnName, BloomFilterConfig bl SegmentMetadataImpl segmentMetadata = mock(SegmentMetadataImpl.class); when(segmentMetadata.getName()).thenReturn("testSegment"); when(segmentMetadata.getTotalDocs()).thenReturn(CARDINALITY); - when(segmentMetadata.getAllColumns()).thenReturn(Set.of(columnName)); + when(segmentMetadata.getAllColumns()).thenReturn(new TreeSet<>(Set.of(columnName))); when(segmentMetadata.getColumnMetadataFor(columnName)).thenReturn(columnMetadata); SegmentDirectory segmentDirectory = mock(SegmentDirectory.class); From e3d86210141c63740580e1b40f5c492e59032ec0 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Thu, 9 Jul 2026 21:11:24 +0530 Subject: [PATCH 07/10] Address review: use VERSION_V2=2 instead of TYPE_VALUE_V2 for new bloom filter format Per @J-HowHuang's feedback: keep TYPE_VALUE=1 unchanged for all bloom filters; distinguish new format by bumping VERSION from 1 to 2 (VERSION_V2=2) rather than changing the type byte. Handler and reader factory now check version at offset 4 instead of type at offset 0 to detect v2 presence of stored fpp. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .../impl/bloom/OnHeapGuavaBloomFilterCreator.java | 15 +++++++-------- .../loader/bloomfilter/BloomFilterHandler.java | 7 ++++--- .../readers/bloom/BloomFilterReaderFactory.java | 8 ++++---- .../bloomfilter/BloomFilterHandlerTest.java | 8 ++++---- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java index 30db2c168221..7749a6ffe38f 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java @@ -43,17 +43,16 @@ public class OnHeapGuavaBloomFilterCreator implements BloomFilterCreator { private static final Logger LOGGER = LoggerFactory.getLogger(OnHeapGuavaBloomFilterCreator.class); - /** Legacy format: {@code [TYPE_VALUE=1 (int)][VERSION (int)][Guava bytes...]} — no fpp in header. */ public static final int TYPE_VALUE = 1; - /** - * V2 format: {@code [TYPE_VALUE_V2=2 (int)][VERSION (int)][effective FPP (double)][Guava bytes...]}. + /** Legacy v1 format: {@code [TYPE_VALUE=1 (int)][VERSION=1 (int)][Guava bytes...]} — no fpp in header. */ + public static final int VERSION = 1; + /** V2 format: {@code [TYPE_VALUE=1 (int)][VERSION_V2=2 (int)][effective FPP (double)][Guava bytes...]}. * The effective fpp (after applying any {@code maxSizeInBytes} cap) is stored at byte offset 8 so that * {@link org.apache.pinot.segment.local.segment.index.loader.bloomfilter.BloomFilterHandler} can compare it * directly without depending on Guava's internal serialisation layout. */ - public static final int TYPE_VALUE_V2 = 2; - public static final int VERSION = 1; - /** Byte offset of the fpp field in a v2 bloom filter file (after TYPE_VALUE_V2 + VERSION). */ + public static final int VERSION_V2 = 2; + /** Byte offset of the fpp field in a v2 bloom filter file (after TYPE_VALUE + VERSION_V2). */ public static final int FPP_OFFSET = 8; private final File _bloomFilterFile; @@ -99,8 +98,8 @@ public void add(String value) { public void seal() throws IOException { try (DataOutputStream out = new DataOutputStream(new FileOutputStream(_bloomFilterFile))) { - out.writeInt(TYPE_VALUE_V2); - out.writeInt(VERSION); + out.writeInt(TYPE_VALUE); + out.writeInt(VERSION_V2); out.writeDouble(_effectiveFpp); _bloomFilter.writeTo(out); } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java index 6953f0bc62fc..48e0e300995a 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java @@ -57,6 +57,7 @@ public class BloomFilterHandler extends BaseIndexHandler { private static final Logger LOGGER = LoggerFactory.getLogger(BloomFilterHandler.class); private static final int TYPE_VALUE_OFFSET = 0; + private static final int VERSION_OFFSET = 4; private final Map _bloomFilterConfigs; @@ -102,7 +103,7 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) { * {@link OnHeapGuavaBloomFilterCreator#FPP_OFFSET} so comparisons are exact and do not depend on Guava's * internal serialisation layout. * - *

Legacy v1 segments (TYPE_VALUE = 1) do not carry fpp in the header; fpp-change detection is skipped + *

Legacy v1 segments (VERSION = 1) do not carry fpp in the header; fpp-change detection is skipped * for those segments. They will be upgraded to v2 the next time the bloom filter is rebuilt for any reason. * *

Accepts both Reader and Writer since Writer extends Reader, allowing this method to be called from @@ -111,8 +112,8 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) { private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segmentName, String column) { try { PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column, StandardIndexes.bloomFilter()); - int typeValue = dataBuffer.getInt(TYPE_VALUE_OFFSET); - if (typeValue != OnHeapGuavaBloomFilterCreator.TYPE_VALUE_V2) { + int version = dataBuffer.getInt(VERSION_OFFSET); + if (version < OnHeapGuavaBloomFilterCreator.VERSION_V2) { // Legacy v1 format — fpp is not stored in the header; skip fpp-change detection. // The index will be upgraded to v2 the next time it is rebuilt. return false; diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java index 623068f7ec94..467972ab56b4 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/bloom/BloomFilterReaderFactory.java @@ -34,16 +34,16 @@ private BloomFilterReaderFactory() { private static final int GUAVA_PAYLOAD_OFFSET_V1 = 8; /** * Byte offset at which Guava bloom filter bytes start in the v2 format. - * V2 header: [TYPE_VALUE_V2 (int, 4)][VERSION (int, 4)][FPP (double, 8)] = 16 bytes. + * V2 header: [TYPE_VALUE (int, 4)][VERSION_V2 (int, 4)][FPP (double, 8)] = 16 bytes. */ private static final int GUAVA_PAYLOAD_OFFSET_V2 = 16; public static BloomFilterReader getBloomFilterReader(PinotDataBuffer dataBuffer, boolean onHeap) { int typeValue = dataBuffer.getInt(TYPE_VALUE_OFFSET); int version = dataBuffer.getInt(VERSION_OFFSET); - if (typeValue == OnHeapGuavaBloomFilterCreator.TYPE_VALUE_V2 - && version == OnHeapGuavaBloomFilterCreator.VERSION) { - // V2 format: [TYPE_VALUE_V2][VERSION][FPP (double)][Guava bytes] + if (typeValue == OnHeapGuavaBloomFilterCreator.TYPE_VALUE + && version == OnHeapGuavaBloomFilterCreator.VERSION_V2) { + // V2 format: [TYPE_VALUE][VERSION_V2][FPP (double)][Guava bytes] PinotDataBuffer bloomFilterDataBuffer = dataBuffer.view(GUAVA_PAYLOAD_OFFSET_V2, dataBuffer.size()); return onHeap ? new OnHeapGuavaBloomFilterReader(bloomFilterDataBuffer) : new OffHeapGuavaBloomFilterReader(bloomFilterDataBuffer); diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java index 9e0329b01907..96d90ea62605 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java @@ -59,7 +59,7 @@ *

Bloom filter file format versions: *

    *
  • v1 (legacy): {@code [TYPE_VALUE=1 (int)][VERSION (int)][Guava bytes...]} — fpp not stored.
    • - *
  • v2: {@code [TYPE_VALUE_V2=2 (int)][VERSION (int)][FPP (double)][Guava bytes...]} — fpp stored + *
  • v2: {@code [TYPE_VALUE=1 (int)][VERSION_V2=2 (int)][FPP (double)][Guava bytes...]} — fpp stored * explicitly at byte offset {@link OnHeapGuavaBloomFilterCreator#FPP_OFFSET}.
    • *
* @@ -153,14 +153,14 @@ private void writeV1BloomFilter(int cardinality, double fpp) /** * Writes a v2 Pinot bloom filter file: - * {@code [TYPE_VALUE_V2=2 (int)][VERSION (int)][FPP (double)][Guava bytes]}. + * {@code [TYPE_VALUE=1 (int)][VERSION_V2=2 (int)][FPP (double)][Guava bytes]}. */ private void writeV2BloomFilter(int cardinality, double fpp) throws Exception { BloomFilter bf = BloomFilter.create(Funnels.stringFunnel(StandardCharsets.UTF_8), cardinality, fpp); try (DataOutputStream out = new DataOutputStream(new FileOutputStream(_bloomFilterFile))) { - out.writeInt(OnHeapGuavaBloomFilterCreator.TYPE_VALUE_V2); - out.writeInt(OnHeapGuavaBloomFilterCreator.VERSION); + out.writeInt(OnHeapGuavaBloomFilterCreator.TYPE_VALUE); + out.writeInt(OnHeapGuavaBloomFilterCreator.VERSION_V2); out.writeDouble(fpp); bf.writeTo(out); } From 9fa6594c82232dbdeb2b197aa36a4146ed0a0684 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Sun, 12 Jul 2026 17:10:47 +0530 Subject: [PATCH 08/10] ci: re-run compat tests From 04ede8930cf4f428fd972bfac45788d5a10bc2a2 Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Sun, 12 Jul 2026 18:37:36 +0530 Subject: [PATCH 09/10] fix(bloom-filter): restore V1 write format to fix backward-compat break MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing VERSION_V2=2 in seal() caused old servers (release-1.5.0 and earlier) to throw 'Unsupported bloom filter type value: 1 and version: 2' on segment load, failing the compat regression test. Revert seal() to write VERSION=1 (V1 format) so all servers can read newly created segments. FPP change detection now works structurally: isFppChanged reads numHashFunctions (1 byte at offset 9) and numLongs (4-byte int at offset 10) from the Guava payload and compares them to the values Guava would compute for the configured fpp+cardinality using the same formulas (optimalNumOfBits / optimalNumOfHashFunctions). Key fix in computeExpectedNumHashFunctionsAndNumLongs: numHashFunctions must be derived from idealNumBits (pre-quantization), not from allocatedNumBits (quantized to multiple of 64) — Guava computes them independently, so using allocatedNumBits would produce wrong k values when idealNumBits is not a multiple of 64. Additional fixes: - Guard against cardinality=0 in computeEffectiveFpp when maxSizeInBytes>0 - Force rebuild for unrecognized versions instead of silently reading bytes at wrong offsets - Keep V2 reader in BloomFilterReaderFactory for any already-written V2 segments; VERSION_V2 constant retained with updated Javadoc --- .../bloom/OnHeapGuavaBloomFilterCreator.java | 16 ++-- .../bloomfilter/BloomFilterHandler.java | 95 +++++++++++++++---- .../bloomfilter/BloomFilterHandlerTest.java | 46 +++++---- 3 files changed, 113 insertions(+), 44 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java index 7749a6ffe38f..9e844408d7f4 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java @@ -44,12 +44,15 @@ public class OnHeapGuavaBloomFilterCreator implements BloomFilterCreator { private static final Logger LOGGER = LoggerFactory.getLogger(OnHeapGuavaBloomFilterCreator.class); public static final int TYPE_VALUE = 1; - /** Legacy v1 format: {@code [TYPE_VALUE=1 (int)][VERSION=1 (int)][Guava bytes...]} — no fpp in header. */ + /** V1 format (current write format): {@code [TYPE_VALUE=1 (int)][VERSION=1 (int)][Guava bytes...]}. */ public static final int VERSION = 1; - /** V2 format: {@code [TYPE_VALUE=1 (int)][VERSION_V2=2 (int)][effective FPP (double)][Guava bytes...]}. - * The effective fpp (after applying any {@code maxSizeInBytes} cap) is stored at byte offset 8 so that - * {@link org.apache.pinot.segment.local.segment.index.loader.bloomfilter.BloomFilterHandler} can compare it - * directly without depending on Guava's internal serialisation layout. + /** + * V2 format: {@code [TYPE_VALUE=1 (int)][VERSION_V2=2 (int)][effective FPP (double)][Guava bytes...]}. + * This format was introduced to embed the effective FPP in the header for change detection, but it broke + * backward compatibility because release-1.5.0 and older servers throw + * {@code IllegalStateException: Unsupported bloom filter type value: 1 and version: 2} on load. + * New segments are written in V1 format; this constant is kept so the reader can still handle any + * V2 segments that were created during the short window when V2 was the write format. */ public static final int VERSION_V2 = 2; /** Byte offset of the fpp field in a v2 bloom filter file (after TYPE_VALUE + VERSION_V2). */ @@ -99,8 +102,7 @@ public void seal() throws IOException { try (DataOutputStream out = new DataOutputStream(new FileOutputStream(_bloomFilterFile))) { out.writeInt(TYPE_VALUE); - out.writeInt(VERSION_V2); - out.writeDouble(_effectiveFpp); + out.writeInt(VERSION); _bloomFilter.writeTo(out); } } diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java index 48e0e300995a..aa504d816ab8 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java @@ -58,6 +58,11 @@ public class BloomFilterHandler extends BaseIndexHandler { private static final int TYPE_VALUE_OFFSET = 0; private static final int VERSION_OFFSET = 4; + // In V1 format the Guava payload starts at byte 8. Within that payload the layout is: + // [strategy (1 byte)][numHashFunctions (1 byte)][numLongs (int, 4 bytes)][bit array...] + // These absolute file offsets match BaseGuavaBloomFilterReader's relative offsets + 8. + private static final int V1_NUM_HASH_FUNCTIONS_OFFSET = 9; // 4 (TYPE) + 4 (VERSION) + 1 (strategy); reads 1 byte + private static final int V1_NUM_LONGS_OFFSET = 10; // + 1 (numHashFunctions); reads 4-byte int private final Map _bloomFilterConfigs; @@ -96,15 +101,16 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) { } /** - * Checks whether the effective fpp config has changed by comparing the fpp stored in the v2 bloom filter - * header with the effective fpp derived from the current {@link BloomFilterConfig}. + * Checks whether the effective fpp config has changed for an existing bloom filter index. * - *

The v2 format stores the effective fpp (post {@code maxSizeInBytes} cap) explicitly at byte offset - * {@link OnHeapGuavaBloomFilterCreator#FPP_OFFSET} so comparisons are exact and do not depend on Guava's - * internal serialisation layout. + *

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. * - *

Legacy v1 segments (VERSION = 1) do not carry fpp in the header; fpp-change detection is skipped - * for those segments. They will be upgraded to v2 the next time the bloom filter is rebuilt for any reason. + *

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. * *

Accepts both Reader and Writer since Writer extends Reader, allowing this method to be called from * both needUpdateIndices and updateIndices. @@ -113,19 +119,40 @@ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segme try { PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column, StandardIndexes.bloomFilter()); int version = dataBuffer.getInt(VERSION_OFFSET); - if (version < OnHeapGuavaBloomFilterCreator.VERSION_V2) { - // Legacy v1 format — fpp is not stored in the header; skip fpp-change detection. - // The index will be upgraded to v2 the next time it is rebuilt. - return false; - } - double storedFpp = dataBuffer.getDouble(OnHeapGuavaBloomFilterCreator.FPP_OFFSET); BloomFilterConfig config = _bloomFilterConfigs.get(column); ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column); - 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); + + 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; @@ -135,6 +162,32 @@ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segme } } + /** + * Computes the {@code numHashFunctions} and {@code numLongs} that Guava would allocate for the given + * fpp and cardinality. These match Guava's {@code optimalNumOfBits} / {@code optimalNumOfHashFunctions} + * formulas and are used to detect fpp changes in V1 bloom filter files. + * + * @return {@code int[]{numHashFunctions, numLongs}} + */ + private int[] computeExpectedNumHashFunctionsAndNumLongs(ColumnMetadata columnMetadata, + BloomFilterConfig bloomFilterConfig) { + double fpp = computeEffectiveFpp(columnMetadata, bloomFilterConfig); + int cardinality = columnMetadata != null ? columnMetadata.getCardinality() : 1; + if (cardinality <= 0) { + cardinality = columnMetadata != null ? columnMetadata.getTotalNumberOfEntries() : 1; + } + if (cardinality <= 0) { + cardinality = 1; + } + // Guava: optimalNumOfBits = -n * ln(p) / (ln 2)^2, minimum 1 + long idealNumBits = Math.max(1, (long) (-cardinality * Math.log(fpp) / (Math.log(2) * Math.log(2)))); + // Guava: numLongs = ceil(idealNumBits / 64). numHashFunctions uses idealNumBits, not the quantized count. + int numLongs = (int) ((idealNumBits + 63) / 64); + // Guava: optimalNumOfHashFunctions(n, idealNumBits) = max(1, round(idealNumBits / n * ln 2)) + int numHashFunctions = Math.max(1, (int) Math.round((double) idealNumBits / cardinality * Math.log(2))); + return new int[]{numHashFunctions, numLongs}; + } + /** * Computes the effective fpp for a new bloom filter: applies the {@code maxSizeInBytes} cap if set. * This matches the fpp computation in {@link OnHeapGuavaBloomFilterCreator}. @@ -147,8 +200,10 @@ private double computeEffectiveFpp(ColumnMetadata columnMetadata, BloomFilterCon if (cardinality <= 0) { cardinality = columnMetadata.getTotalNumberOfEntries(); } - double minFpp = GuavaBloomFilterReaderUtils.computeFPP(maxSizeInBytes, cardinality); - fpp = Math.max(fpp, minFpp); + if (cardinality > 0) { + double minFpp = GuavaBloomFilterReaderUtils.computeFPP(maxSizeInBytes, cardinality); + fpp = Math.max(fpp, minFpp); + } } return fpp; } diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java index 96d90ea62605..1819eace700d 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java @@ -52,19 +52,17 @@ /** * Unit tests for {@link BloomFilterHandler} fpp-change detection. * - *

The key invariant being tested: fpp-change detection reads the effective fpp stored directly - * in the v2 bloom filter header, rather than inferring it from Guava-internal fields (numHashFunctions, - * numLongs). This avoids a fragile dependency on Guava's internal serialisation layout. - * *

Bloom filter file format versions: *

    - *
  • v1 (legacy): {@code [TYPE_VALUE=1 (int)][VERSION (int)][Guava bytes...]} — fpp not stored.
    • - *
  • v2: {@code [TYPE_VALUE=1 (int)][VERSION_V2=2 (int)][FPP (double)][Guava bytes...]} — fpp stored - * explicitly at byte offset {@link OnHeapGuavaBloomFilterCreator#FPP_OFFSET}.
    • + *
  • v1 (current write format): + * {@code [TYPE_VALUE=1 (int)][VERSION=1 (int)][Guava bytes...]} — fpp not stored explicitly. + * Change detection reads {@code numHashFunctions} and {@code numLongs} from the Guava payload + * at fixed offsets and compares them to the values computed from the configured fpp + cardinality.
    • + *
  • v2 (legacy, no longer written): + * {@code [TYPE_VALUE=1 (int)][VERSION_V2=2 (int)][FPP (double)][Guava bytes...]} — fpp stored + * explicitly at byte offset {@link OnHeapGuavaBloomFilterCreator#FPP_OFFSET}. + * V2 reading is retained for segments created during the short window when V2 was the write format.
    • *
- * - *

Legacy v1 segments skip fpp-change detection (return false) because the fpp is absent from the - * header. They are upgraded to v2 the next time the bloom filter is rebuilt for any other reason. */ public class BloomFilterHandlerTest { private static final String COLUMN = "myBloomCol"; @@ -90,20 +88,34 @@ public void tearDown() { } /** - * Legacy v1 segments do not carry fpp in the header; fpp-change detection must be skipped regardless - * of what the current config says. The segment will be upgraded to v2 on its next rebuild. + * V1 file with the same fpp as the current config: numHashFunctions and numLongs match, no rebuild needed. */ @Test - public void testV1LegacyFileSkipsFppChangeDetection() + public void testNeedUpdateReturnsFalseWhenFppUnchangedV1() throws Exception { writeV1BloomFilter(CARDINALITY, FPP_STORED); - // Config uses a clearly different fpp, but because the file is v1 the handler must not trigger a rebuild. - BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_DIFFERENT, 0, false)); + BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_STORED, 0, false)); SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); assertFalse(handler.needUpdateIndices(reader), - "No rebuild expected for legacy v1 segments — fpp is not stored in the header"); + "No rebuild expected when stored v1 numHashFunctions/numLongs match the current config"); + } + + /** + * V1 file with a different fpp from the current config: numHashFunctions or numLongs differ, + * rebuild must be triggered. + */ + @Test + public void testNeedUpdateReturnsTrueWhenFppChangedV1() + throws Exception { + writeV1BloomFilter(CARDINALITY, FPP_STORED); + + BloomFilterHandler handler = createHandler(COLUMN, new BloomFilterConfig(FPP_DIFFERENT, 0, false)); + SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); + + assertTrue(handler.needUpdateIndices(reader), + "Rebuild expected when v1 numHashFunctions/numLongs do not match the current config"); } /** @@ -139,7 +151,7 @@ public void testNeedUpdateReturnsTrueWhenFppChangedV2() // --- helpers --- /** - * Writes a legacy v1 Pinot bloom filter file: {@code [TYPE_VALUE=1 (int)][VERSION (int)][Guava bytes]}. + * Writes a v1 Pinot bloom filter file: {@code [TYPE_VALUE=1 (int)][VERSION=1 (int)][Guava bytes]}. */ private void writeV1BloomFilter(int cardinality, double fpp) throws Exception { From c65b55ba346b6c97d1306c53b71b9697cb739e2b Mon Sep 17 00:00:00 2001 From: Akanksha Kedia Date: Mon, 13 Jul 2026 11:39:34 +0530 Subject: [PATCH 10/10] fix(bloom-filter): use Guava 33.5+ direct formula for numHashFunctions detection In Guava 33.5, optimalNumOfHashFunctions was changed from the two-argument form (n, m) to a single-argument form (p): max(1, round(-ln(p)/ln(2))). The old two-step formula via idealNumBits/n*ln(2) diverges from what Guava actually writes for small cardinalities (e.g. n=1, p=0.01 gives k=6 vs Guava's k=7), causing isFppChanged() to always return true and triggering an infinite rebuild loop on every segment reload. Update computeExpectedNumHashFunctionsAndNumLongs to use the direct formula matching the runtime Guava version (33.5.0-jre). Add a small-cardinality unit test (n=1, p=0.01) to BloomFilterHandlerTest as a regression guard. --- .../bloomfilter/BloomFilterHandler.java | 16 ++++++---- .../bloomfilter/BloomFilterHandlerTest.java | 29 +++++++++++++++++-- 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java index aa504d816ab8..5c191c2164bc 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java @@ -164,8 +164,12 @@ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segme /** * Computes the {@code numHashFunctions} and {@code numLongs} that Guava would allocate for the given - * fpp and cardinality. These match Guava's {@code optimalNumOfBits} / {@code optimalNumOfHashFunctions} - * formulas and are used to detect fpp changes in V1 bloom filter files. + * fpp and cardinality, matching the formulas in Guava's {@code BloomFilter}. + * + *

{@code numLongs} uses {@code optimalNumOfBits(n, p) = (long)(-n * ln(p) / (ln 2)^2)}. + * {@code numHashFunctions} uses the direct formula {@code max(1, round(-ln(p) / ln(2)))}, + * which is what current Guava (33+) writes. The older two-step path via {@code idealNumBits / n * ln(2)} + * diverges for small cardinalities due to long-cast truncation and must not be used. * * @return {@code int[]{numHashFunctions, numLongs}} */ @@ -181,10 +185,12 @@ private int[] computeExpectedNumHashFunctionsAndNumLongs(ColumnMetadata columnMe } // Guava: optimalNumOfBits = -n * ln(p) / (ln 2)^2, minimum 1 long idealNumBits = Math.max(1, (long) (-cardinality * Math.log(fpp) / (Math.log(2) * Math.log(2)))); - // Guava: numLongs = ceil(idealNumBits / 64). numHashFunctions uses idealNumBits, not the quantized count. + // Guava: numLongs = ceil(idealNumBits / 64). int numLongs = (int) ((idealNumBits + 63) / 64); - // Guava: optimalNumOfHashFunctions(n, idealNumBits) = max(1, round(idealNumBits / n * ln 2)) - int numHashFunctions = Math.max(1, (int) Math.round((double) idealNumBits / cardinality * Math.log(2))); + // Guava (33+): optimalNumOfHashFunctions(p) = max(1, round(-ln(p) / ln(2))) + // This is the direct formula Guava now uses; it avoids the long-cast quantization error + // in the old two-step path (via idealNumBits / n) that diverges for small cardinalities. + int numHashFunctions = Math.max(1, (int) Math.round(-Math.log(fpp) / Math.log(2))); return new int[]{numHashFunctions, numLongs}; } diff --git a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java index 1819eace700d..b18ab68dfa60 100644 --- a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java +++ b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandlerTest.java @@ -118,6 +118,24 @@ public void testNeedUpdateReturnsTrueWhenFppChangedV1() "Rebuild expected when v1 numHashFunctions/numLongs do not match the current config"); } + /** + * Regression guard for the small-cardinality formula bug: with n=1 and fpp=0.01, the formula must produce + * numHashFunctions=7 to match Guava 33.5+'s {@code optimalNumOfHashFunctions(p) = round(-ln(p)/ln(2))}. + * An older two-step formula via {@code idealNumBits/n * ln(2)} gives 6, causing an infinite rebuild loop. + */ + @Test + public void testNeedUpdateReturnsFalseWhenFppUnchangedV1SmallCardinality() + throws Exception { + // n=1, p=0.01 → Guava 33.5+ writes numHashFunctions=7 (round(-ln(0.01)/ln(2)) = round(6.64) = 7) + writeV1BloomFilter(1, 0.01); + + BloomFilterHandler handler = createHandlerWithCardinality(COLUMN, new BloomFilterConfig(0.01, 0, false), 1); + SegmentDirectory.Reader reader = mockReaderWithBloomFilter(COLUMN, _bloomFilterFile); + + assertFalse(handler.needUpdateIndices(reader), + "No rebuild expected for small cardinality (n=1) when fpp is unchanged — formula must match Guava exactly"); + } + /** * Sanity check: when the v2 header fpp matches the current config, the handler must not trigger a rebuild. */ @@ -179,16 +197,21 @@ private void writeV2BloomFilter(int cardinality, double fpp) } private BloomFilterHandler createHandler(String columnName, BloomFilterConfig bloomFilterConfig) { + return createHandlerWithCardinality(columnName, bloomFilterConfig, CARDINALITY); + } + + private BloomFilterHandler createHandlerWithCardinality(String columnName, BloomFilterConfig bloomFilterConfig, + int cardinality) { FieldIndexConfigs fieldIndexConfigs = new FieldIndexConfigs.Builder().add(StandardIndexes.bloomFilter(), bloomFilterConfig).build(); ColumnMetadata columnMetadata = mock(ColumnMetadata.class); - when(columnMetadata.getCardinality()).thenReturn(CARDINALITY); - when(columnMetadata.getTotalNumberOfEntries()).thenReturn(CARDINALITY); + when(columnMetadata.getCardinality()).thenReturn(cardinality); + when(columnMetadata.getTotalNumberOfEntries()).thenReturn(cardinality); SegmentMetadataImpl segmentMetadata = mock(SegmentMetadataImpl.class); when(segmentMetadata.getName()).thenReturn("testSegment"); - when(segmentMetadata.getTotalDocs()).thenReturn(CARDINALITY); + when(segmentMetadata.getTotalDocs()).thenReturn(cardinality); when(segmentMetadata.getAllColumns()).thenReturn(new TreeSet<>(Set.of(columnName))); when(segmentMetadata.getColumnMetadataFor(columnName)).thenReturn(columnMetadata);