Copilot commented on code in PR #18932:
URL: https://github.com/apache/pinot/pull/18932#discussion_r3549581101
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformerTest.java:
##########
@@ -245,6 +245,31 @@ public void
testTransformReturningNullDoesNotOverrideExistingBytesValue() {
Assert.assertFalse(row.isNullValue("payload"));
}
+ @Test
+ public void testNonDeterministicTransformFunctionStillRunsAtRuntime() {
+ Schema schema = new Schema.SchemaBuilder()
+ .addSingleValueDimension("eventTimeMs", FieldSpec.DataType.LONG)
+ .build();
Review Comment:
This new test asserts that ingestion-time transforms like `now()` still
execute successfully via `ExpressionTransformer`. That contradicts the PR goal
of disallowing non-deterministic ingestion transforms (and the summary’s
mention of rejecting them when constructing ingestion transform evaluators).
As-is, any ingestion path that constructs `ExpressionTransformer` without
running `TableConfigUtils.validate*` first can still accept and execute
non-deterministic transforms. Consider enforcing deterministic-only evaluation
in `ExpressionTransformer`/transformer construction (or clarifying the PR scope
and renaming/removing this test accordingly).
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformerTest.java:
##########
@@ -245,6 +245,31 @@ public void
testTransformReturningNullDoesNotOverrideExistingBytesValue() {
Assert.assertFalse(row.isNullValue("payload"));
}
+ @Test
+ public void testNonDeterministicTransformFunctionStillRunsAtRuntime() {
+ Schema schema = new Schema.SchemaBuilder()
+ .addSingleValueDimension("eventTimeMs", FieldSpec.DataType.LONG)
+ .build();
+ IngestionConfig ingestionConfig = new IngestionConfig();
+ ingestionConfig.setTransformConfigs(List.of(new
TransformConfig("eventTimeMs", "now()")));
+ TableConfig tableConfig = new TableConfigBuilder(TableType.REALTIME)
+
.setTableName("testNonDeterministicTransformFunctionStillRunsAtRuntime")
+ .setIngestionConfig(ingestionConfig)
+ .build();
+ ExpressionTransformer expressionTransformer = new
ExpressionTransformer(tableConfig, schema);
+
+ GenericRow row = new GenericRow();
+ long lowerBound = System.currentTimeMillis();
+ expressionTransformer.transform(row);
+ long upperBound = System.currentTimeMillis();
+
+ Object value = row.getValue("eventTimeMs");
+ Assert.assertTrue(value instanceof Long, "Expected now() transform to
produce a LONG value");
+ long eventTimeMs = (Long) value;
+ Assert.assertTrue(eventTimeMs >= lowerBound);
+ Assert.assertTrue(eventTimeMs <= upperBound);
Review Comment:
This test relies on tight wall-clock bounds around
`System.currentTimeMillis()`. On some CI hosts with clock adjustments (e.g.,
NTP step) this can be flaky even if `now()` is functioning correctly. Adding a
small tolerance keeps the intent while reducing flakiness risk.
--
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]
