xiangfu0 commented on code in PR #18872:
URL: https://github.com/apache/pinot/pull/18872#discussion_r3740132322
##########
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:
There is no difference — you were right, and the merged version in #19183
treats them the same.
The `getBytesValues()` coercion helper I had added is gone. UUID now shares
the BYTES literal path outright:
```java
case BYTES:
case UUID:
try {
byte[] bytes = BytesUtils.toBytes(literal);
Preconditions.checkArgument(dataType != DataType.UUID || bytes.length ==
dataType.size());
} catch (Exception e) {
throw new IllegalArgumentException("Invalid literal: " + literal + " for
" + dataType);
}
```
so a UUID literal is identified as its own type exactly the way a BOOLEAN or
TIMESTAMP literal is, with the only addition being the fixed 16-byte length
check. Same single-canonical-format rule as the `IN` thread.
That change also fixed a real bug next to it: the bytes path was calling
`initStringValuesSV(numDocs)` and then filling `_bytesValuesSV`, which is now
`initBytesValuesSV(numDocs)`.
Resolving, since this file is no longer in this PR.
##########
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:
This one did **not** get fixed — flagging it rather than closing it silently.
The merged #19183 actually introduced the stray line instead of removing it.
On master today:
```
875: return _bytesValuesSV;
876: }
877:
878:
879: @Override
880: public RoaringBitmap getNullBitmap(ValueBlock valueBlock) {
```
Checkstyle does not catch it — the `RegexpMultiline` rule in
`config/checkstyle.xml` only covers a blank line *before a closing brace*, not
a double blank line between methods.
`CaseTransformFunction` is no longer part of this PR, and adding it back
purely for a whitespace fix would undo the split. I will send it as a one-line
follow-up unless you would rather it ride along with something else already
touching this file.
--
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]
