xiangfu0 commented on code in PR #19888:
URL: https://github.com/apache/pinot/pull/19888#discussion_r4288758904
##########
pinot-common/src/main/java/org/apache/pinot/common/evaluator/FunctionEvaluatorFactory.java:
##########
@@ -109,11 +111,40 @@ public static boolean isGroovyExpression(String
transformExpression) {
return
transformExpression.startsWith(GroovyFunctionEvaluator.getGroovyExpressionPrefix());
}
- private static String getDefaultMapKeysTransformExpression(String
mapColumnName) {
- return String.format("Groovy({%s.sort()*.key}, %s)", mapColumnName,
mapColumnName);
- }
+ /// Extracts legacy Avro map keys or values in key order. Immutable and safe
to share between threads.
+ private static class MapFunctionEvaluator implements FunctionEvaluator {
+ private final String _mapColumnName;
+ private final boolean _extractKeys;
+
+ MapFunctionEvaluator(String mapColumnName, boolean extractKeys) {
+ _mapColumnName = mapColumnName;
+ _extractKeys = extractKeys;
+ }
- private static String getDefaultMapValuesTransformExpression(String
mapColumnName) {
- return String.format("Groovy({%s.sort()*.value}, %s)", mapColumnName,
mapColumnName);
+ @Override
+ public List<String> getArguments() {
+ return List.of(_mapColumnName);
+ }
+
+ @Override
+ public Object evaluate(GenericRow genericRow) {
+ return evaluateMap(genericRow.getValue(_mapColumnName));
+ }
+
+ @Override
+ public Object evaluate(Object[] values) {
+ return evaluateMap(values[0]);
+ }
+
+ @Nullable
+ private List<?> evaluateMap(@Nullable Object value) {
+ if (value == null) {
+ return null;
+ }
+ Map<?, ?> map = (Map<?, ?>) value;
+ // Groovy's sort() preserves the comparator of an existing SortedMap.
Review Comment:
This comment is misleading: Groovy's `Map.sort()` actually ignores any
existing comparator and re-sorts into ascending natural key order, so the old
Groovy path and this new code diverge when the input is a `SortedMap` with a
non-natural comparator (old would force ascending, new keeps the reverse
order). For plain Avro `HashMap`/`LinkedHashMap` inputs the two are equivalent,
so real ingestion is unaffected, but please reword the comment to say the
preservation is a deliberate new behavior rather than matching Groovy.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigValidationUtils.java:
##########
@@ -49,6 +51,23 @@ public final class TableConfigValidationUtils {
private TableConfigValidationUtils() {
}
+ /// Rejects legacy FieldSpec Groovy transforms before schema validation can
compile them.
+ @SuppressWarnings("deprecation") // FieldSpec transforms remain supported in
stored schemas.
+ static void validateSchemaGroovy(Schema schema, ControllerConf
controllerConf) {
+ if (controllerConf.isDisableIngestionGroovy()) {
+ String groovyPrefix =
GroovyFunctionEvaluator.getGroovyExpressionPrefix() + "(";
+ for (FieldSpec fieldSpec : schema.getAllFieldSpecs()) {
+ String transformFunction = fieldSpec.getTransformFunction();
+ if (transformFunction != null
+ && transformFunction.regionMatches(true, 0, groovyPrefix, 0,
groovyPrefix.length())) {
Review Comment:
Nice that this schema check matches the prefix case-insensitively, since the
evaluator regex is case-insensitive. Just a heads-up that the table-level
filter/transform checks in `TableConfigUtils` still use
`FunctionEvaluatorFactory.isGroovyExpression`, which is case-sensitive
(`startsWith("Groovy")`), so a lowercase `groovy({...})` slips past those. The
new ingestion compiler gate backstops it, so this is only a defense-in-depth
consistency note, not a blocker.
--
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]
