Copilot commented on code in PR #19165:
URL: https://github.com/apache/pinot/pull/19165#discussion_r3723240839
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CastTransformFunctionTest.java:
##########
@@ -35,6 +35,17 @@
public class CastTransformFunctionTest extends BaseTransformFunctionTest {
+ @Test
+ public void testCastUuidToString() {
+ String uuid = "550e8400-e29b-41d4-a716-446655440000";
+ ExpressionContext expression =
RequestContextUtils.getExpression("CAST(toUuid('" + uuid + "') AS STRING)");
+ TransformFunction transformFunction =
TransformFunctionFactory.get(expression, _dataSourceMap);
+
+ String[] expectedValues = new String[NUM_ROWS];
+ Arrays.fill(expectedValues, uuid);
+ testTransformFunction(transformFunction, expectedValues);
+ }
Review Comment:
The PR description says it adds coverage for both single-value and
multi-value UUID expressions, but this test only covers the single-value path.
Either add a corresponding MV test (e.g., casting an MV UUID expression to
STRING) or adjust the PR description to reflect the current test scope.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CastTransformFunction.java:
##########
@@ -423,6 +433,19 @@ public String[][] transformToStringValuesMV(ValueBlock
valueBlock) {
long[][] longValuesMV =
_transformFunction.transformToLongValuesMV(valueBlock);
ArrayCopyUtils.copyFromTimestamp(longValuesMV, _stringValuesMV,
length);
return _stringValuesMV;
+ case UUID:
+ length = valueBlock.getNumDocs();
+ initStringValuesMV(length);
+ byte[][][] uuidValuesMV =
_transformFunction.transformToBytesValuesMV(valueBlock);
+ for (int i = 0; i < length; i++) {
+ int numValues = uuidValuesMV[i].length;
+ String[] stringValues = new String[numValues];
+ for (int j = 0; j < numValues; j++) {
+ stringValues[j] = UuidUtils.toString(uuidValuesMV[i][j]);
+ }
+ _stringValuesMV[i] = stringValues;
+ }
+ return _stringValuesMV;
Review Comment:
The new MV UUID-to-STRING conversion path isn’t covered by the added tests.
Please add a unit test that exercises `transformToStringValuesMV` for UUIDs
(including at least one row with multiple UUIDs) to ensure canonical dashed
formatting is preserved for MV inputs.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/CastTransformFunction.java:
##########
@@ -423,6 +433,19 @@ public String[][] transformToStringValuesMV(ValueBlock
valueBlock) {
long[][] longValuesMV =
_transformFunction.transformToLongValuesMV(valueBlock);
ArrayCopyUtils.copyFromTimestamp(longValuesMV, _stringValuesMV,
length);
return _stringValuesMV;
+ case UUID:
+ length = valueBlock.getNumDocs();
+ initStringValuesMV(length);
+ byte[][][] uuidValuesMV =
_transformFunction.transformToBytesValuesMV(valueBlock);
+ for (int i = 0; i < length; i++) {
+ int numValues = uuidValuesMV[i].length;
+ String[] stringValues = new String[numValues];
+ for (int j = 0; j < numValues; j++) {
+ stringValues[j] = UuidUtils.toString(uuidValuesMV[i][j]);
+ }
+ _stringValuesMV[i] = stringValues;
+ }
+ return _stringValuesMV;
Review Comment:
In `transformToStringValuesSV`, the `case UUID` block is wrapped in braces
(`case UUID: { ... }`), but the MV version isn’t. Consider using braces here as
well for consistency and to reduce the risk of scoping/maintenance issues when
future edits introduce additional local variables in this case.
--
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]
