xiangfu0 commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3700953114
##########
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.
+ private String bloomFilterKey() {
Review Comment:
Canonical string, not hex — and that asymmetry with BYTES is exactly why
this line is not just `_comparableValue.toString()`.
The writer is `BloomFilterCreator#add(Object, int)` in pinot-segment-spi:
```java
if (getDataType() == FieldSpec.DataType.BYTES) {
add(BytesUtils.toHexString((byte[]) value));
} else if (getDataType() == FieldSpec.DataType.UUID) {
add(uuidToCanonicalString(value)); // canonical dashed form
} else {
add(value.toString());
}
```
So BYTES is indexed as hex and UUID as the canonical string. On the reader
side `_comparableValue` is a `ByteArray` for both (`DataType#convertInternal`
returns `ByteArray` for BYTES and UUID), and `ByteArray.toString()` is
`toHexString()` — correct for BYTES, wrong for UUID. Hence the UUID branch.
Worth flagging what this line must *not* become: routing it through
`FieldSpec.DataType#toString` looks tidier and handles UUID, but that method
renders BIG_DECIMAL with `toPlainString()` while the creator uses
`value.toString()`. `new BigDecimal("1.0E-7")` would then be indexed as
`1.0E-7` and looked up as `0.00000010`, so every BIG_DECIMAL bloom filter
starts silently missing and pruning segments that do contain matches. I had it
that way at one point;
`BloomFilterSegmentPrunerTest.testBloomFilterRoundTripsThroughCreatorRendering`
now builds the filter through the real `BloomFilterCreator` and covers DOUBLE /
BIG_DECIMAL / BYTES / UUID to pin 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]
