xiangfu0 commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3700956196
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunction.java:
##########
@@ -872,6 +886,22 @@ protected byte[][]
transformToBytesValuesSVUsingValueAndNull(ValueBlock valueBlo
return _bytesValuesSV;
}
+ private byte[][] getBytesValues(TransformFunction transformFunction,
ValueBlock valueBlock) {
+ if (_resultMetadata.getDataType() != DataType.UUID || !(transformFunction
instanceof LiteralTransformFunction)) {
Review Comment:
Because BOOLEAN and TIMESTAMP literals keep their type and a UUID literal
does not.
`LiteralTransformFunction` delegates to `LiteralContext`, which holds the
literal's `DataType`. A BOOLEAN literal is `DataType.BOOLEAN`, so
`getIntLiteral()` → `getIntValue()` converts correctly; likewise TIMESTAMP →
`getLongValue()`. For UUID there is no UUID literal: `CAST('550e8400-...' AS
UUID)` is folded by `CompileTimeFunctionsInvoker` into a plain **STRING**
literal, so `LiteralContext._type` is STRING and `getBytesLiteral()` →
`getBytesValue()` hex-decodes it, which fails or silently produces the wrong
bytes.
So this is compensating in the consumer for type information lost at the
literal — the same root cause as the RHS constant-folding discussion earlier in
this PR.
The principled fix is to carry the UUID type on the literal so
`LiteralContext` reports `DataType.UUID` and `getBytesValue()` parses
canonically; then this method disappears and `CASE` needs no UUID branch at
all. That means touching literal typing in the planner/rewriter, which is
squarely part 6/7 territory rather than this PR. Happy to do it there and
delete this — or here if you would rather it not land in this shape.
##########
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:
I could not establish that it is, which is why the polymorphic helper is
there rather than a cast.
`UuidUtils.toBytes(Object)` accepts `java.util.UUID`, `String`, `byte[]` and
`ByteArray`. The value here comes from `ColumnValueExtractor.extract`, which is
just `row[_index]` off a reduce-path row, and `PredicateRowMatcher` is reached
from `HavingFilterHandler` and `GapfillFilterHandler`. Depending on how the row
was produced, a UUID column can be the internal `ByteArray` (what
`ColumnDataType.UUID#toInternal` yields) or the external `java.util.UUID`
(`toExternal`), so I did not want to commit to `(UUID) value` without being
able to show it always holds.
If you know the reduce path normalises to one of these before it reaches the
matcher, I will make it a direct cast to match the other branches. I would also
note there is no test exercising a UUID column through HAVING, so this line is
currently unverified either way — worth adding once the expected type is
settled.
--
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]
