xiangfu0 commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3740131804
##########
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:
Your point held, and this is now resolved on master by #19182 rather than by
the branch you were reviewing here.
Rather than teach the reader a UUID-specific rendering, the **creator** now
keys UUID bloom filters on the stored BYTES form (lowercase hex). Since
`ByteArray#toString()` is already `toHexString()`, the reader needs no UUID
case at all — `bloomFilterKey()` is gone and the probe is back to plain
`_comparableValue.toString()`, exactly as you were pushing for. That answers
your "shouldn't it already be converted into `ByteArray`?" — it is, and now
nothing further is done to it.
That also removes my objection about the on-disk format: the switch happened
in the same change that added segment-generation and reload coverage across
dictionary/raw and SV/MV, plus a query-level integration test, so the writer
and reader moved together rather than needing a version gate.
This file is no longer part of this PR, so I am resolving the thread.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -117,13 +118,18 @@ public void init(List<TransformFunction> arguments,
Map<String, ColumnContext> c
case STRING:
_valueSet = stringValues;
break;
- case BYTES:
+ case BYTES: {
+ // UUID and BYTES differ only in how the literal is parsed --
canonical UUID text vs hex. The stored form,
+ // and therefore the lookup below, is identical.
+ boolean isUuid = _mainFunction.getResultMetadata().getDataType() ==
DataType.UUID;
Review Comment:
Agreed it was a wiring bug, and it is fixed on master by #19183 — by
deleting the branch rather than keeping two formats.
`InTransformFunction.java` ends up **unchanged**: the merged change touches
only its test. A single format reaches it — the stored hex — and the canonical
dashed form is not accepted as a bare literal at all. That is now pinned by a
test:
```java
@Test(expectedExceptions = BadQueryRequestException.class)
public void testUuidInTransformFunctionRejectsBareCanonicalLiteral() { ... }
```
The dashed form is reachable only through an explicit `CAST('...' AS UUID)`,
which folds to the stored form upstream, so the `isUuid` flag and the dual
parsing are both gone.
This is the same rule #19182 settled for bloom filters and the one
`PredicateUtils.getStoredValue` follows in this PR: at a String-typed boundary
a UUID is carried as its stored hex, and the canonical dashed form is a
user-facing surface reached only via an explicit cast.
Resolving, since this file is no longer in this PR.
--
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]
