xiangfu0 commented on code in PR #19887:
URL: https://github.com/apache/pinot/pull/19887#discussion_r4288755712
##########
pinot-common/src/main/java/org/apache/pinot/common/evaluator/GroovyFunctionEvaluator.java:
##########
@@ -60,6 +60,7 @@ public class GroovyFunctionEvaluator implements
FunctionEvaluator {
private static final String SCRIPT_GROUP_NAME = "script";
private static final String ARGUMENTS_SEPARATOR = ",";
private static GroovyStaticAnalyzerConfig _groovyStaticAnalyzerConfig;
+ private static volatile boolean _ingestionGroovyDisabled;
Review Comment:
This is a JVM-wide latch toggled by a static setter. Fine for the
service-config use, but the tests flip it and reset it in `finally`, so if any
of these classes ever run in parallel in the same JVM they could observe each
other's state. Just worth keeping in mind if test parallelism gets enabled
later.
##########
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.
+ Map<?, ?> sortedMap = map instanceof SortedMap<?, ?> ? map : new
TreeMap<>(map);
+ return new ArrayList<>(_extractKeys ? sortedMap.keySet() :
sortedMap.values());
Review Comment:
Nice call reusing the map's own comparator here. I confirmed against Groovy
3.0.25 that `map.sort()` keeps an existing SortedMap comparator and
natural-orders everything else, so this `TreeMap` fallback matches the old
`Groovy({...sort()...})` output exactly, including the reverse-ordered case.
Behavior-preserving.
--
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]
