[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8137: do not identify function types by throwing exceptions

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #8137:
URL: https://github.com/apache/pinot/pull/8137#discussion_r801003386



##########
File path: pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java
##########
@@ -101,12 +107,28 @@
   // Geo indexing
   GEOTOH3("geoToH3");
 
+  private static final Set<String> NAMES = Arrays.stream(values())
+      .flatMap(func -> Stream.of(func.getName(), func.getName().replace("_", "").toUpperCase(),
+          func.getName().toUpperCase(), func.getName().toLowerCase(), func.name(), func.name().toLowerCase()))
+      .collect(Collectors.toSet());
+
   private final String _name;
 
   TransformFunctionType(String name) {
     _name = name;
   }
 
+  public static boolean isTransformFunction(String functionName) {
+    if (NAMES.contains(functionName)) {
+      return true;
+    }
+    // scalar functions
+    if (FunctionRegistry.containsFunction(functionName)) {
+      return true;
+    }
+    return NAMES.contains(functionName.toUpperCase().replace("_", ""));

Review comment:
       Is `replace("_", "")` faster than `StringUtils.remove(functionName, '_')`? Per the implementation, seems `replace("_", "")` is much more costly, and I don't see annotation about it being optimized by JDK

##########
File path: pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java
##########
@@ -81,14 +87,28 @@ public String getName() {
     return _name;
   }
 
+  public static boolean isAggregationFunction(String functionName) {
+    if (NAMES.contains(functionName)) {
+      return true;
+    }
+    if (functionName.regionMatches(true, 0, "percentile", 0, 10)) {
+      try {
+        getAggregationFunctionType(functionName);
+        return true;
+      } catch (Exception ignore) {
+      }
+    }
+    String upperCaseFunctionName = functionName.replace("_", "").toUpperCase();
+    return NAMES.contains(upperCaseFunctionName);
+  }
+
   /**
    * Returns the corresponding aggregation function type for the given function name.
    * <p>NOTE: Underscores in the function name are ignored.
    */
   public static AggregationFunctionType getAggregationFunctionType(String functionName) {
-    String upperCaseFunctionName = StringUtils.remove(functionName, '_').toUpperCase();
-    if (upperCaseFunctionName.startsWith("PERCENTILE")) {
-      String remainingFunctionName = upperCaseFunctionName.substring(10);
+    if (functionName.regionMatches(true, 0, "percentile", 0, 10)) {

Review comment:
       Does this give better performance than `startsWith()`? In most cases, the query won't match this if check




-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org