You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Akanksha-kedia (via GitHub)" <gi...@apache.org> on 2026/07/01 09:26:24 UTC
[PR] Rebuild bloom filter index when fpp config changes [pinot]
Akanksha-kedia opened a new pull request, #18898:
URL: https://github.com/apache/pinot/pull/18898
## Description
When a user changes the `fpp` (false positive probability) config for a bloom filter index, Pinot previously did NOT detect the change and would not rebuild the index. Users had to:
1. Remove the bloom filter config
2. Reload table
3. Re-add bloom filter config with new fpp
4. Reload table again
This PR adds fpp change detection to `BloomFilterHandler`, following the same pattern used for H3 index resolution detection (PR #16953). The detection works by comparing the number of hash functions stored in the existing bloom filter with what the new fpp config would produce (given the column's cardinality). If they differ, the bloom filter is removed and recreated with the updated config.
### Changes Made
- Modified `BloomFilterHandler.needUpdateIndices()` to check for fpp config changes on existing bloom filter columns
- Modified `BloomFilterHandler.updateIndices()` to remove and rebuild bloom filters when fpp config has changed
- Added `isFppChanged()` helper that reads `numHashFunctions` from the existing bloom filter data buffer
- Added `computeExpectedNumHashFunctions()` that mirrors Guava's BloomFilter formula to compute the expected number of hash functions from fpp and cardinality
## Related Issue
Fixes #17137
## Upgrade Notes
None. This is a purely additive behavior change - bloom filter indexes will now be automatically rebuilt when fpp config changes, instead of silently keeping the old index.
## Testing Done
- [x] Unit tests added: `SegmentPreProcessorTest#testBloomFilterFppUpdate` (tests both v1 and v3 segment formats)
- [x] Test verifies: creating bloom filter with fpp=0.1, confirming no processing needed, changing fpp to 0.01, confirming processing IS needed, rebuilding, and confirming no further processing needed
- [x] All existing bloom filter tests pass
- [x] Checkstyle, spotless, and license checks pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-5236287622
Hi @J-HowHuang — thank you for the detailed suggestion! Your proposed approach has been implemented in the current revision.
The PR now uses a V2 header layout exactly as you described, with the effective `fpp` stored at `FPP_OFFSET` immediately after the version field:
```
+------------------+---------------+--------------+-----------------------------+
| TYPE_VALUE (int) | VERSION (int) | FPP (double) | Guava bloom filter bytes... |
+------------------+---------------+--------------+-----------------------------+
```
All new bloom filter segments are written with `VERSION_V2 = 2`. The reader checks the version to decide which path to take:
- **V2 segments**: read `fpp` directly from the header at `FPP_OFFSET` and compare with the configured value — no Guava internals involved.
- **V1 legacy segments**: fpp change detection is skipped (graceful degradation); the segment upgrades to V2 naturally on the next rebuild triggered by other criteria.
Regarding the broader consistency point raised in apache/pinot#18920: we've also added `readStoredIndexConfig()` / `setStoredIndexConfig()` utilities to `BaseIndexHandler` as a uniform `metadata.properties`-based alternative for index types that prefer not to embed config in their binary format. For the bloom filter, the header approach you suggested is the cleaner choice since it keeps the config self-contained in the index file itself — so this PR stays with the V2 header.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4861981518
## Review: Rebuild bloom filter on fpp change
The overall approach is sound and consistent with how `RangeIndexHandler` detects version changes. **3 MAJOR issues** need to be addressed before merging, including one that causes an infinite rebuild loop.
---
### CRITICAL — Formula integer truncation causes infinite rebuild loop
`BloomFilterHandler.java`, `computeExpectedNumHashFunctions()`
The PR computes `k` via an intermediate `(long)` truncation of `optimalNumOfBits`, but Guava's `optimalNumOfHashFunctions` computes `k` directly from `fpp` only:
```java
// Guava BloomFilter.java
static int optimalNumOfHashFunctions(double p) {
return max(1, (int) Math.round(-Math.log(p) / LOG_TWO));
}
```
The integer truncation causes mismatches at small cardinalities: `cardinality=1, fpp=0.01` → Guava writes `k=7`, PR computes expected `k=6` → every reload triggers rebuild → **infinite rebuild loop**.
Fix — use Guava's direct formula:
```java
return Math.max(1, (int) Math.round(-Math.log(fpp) / Math.log(2)));
```
---
### MAJOR — `updateIndices` duplicates ~20 lines from `isFppChanged` instead of calling the helper
`BloomFilterHandler.java`, lines 158–183. `RangeIndexHandler` passes `segmentWriter` (a `Writer extends Reader`) directly to the helper that accepts a `Reader` — same pattern should be used here to avoid duplicated logic that can silently diverge.
---
### MAJOR — Version guard missing before reading at byte offset 9
Without first verifying `VERSION == OnHeapGuavaBloomFilterCreator.VERSION`, a future format change silently reads garbage. Add:
```java
int version = dataBuffer.getInt(4);
if (version != OnHeapGuavaBloomFilterCreator.VERSION) {
LOGGER.warn("Unexpected bloom filter version {} for segment/column {}/{}", version, segmentName, column);
return false;
}
```
---
### MAJOR — Test does not exercise the formula bug; needs a small-cardinality case
`SegmentPreProcessorTest.java`, `testBloomFilterFppUpdate` uses `column3` which has high cardinality. The truncation mismatch only manifests for `cardinality ∈ {1, 3}` with `fpp=0.01`, so the test passes today despite the bug. Please add a test with a cardinality-1 column verifying: (1) unchanged fpp → no rebuild, (2) changed fpp → rebuild triggered, (3) after rebuild → no rebuild (idempotency).
---
### MINOR — `maxSizeInBytes > 0` path not covered by the new test
New test uses `BloomFilterConfig(0.1, 0, false)` (no size cap). A test that changes `maxSizeInBytes` such that the effective `fpp` and `k` change would cover the `GuavaBloomFilterReaderUtils.computeFPP` branch.
---
### Byte-offset correctness (verified — no issue)
`NUM_HASH_FUNCTIONS_OFFSET = 9` is correct: 4 bytes `TYPE_VALUE` + 4 bytes `VERSION` + 1 byte Guava strategy ordinal + 1 byte `numHashFunctions`. Consistent with `BloomFilterReaderFactory.HEADER_SIZE=8` and `BaseGuavaBloomFilterReader.NUM_HASH_FUNCTIONS_OFFSET=1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4900299182
Addressed @xiangfu0's review feedback: now comparing both `numHashFunctions` and `numLongs` in `isFppChanged`. Added `NUM_LONGS_OFFSET = 10`, `computeExpectedNumLongs()` (Guava's `ceil(numBits/64)` formula), and extracted `effectiveFpp()` to deduplicate the fpp-capping logic. Rebuild is triggered when either dimension differs. Added `BloomFilterHandlerTest` with a same-k regression test: fpp 0.05→0.045 at cardinality=1000 keeps k=4 but changes numLongs from 98→101, confirming the stale-index case is now caught. Ready for re-review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-5109628497
@J-HowHuang Can you review and sign off on this?
@Akanksha-kedia Could you please respond to @J-HowHuang 's comments if they are addressed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3547375507
##########
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;
Review Comment:
I think if we're using the same `OnHeapGuavaBloomFilterCreator`, then it makes more sense to keep the same `TYPE_VALUE` and increase a new `VERSION`. In the factory, read from a different buffer offset based on which version it is.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/bloom/OnHeapGuavaBloomFilterCreator.java:
##########
@@ -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);
Review Comment:
Is there any case that an original v1 index would be preferred? Or is it okay to make every newly created index v2?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4889223672
Hi @Jackie-Jiang @xiangfu0 — this PR fixes a CRITICAL correctness bug in the bloom filter index rebuild logic. The existing formula used an incorrect computation that caused a mismatch between the stored and expected number of hash functions, resulting in an infinite rebuild loop for small-cardinality columns. All CI checks are green. Would appreciate a review when you have a chance!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4951155439
@xiangfu0 @shounakmk219 would appreciate a review when you get a chance. This PR makes the bloom filter handler detect FPP changes (by persisting the configured FPP in a metadata sidecar file) and rebuild the index when the value changes, consistent with how the vector index handler detects config drift.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4965123093
@xiangfu0 @gortiz — would appreciate a review when you get a chance 🙏
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "J-HowHuang (via GitHub)" <gi...@apache.org>.
J-HowHuang commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3581823158
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java:
##########
@@ -85,6 +100,120 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) {
return false;
}
+ /**
+ * Checks whether the effective fpp config has changed for an existing bloom filter index.
+ *
+ * <p>V2 segments (VERSION_V2 = 2) store the effective fpp explicitly in the header; the comparison is exact.
+ * V2 segments were created during a short window before the format was rolled back for backward compatibility;
+ * V2 reading is retained so those segments are not permanently stuck.
+ *
+ * <p>V1 segments (VERSION = 1, the current write format) do not store fpp in the header. FPP change is
+ * detected structurally: the stored {@code numHashFunctions} and {@code numLongs} in the Guava payload
+ * are compared to the values that would be produced by the configured fpp + cardinality. These two fields
+ * jointly encode the filter capacity and are stable across Guava versions.
+ *
+ * <p>Accepts both Reader and Writer since Writer extends Reader, allowing this method to be called from
+ * both needUpdateIndices and updateIndices.
+ */
+ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segmentName, String column) {
+ try {
+ PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column, StandardIndexes.bloomFilter());
+ int version = dataBuffer.getInt(VERSION_OFFSET);
+ BloomFilterConfig config = _bloomFilterConfigs.get(column);
+ ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+
+ if (version == OnHeapGuavaBloomFilterCreator.VERSION_V2) {
+ // V2 header stores the effective fpp explicitly.
+ double storedFpp = dataBuffer.getDouble(OnHeapGuavaBloomFilterCreator.FPP_OFFSET);
+ double expectedFpp = computeEffectiveFpp(columnMetadata, config);
+ if (Double.compare(storedFpp, expectedFpp) != 0) {
+ LOGGER.info("Bloom filter fpp config changed for segment: {}, column: {}, stored fpp: {}, "
+ + "expected fpp: {}. Index needs to be rebuilt.",
+ segmentName, column, storedFpp, expectedFpp);
+ return true;
+ }
+ return false;
+ }
+
+ if (version != OnHeapGuavaBloomFilterCreator.VERSION) {
+ // Unrecognized version — force rebuild rather than silently reading bytes at wrong offsets.
+ LOGGER.warn("Unrecognized bloom filter version {} for segment: {}, column: {}; forcing rebuild.",
+ version, segmentName, column);
+ return true;
+ }
+ // V1 format: detect fpp change via numHashFunctions + numLongs in the Guava payload.
+ // These values are at fixed offsets (see V1_NUM_HASH_FUNCTIONS_OFFSET / V1_NUM_LONGS_OFFSET).
+ int storedNumHashFunctions = dataBuffer.getByte(V1_NUM_HASH_FUNCTIONS_OFFSET) & 0xFF;
+ int storedNumLongs = dataBuffer.getInt(V1_NUM_LONGS_OFFSET);
+ int[] expected = computeExpectedNumHashFunctionsAndNumLongs(columnMetadata, config);
+ int expectedNumHashFunctions = expected[0];
+ int expectedNumLongs = expected[1];
+ if (storedNumHashFunctions != expectedNumHashFunctions || storedNumLongs != expectedNumLongs) {
+ LOGGER.info("Bloom filter fpp config changed for segment: {}, column: {}, "
+ + "stored numHashFunctions: {}, expected: {}, stored numLongs: {}, expected: {}. "
+ + "Index needs to be rebuilt.",
+ segmentName, column, storedNumHashFunctions, expectedNumHashFunctions, storedNumLongs, expectedNumLongs);
+ return true;
+ }
+ return false;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to read existing bloom filter for segment: {}, column: {}", segmentName, column, e);
+ return false;
+ }
+ }
Review Comment:
Why are we adding this back? Is there any good reason to directly read into Guava's raw bytes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4853657638
cc @Jackie-Jiang @klsince @xiangfu0 — requesting your review.
## What this PR does
Implements bloom filter rebuild detection when the fpp (false positive probability) config changes (#17137).
**Problem:** When a user updates fpp in their bloom filter config, Pinot's segment pre-processor silently kept the old — now misconfigured — bloom filter. Other index types (H3, range, text) already detect config changes and trigger rebuilds; bloom filters were the odd one out.
**Approach:** Follows the H3 index resolution change-detection pattern.
1. Reads the numHashFunctions stored in the existing bloom filter's data buffer (byte offset 9)
2. Computes the expected value from the new fpp config: ceil(-ln(fpp) / ln(2))
3. If they differ → marks the index for rebuild
**Files changed:**
- BloomFilterHandler.java — added isFppChanged() and computeExpectedNumHashFunctions() hooked into needUpdateIndices() / updateIndices()
- SegmentPreProcessorTest.java — added testBloomFilterFppUpdate covering both v1 and v3 segment formats
**Change scope:** Purely additive — existing segments with unchanged fpp are unaffected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-5087034411
cc @Jackie-Jiang @richardstartin — all tasks are done and review feedback has been addressed. Would appreciate a re-review when you get a chance!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3526868111
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java:
##########
@@ -85,6 +97,61 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) {
return false;
}
+ /**
+ * Checks whether the fpp (false positive probability) config has changed by comparing the number of hash functions
+ * in the existing bloom filter with what the new config would produce. Accepts both Reader and Writer since
+ * Writer extends Reader, allowing this method to be called from both needUpdateIndices and updateIndices.
+ */
+ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segmentName, String column) {
+ ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+ if (columnMetadata == null) {
+ return false;
+ }
+ int existingNumHashFunctions;
+ try {
+ PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column, StandardIndexes.bloomFilter());
+ int version = dataBuffer.getInt(VERSION_OFFSET);
+ if (version != OnHeapGuavaBloomFilterCreator.VERSION) {
+ LOGGER.warn("Unexpected bloom filter version {} for segment: {}, column: {}; skipping fpp check", version,
+ segmentName, column);
+ return false;
+ }
+ existingNumHashFunctions = dataBuffer.getByte(NUM_HASH_FUNCTIONS_OFFSET) & 0xFF;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to read existing bloom filter for segment: {}, column: {}", segmentName, column, e);
+ return false;
+ }
+ int expectedNumHashFunctions = computeExpectedNumHashFunctions(columnMetadata, _bloomFilterConfigs.get(column));
Review Comment:
**Major:** This only compares the stored `numHashFunctions`, but Guava's serialized bloom filter also includes the bit-array length. Different fpp values can round to the same hash-function count while producing a different bit-array size, so Pinot would skip a needed rebuild.
For example, with cardinality 1000, fpp `0.05` and `0.045` both use `k=4`, but Guava would allocate different underlying long-array sizes. The old bloom filter would remain in place even though the configured fpp changed.
Can we compare both `numHashFunctions` and the stored number of longs / bit-array size? `BaseGuavaBloomFilterReader` already reads that at Guava payload offset 2. It would also be good to add a regression test for a same-`k` fpp transition such as `0.05 -> 0.045`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3532886963
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java:
##########
@@ -85,6 +97,61 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) {
return false;
}
+ /**
+ * Checks whether the fpp (false positive probability) config has changed by comparing the number of hash functions
+ * in the existing bloom filter with what the new config would produce. Accepts both Reader and Writer since
+ * Writer extends Reader, allowing this method to be called from both needUpdateIndices and updateIndices.
+ */
+ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segmentName, String column) {
+ ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+ if (columnMetadata == null) {
+ return false;
+ }
+ int existingNumHashFunctions;
+ try {
+ PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column, StandardIndexes.bloomFilter());
+ int version = dataBuffer.getInt(VERSION_OFFSET);
+ if (version != OnHeapGuavaBloomFilterCreator.VERSION) {
+ LOGGER.warn("Unexpected bloom filter version {} for segment: {}, column: {}; skipping fpp check", version,
+ segmentName, column);
+ return false;
+ }
+ existingNumHashFunctions = dataBuffer.getByte(NUM_HASH_FUNCTIONS_OFFSET) & 0xFF;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to read existing bloom filter for segment: {}, column: {}", segmentName, column, e);
+ return false;
+ }
+ int expectedNumHashFunctions = computeExpectedNumHashFunctions(columnMetadata, _bloomFilterConfigs.get(column));
+ if (expectedNumHashFunctions != existingNumHashFunctions) {
Review Comment:
This only compares numHashFunctions, but fpp also changes Guava's bit-array size. For example, fpp 0.03 and 0.025 both use k=5 for a fixed cardinality while the backing bit array grows, so Pinot would keep serving the old bloom filter and silently ignore the stricter config. Compare the serialized bit-array length/effective numBits as well, or persist/compare the full effective bloom config, and add a same-k fpp regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4853312167
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18898?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `23.91304%` with `35 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.97%. Comparing base ([`7fe517a`](https://app.codecov.io/gh/apache/pinot/commit/7fe517afce7bd1d892516f7cddcd7baff41278aa?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8ea839c`](https://app.codecov.io/gh/apache/pinot/commit/8ea839c364b104cec27d4cacc0f3066cf9fdf185?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 300 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18898?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...t/index/loader/bloomfilter/BloomFilterHandler.java](https://app.codecov.io/gh/apache/pinot/pull/18898?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Findex%2Floader%2Fbloomfilter%2FBloomFilterHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2luZGV4L2xvYWRlci9ibG9vbWZpbHRlci9CbG9vbUZpbHRlckhhbmRsZXIuamF2YQ==) | 23.91% | [31 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (7fe517a) and HEAD (8ea839c). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (7fe517a) | HEAD (8ea839c) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|custom-integration1|1|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18898 +/- ##
============================================
- Coverage 63.68% 56.97% -6.71%
+ Complexity 1684 1 -1683
============================================
Files 3262 2604 -658
Lines 199826 152991 -46835
Branches 31031 24884 -6147
============================================
- Hits 127264 87171 -40093
+ Misses 62414 58394 -4020
+ Partials 10148 7426 -2722
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <23.91%> (-6.71%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <23.91%> (-6.71%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <23.91%> (-6.71%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.97% <23.91%> (+1.21%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18898/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/18898?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4900441625
please fix the CI failure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on code in PR #18898:
URL: https://github.com/apache/pinot/pull/18898#discussion_r3535392531
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/bloomfilter/BloomFilterHandler.java:
##########
@@ -85,6 +97,61 @@ public boolean needUpdateIndices(SegmentDirectory.Reader segmentReader) {
return false;
}
+ /**
+ * Checks whether the fpp (false positive probability) config has changed by comparing the number of hash functions
+ * in the existing bloom filter with what the new config would produce. Accepts both Reader and Writer since
+ * Writer extends Reader, allowing this method to be called from both needUpdateIndices and updateIndices.
+ */
+ private boolean isFppChanged(SegmentDirectory.Reader segmentReader, String segmentName, String column) {
+ ColumnMetadata columnMetadata = _segmentDirectory.getSegmentMetadata().getColumnMetadataFor(column);
+ if (columnMetadata == null) {
+ return false;
+ }
+ int existingNumHashFunctions;
+ try {
+ PinotDataBuffer dataBuffer = segmentReader.getIndexFor(column, StandardIndexes.bloomFilter());
+ int version = dataBuffer.getInt(VERSION_OFFSET);
+ if (version != OnHeapGuavaBloomFilterCreator.VERSION) {
+ LOGGER.warn("Unexpected bloom filter version {} for segment: {}, column: {}; skipping fpp check", version,
+ segmentName, column);
+ return false;
+ }
+ existingNumHashFunctions = dataBuffer.getByte(NUM_HASH_FUNCTIONS_OFFSET) & 0xFF;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to read existing bloom filter for segment: {}, column: {}", segmentName, column, e);
+ return false;
+ }
+ int expectedNumHashFunctions = computeExpectedNumHashFunctions(columnMetadata, _bloomFilterConfigs.get(column));
+ if (expectedNumHashFunctions != existingNumHashFunctions) {
Review Comment:
Addressed. Instead of reading Guava internals at all, the effective fpp is now stored explicitly in a new Pinot v2 header so the comparison is clean and format-independent.
**New bloom filter file format (v2):**
```
[TYPE_VALUE_V2=2 (int)][VERSION (int)][effective_fpp (double, 8 B)][Guava bytes...]
```
Legacy v1 format (`TYPE_VALUE=1`) is preserved unchanged for backward compatibility.
**Changes:**
- `OnHeapGuavaBloomFilterCreator.seal()` writes the v2 header with the effective fpp (after `maxSizeInBytes` cap) before the Guava payload.
- `BloomFilterReaderFactory` dispatches on `TYPE_VALUE`: v2 Guava payload starts at offset 16, v1 at offset 8.
- `BloomFilterHandler.isFppChanged()` reads `storedFpp` directly from the v2 header at byte offset 8. For legacy v1 segments it skips fpp detection — those upgrade to v2 on next rebuild.
- `BloomFilterHandlerTest` rewritten for v2: legacy v1 (skip detection), v2 same fpp (no rebuild), v2 different fpp (rebuild).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Rebuild bloom filter index when fpp config changes [pinot]
Posted by "Akanksha-kedia (via GitHub)" <gi...@apache.org>.
Akanksha-kedia commented on PR #18898:
URL: https://github.com/apache/pinot/pull/18898#issuecomment-4915090106
The two failing checks (**Pinot Compatibility Regression Testing against master** and **against release-1.5.0**) are pre-existing flaky infrastructure failures, unrelated to this PR.
The failure is a real-time ingestion timing issue — Helix sees `No instances available for leadControllerResource` and queries against `FeatureTest2_REALTIME` return error code 305 before the cluster fully initializes. This is a known intermittent failure in the compatibility test harness.
This PR only touches `pinot-segment-local` (bloom filter rebuild logic when `fpp` config changes). All other checks pass: Linter ✅, Unit Tests ✅, Integration Tests ✅, Quickstart ✅, Binary Compat ✅.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org