Copilot commented on code in PR #19182:
URL: https://github.com/apache/pinot/pull/19182#discussion_r3738425411
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ValueBasedSegmentPruner.java:
##########
@@ -230,15 +232,30 @@ public void ensureDataType(DataType dt) {
}
public boolean mightBeContained(BloomFilterReader bloomFilter) {
+ // The rendering and hashing below run once per (value, data type):
the resulting hashes are memoized and
+ // every subsequent segment in the query reuses them. Deliberately not
precomputed in ensureDataType, so a
+ // query that only reaches min/max pruning never pays for it.
if (!_hashed) {
GuavaBloomFilterReaderUtils.Hash128AsLongs hash128AsLongs =
-
GuavaBloomFilterReaderUtils.hashAsLongs(_comparableValue.toString());
+ GuavaBloomFilterReaderUtils.hashAsLongs(bloomFilterKey());
_hash1 = hash128AsLongs.getHash1();
_hash2 = hash128AsLongs.getHash2();
_hashed = true;
}
return bloomFilter.mightContain(_hash1, _hash2);
}
Review Comment:
The PR description states this was “not a correctness bug” because a
bloom-filter miss “only ever means a segment isn't pruned”. However, the
pruning logic prunes a segment when `mightBeContained()` is false (bloom-filter
miss), so probing the bloom filter with a differently-rendered key can prune
segments that actually contain matching rows. Please update/clarify the PR
description accordingly so readers don’t underestimate the impact.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/pruner/ValueBasedSegmentPruner.java:
##########
@@ -230,15 +232,30 @@ public void ensureDataType(DataType dt) {
}
public boolean mightBeContained(BloomFilterReader bloomFilter) {
+ // The rendering and hashing below run once per (value, data type):
the resulting hashes are memoized and
+ // every subsequent segment in the query reuses them. Deliberately not
precomputed in ensureDataType, so a
+ // query that only reaches min/max pruning never pays for it.
if (!_hashed) {
GuavaBloomFilterReaderUtils.Hash128AsLongs hash128AsLongs =
-
GuavaBloomFilterReaderUtils.hashAsLongs(_comparableValue.toString());
+ GuavaBloomFilterReaderUtils.hashAsLongs(bloomFilterKey());
_hash1 = hash128AsLongs.getHash1();
_hash2 = hash128AsLongs.getHash2();
_hashed = true;
}
return bloomFilter.mightContain(_hash1, _hash2);
}
+
+ /// Renders the value exactly as `BloomFilterCreator#add(Object, int)`
did when the index was built. If the
+ /// two disagree the lookup silently misses and the segment is wrongly
pruned, dropping matching rows with no
+ /// error. That creator special-cases UUID to the canonical string and
renders everything else with
+ /// `value.toString()` -- which for BYTES is already hex via [ByteArray].
+ ///
+ /// Deliberately NOT routed through `DataType#toString`: that renders
BIG_DECIMAL with
+ /// `toPlainString()`, which the creator does not, so every BIG_DECIMAL
bloom filter would start missing.
Review Comment:
The comment implies BloomFilterCreator renders “everything else” via
`value.toString()` and that this is safe for BYTES because of `ByteArray`.
However, BloomFilterCreator actually special-cases BYTES and expects a `byte[]`
(where `toString()` would be `[B@...`), so the current wording is misleading
and could cause a future regression if someone refactors based on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
