[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8159: Adding broker level config for disabling Pinot queries with Groovy

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

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



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1168,6 +1173,56 @@ private static void handleHLLLog2mOverride(PinotQuery pinotQuery, int hllLog2mOv
     }
   }
 
+  /**
+   * Verifies that no groovy is present in the PinotQuery when disabled.
+   */
+  @VisibleForTesting
+  static void handleDisableGroovyOverride(PinotQuery pinotQuery) {
+    List<Expression> selectList = pinotQuery.getSelectList();
+    for (Expression expression : selectList) {
+      handleDisableGroovyOverride(expression);
+    }
+    List<Expression> orderByList = pinotQuery.getOrderByList();
+    if (orderByList != null) {
+      for (Expression expression : orderByList) {
+        // NOTE: Order-by is always a Function with the ordering of the Expression
+        handleDisableGroovyOverride(expression.getFunctionCall().getOperands().get(0));
+      }
+    }
+    Expression havingExpression = pinotQuery.getHavingExpression();
+    if (havingExpression != null) {
+      handleDisableGroovyOverride(havingExpression);
+    }
+    Expression filterExpression = pinotQuery.getFilterExpression();
+    if (filterExpression != null) {
+      handleDisableGroovyOverride(filterExpression);
+    }
+    List<Expression> groupByList = pinotQuery.getGroupByList();
+    if (groupByList != null) {
+      for (Expression expression : groupByList) {
+        handleDisableGroovyOverride(expression);
+      }
+    }
+  }
+
+  private static void handleDisableGroovyOverride(Expression expression) {
+    Function functionCall = expression.getFunctionCall();
+    if (functionCall == null) {
+      return;
+    }
+
+    if (functionCall.getOperator().toUpperCase().contains("GROOVY")) {
+      throw new RuntimeException("Groovy transform functions are disabled for queries");

Review comment:
       Let's throw `BadQueryRequestException` here

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1168,6 +1173,56 @@ private static void handleHLLLog2mOverride(PinotQuery pinotQuery, int hllLog2mOv
     }
   }
 
+  /**
+   * Verifies that no groovy is present in the PinotQuery when disabled.
+   */
+  @VisibleForTesting
+  static void handleDisableGroovyOverride(PinotQuery pinotQuery) {

Review comment:
       Let's rename it to `rejectGroovyQuery` since it is not override

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1168,6 +1173,56 @@ private static void handleHLLLog2mOverride(PinotQuery pinotQuery, int hllLog2mOv
     }
   }
 
+  /**
+   * Verifies that no groovy is present in the PinotQuery when disabled.
+   */
+  @VisibleForTesting
+  static void handleDisableGroovyOverride(PinotQuery pinotQuery) {
+    List<Expression> selectList = pinotQuery.getSelectList();
+    for (Expression expression : selectList) {
+      handleDisableGroovyOverride(expression);
+    }
+    List<Expression> orderByList = pinotQuery.getOrderByList();
+    if (orderByList != null) {
+      for (Expression expression : orderByList) {
+        // NOTE: Order-by is always a Function with the ordering of the Expression
+        handleDisableGroovyOverride(expression.getFunctionCall().getOperands().get(0));
+      }
+    }
+    Expression havingExpression = pinotQuery.getHavingExpression();
+    if (havingExpression != null) {
+      handleDisableGroovyOverride(havingExpression);
+    }
+    Expression filterExpression = pinotQuery.getFilterExpression();
+    if (filterExpression != null) {
+      handleDisableGroovyOverride(filterExpression);
+    }
+    List<Expression> groupByList = pinotQuery.getGroupByList();
+    if (groupByList != null) {
+      for (Expression expression : groupByList) {
+        handleDisableGroovyOverride(expression);
+      }
+    }
+  }
+
+  private static void handleDisableGroovyOverride(Expression expression) {
+    Function functionCall = expression.getFunctionCall();
+    if (functionCall == null) {
+      return;
+    }
+
+    if (functionCall.getOperator().toUpperCase().contains("GROOVY")) {

Review comment:
       The function name needs to be canonicalized. See `TransformFunctionFactory` for details




-- 
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