Copilot commented on code in PR #17363:
URL: https://github.com/apache/pinot/pull/17363#discussion_r2615762989
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformerTest.java:
##########
@@ -430,4 +458,32 @@ public void testJsonToMapIngestionTransform() {
Assert.assertEquals(map.get("a"), 1);
Assert.assertEquals(map.get("b"), "x");
}
+
+ @Test
+ public void testJsonToArrayIngestionTransform() {
+ Schema schema = new Schema.SchemaBuilder()
+ .addSingleValueDimension("columnJson", FieldSpec.DataType.STRING)
+ .addMultiValueDimension("columnArray", FieldSpec.DataType.STRING)
+ .build();
+
+ IngestionConfig ingestionConfig = new IngestionConfig();
+ ingestionConfig.setTransformConfigs(Collections.singletonList(
+ new TransformConfig("columnArray", "jsonPathArray(columnJson, '$')")));
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE)
+ .setTableName("testJsonToArrayIngestionTransform")
+ .setIngestionConfig(ingestionConfig)
+ .build();
+
+ ExpressionTransformer expressionTransformer = new
ExpressionTransformer(tableConfig, schema);
+
+ GenericRow row = new GenericRow();
+ row.putValue("columnJson", "[\"a\",\"b\",\"c\"]");
+ // Pre-existing collection should be overwritten because transform returns
an array (incompatible type)
+ row.putValue("columnArray", Arrays.asList("preExisting"));
+
+ expressionTransformer.transform(row);
+ Object transformedValue = row.getValue("columnArray");
+ Assert.assertTrue(transformedValue.getClass().isArray());
+ Assert.assertEquals(Arrays.asList((Object[]) transformedValue),
Arrays.asList("a", "b", "c"));
Review Comment:
Similar to the previous test, the cast to `(Object[])` assumes the array is
an object array. Consider adding assertions about the array component type or
using array-specific comparison utilities to make the test more robust.
```suggestion
// Assert that the array is of type String[]
Assert.assertEquals(transformedValue.getClass().getComponentType(),
String.class);
Assert.assertEquals(Arrays.asList((String[]) transformedValue),
Arrays.asList("a", "b", "c"));
```
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformerTest.java:
##########
@@ -241,6 +241,34 @@ public void testValueAlreadyExists() {
Assert.assertEquals(genericRow.getValue("outgoing"), "123");
}
+ @Test
+ public void testExistingCollectionIsTransformedWhenIncompatibleType() {
+ Schema schema = new Schema.SchemaBuilder()
+ .addMultiValueDimension("rawBids", FieldSpec.DataType.INT)
+ .addMultiValueDimension("bids", FieldSpec.DataType.INT)
+ .build();
+
+ IngestionConfig ingestionConfig = new IngestionConfig();
+ ingestionConfig.setTransformConfigs(Collections.singletonList(
+ new TransformConfig("bids", "Groovy({rawBids.toArray()}, rawBids)")));
+ TableConfig tableConfig = new TableConfigBuilder(TableType.OFFLINE)
+ .setTableName("testExistingCollectionIsTransformed")
+ .setIngestionConfig(ingestionConfig)
+ .build();
+
+ ExpressionTransformer expressionTransformer = new
ExpressionTransformer(tableConfig, schema);
+ GenericRow genericRow = new GenericRow();
+ genericRow.putValue("rawBids", Arrays.asList(1, 2, 3));
+ // Simulate pre-existing collection value that should be overwritten by
transform
+ genericRow.putValue("bids", Arrays.asList(10, 20));
+
+ expressionTransformer.transform(genericRow);
+
+ Object transformedValue = genericRow.getValue("bids");
+ Assert.assertTrue(transformedValue.getClass().isArray());
+ Assert.assertEquals(Arrays.asList((Object[]) transformedValue),
Arrays.asList(1, 2, 3));
Review Comment:
The cast to `(Object[])` may fail if the array is a primitive array type
(e.g., `int[]`). Consider using `Arrays.equals()` for primitive arrays or
adding a type check before casting to ensure robustness.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformer.java:
##########
@@ -20,7 +20,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
-import java.util.Collections;
+import java.util.Collection;
Review Comment:
The import for `java.util.Collections` was removed but may still be needed
elsewhere in the file. Verify that no other code in this file uses
`Collections` utility methods (e.g., `Collections.emptyList()`,
`Collections.singletonList()`, etc.) before removing this import.
--
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]
