Jackie-Jiang commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3708803895
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -822,19 +830,43 @@ protected byte[][]
transformToBytesValuesSVUsingValue(ValueBlock valueBlock) {
return _bytesValuesSV;
}
+ /// Coerces a bare STRING literal branch to UUID bytes when the `CASE`
result type is UUID, e.g.
+ /// `CASE WHEN c < 2 THEN '550e8400-...' ELSE CAST(... AS UUID) END`. Such a
literal is typed STRING, so
+ /// [LiteralTransformFunction#getBytesLiteral] would hex-decode it and get
the wrong bytes.
+ ///
+ /// A `CAST(... AS UUID)` branch does NOT come through here: it is folded to
a BINARY literal carrying the 16
+ /// stored bytes (see `RequestUtils#getLiteral(Object)`), so it takes the
normal path below. BOOLEAN and TIMESTAMP
+ /// need no equivalent because their literals keep their own type and
convert themselves.
+ private byte[][] getBytesValues(TransformFunction transformFunction,
ValueBlock valueBlock) {
Review Comment:
Do you mean BOOLEAN and TIMESTAMP literal can be directly identified as
their own type? What is the difference for UUID?
##########
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:
Why UUID is different from BOOLEAN and TIMESTAMP? Shouldn't it already be
converted into `ByteArray`?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/filter/PredicateRowMatcher.java:
##########
@@ -78,6 +79,8 @@ public boolean isMatch(Object[] row) {
return _predicateEvaluator.applySV((String) value);
case BYTES:
return _predicateEvaluator.applySV((byte[]) value);
+ case UUID:
+ return _predicateEvaluator.applySV(UuidUtils.toBytes(value));
Review Comment:
^^
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -872,6 +904,7 @@ protected byte[][]
transformToBytesValuesSVUsingValueAndNull(ValueBlock valueBlo
return _bytesValuesSV;
}
+
Review Comment:
(minor) Remove
##########
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:
I feel this inconsistency (canonical dash and hex) is a wiring bug. There
should be a single canonical format reaching here
--
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]
