xiangfu0 commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3706905788
##########
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:
Because the switch is on `getStoredType()`, and UUID is the only logical
type that *collides* with another type there.
BOOLEAN reduces to INT and TIMESTAMP to LONG, and each is the only logical
type sharing that stored type, so `case INT` / `case LONG` can parse
unconditionally. UUID reduces to BYTES — which BYTES itself already occupies —
and the two spell their literals differently (canonical dashed text vs hex). So
`case BYTES` cannot pick a parser from the stored type alone; it has to ask
whether the column is UUID.
One caveat I do not want to overstate: `case INT` parses with
`Integer.parseInt`, which would throw on a `true` / `false` literal, since
`LiteralContext#getStringValue()` renders a BOOLEAN literal via
`PinotDataType.BOOLEAN.toString`. The predicate-side equivalent,
`BaseInPredicate#getBooleanValues()`, goes through `BooleanUtils.toInt` and
does handle it. So BOOLEAN here looks like a pre-existing gap rather than
evidence that no handling is needed — I have not confirmed it end to end and it
is outside this PR, but worth a look.
##########
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:
BOOLEAN and TIMESTAMP go through the `else` branch — `add(value.toString())`
— because by the time the creator sees them they are already their stored types
(`int` / `long`), so there is no logical-type rendering left to do. Only BYTES
and UUID need an explicit branch, because both arrive as `byte[]` and
`byte[]#toString` is the object identity.
On storing hex for UUID instead: I agree it would be more consistent, and it
would let `bloomFilterKey()` collapse to `_comparableValue.toString()` since
`ByteArray#toString()` is already hex. The blocker is that it changes the
on-disk index. Bloom filters in existing segments were written with the
canonical form, so flipping the writer makes every pre-existing UUID bloom
filter miss — and a miss here prunes a segment that does contain matches, i.e.
silently wrong results rather than an error. It would need a segment-format
version gate to be safe.
Given UUID segments only became possible in #18870, the population of
affected segments is small right now, so this is the cheapest moment to make
that switch if you want it. Happy to do it as its own PR with the version gate;
I would rather not fold an index-format change into this one.
--
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]
