Disable Groovy function by default

This is the last step for #7966 to disable groovy function by default.

compatibility-verifier/sample-test-suite/config/BrokerConfig.properties

@@ -20,3 +20,4 @@
 pinot.broker.client.queryPort = 8099
 pinot.zk.server = localhost:2181
 pinot.cluster.name = PinotCluster
+pinot.broker.disable.query.groovy=false

compatibility-verifier/sample-test-suite/config/ControllerConfig.properties

@@ -22,3 +22,4 @@ controller.port = 9000
 controller.zk.str = localhost:2181
 controller.data.dir = /tmp/PinotController
 controller.helix.cluster.name = PinotCluster
+controller.disable.ingestion.groovy = false

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

@@ -70,6 +70,7 @@
 import org.apache.pinot.core.util.GapfillUtils;
 import org.apache.pinot.core.util.QueryOptionsUtils;
 import org.apache.pinot.spi.config.table.FieldConfig;
+import org.apache.pinot.spi.config.table.QueryConfig;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.config.table.TableType;
 import org.apache.pinot.spi.config.table.TimestampIndexGranularity;
@@ -133,7 +134,7 @@ public BaseBrokerRequestHandler(PinotConfiguration config, BrokerRoutingManager
     _queryQuotaManager = queryQuotaManager;
     _tableCache = tableCache;
     _brokerMetrics = brokerMetrics;
-    _disableGroovy = _config.getProperty(CommonConstants.Broker.DISABLE_GROOVY, false);
+    _disableGroovy = _config.getProperty(Broker.DISABLE_GROOVY, Broker.DEFAULT_DISABLE_GROOVY);
     _useApproximateFunction = _config.getProperty(Broker.USE_APPROXIMATE_FUNCTION, false);
     _defaultHllLog2m = _config.getProperty(CommonConstants.Helix.DEFAULT_HYPERLOGLOG_LOG2M_KEY,
         CommonConstants.Helix.DEFAULT_HYPERLOGLOG_LOG2M);
@@ -1007,37 +1008,44 @@ private static void handleDistinctCountBitmapOverride(Expression expression) {
 
   private HandlerContext getHandlerContext(@Nullable TableConfig offlineTableConfig,
       @Nullable TableConfig realtimeTableConfig) {
-    boolean offlineTableDisableGroovyQuery = _disableGroovy;
-    boolean offlineTableUseApproximateFunction = _useApproximateFunction;
+    Boolean disableGroovyOverride = null;
+    Boolean useApproximateFunctionOverride = null;
     if (offlineTableConfig != null && offlineTableConfig.getQueryConfig() != null) {
-      Boolean disableGroovyOverride = offlineTableConfig.getQueryConfig().getDisableGroovy();
-      if (disableGroovyOverride != null) {
-        offlineTableDisableGroovyQuery = disableGroovyOverride;
+      QueryConfig offlineTableQueryConfig = offlineTableConfig.getQueryConfig();
+      Boolean disableGroovyOfflineTableOverride = offlineTableQueryConfig.getDisableGroovy();
+      if (disableGroovyOfflineTableOverride != null) {
+        disableGroovyOverride = disableGroovyOfflineTableOverride;
       }
-      Boolean useApproximateFunctionOverride = offlineTableConfig.getQueryConfig().getUseApproximateFunction();
-      if (useApproximateFunctionOverride != null) {
-        offlineTableUseApproximateFunction = useApproximateFunctionOverride;
+      Boolean useApproximateFunctionOfflineTableOverride = offlineTableQueryConfig.getUseApproximateFunction();
+      if (useApproximateFunctionOfflineTableOverride != null) {
+        useApproximateFunctionOverride = useApproximateFunctionOfflineTableOverride;
       }
     }
-
-    boolean realtimeTableDisableGroovyQuery = _disableGroovy;
-    boolean realtimeTableUseApproximateFunction = _useApproximateFunction;
     if (realtimeTableConfig != null && realtimeTableConfig.getQueryConfig() != null) {
-      Boolean disableGroovyOverride = realtimeTableConfig.getQueryConfig().getDisableGroovy();
-      if (disableGroovyOverride != null) {
-        realtimeTableDisableGroovyQuery = disableGroovyOverride;
+      QueryConfig realtimeTableQueryConfig = realtimeTableConfig.getQueryConfig();
+      Boolean disableGroovyRealtimeTableOverride = realtimeTableQueryConfig.getDisableGroovy();
+      if (disableGroovyRealtimeTableOverride != null) {
+        if (disableGroovyOverride == null) {
+          disableGroovyOverride = disableGroovyRealtimeTableOverride;
+        } else {
+          // Disable Groovy if either offline or realtime table config disables Groovy
+          disableGroovyOverride |= disableGroovyRealtimeTableOverride;
+        }
       }
-      Boolean useApproximateFunctionOverride = realtimeTableConfig.getQueryConfig().getUseApproximateFunction();
-      if (useApproximateFunctionOverride != null) {
-        realtimeTableUseApproximateFunction = useApproximateFunctionOverride;
+      Boolean useApproximateFunctionRealtimeTableOverride = realtimeTableQueryConfig.getUseApproximateFunction();
+      if (useApproximateFunctionRealtimeTableOverride != null) {
+        if (useApproximateFunctionOverride == null) {
+          useApproximateFunctionOverride = useApproximateFunctionRealtimeTableOverride;
+        } else {
+          // Use approximate function if both offline and realtime table config uses approximate function
+          useApproximateFunctionOverride &= useApproximateFunctionRealtimeTableOverride;
+        }
       }
     }
 
-    // Disable Groovy if either offline or realtime table config disables Groovy
-    boolean disableGroovy = offlineTableDisableGroovyQuery | realtimeTableDisableGroovyQuery;
-    // Use approximate function if both offline and realtime table config uses approximate function
-    boolean useApproximateFunction = offlineTableUseApproximateFunction & realtimeTableUseApproximateFunction;
-
+    boolean disableGroovy = disableGroovyOverride != null ? disableGroovyOverride : _disableGroovy;
+    boolean useApproximateFunction =
+        useApproximateFunctionOverride != null ? useApproximateFunctionOverride : _useApproximateFunction;
     return new HandlerContext(disableGroovy, useApproximateFunction);
   }
 

pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java

@@ -255,7 +255,9 @@ private static long getRandomInitialDelayInSeconds() {
   private static final String DEFAULT_DIM_TABLE_MAX_SIZE = "200M";
 
   private static final String DEFAULT_PINOT_FS_FACTORY_CLASS_LOCAL = LocalPinotFS.class.getName();
-  private static final String DISABLE_GROOVY = "controller.disable.ingestion.groovy";
+
+  public static final String DISABLE_GROOVY = "controller.disable.ingestion.groovy";
+  public static final boolean DEFAULT_DISABLE_GROOVY = true;
 
   public ControllerConf() {
     super(new HashMap<>());
@@ -867,7 +869,7 @@ public String getControllerResourcePackages() {
    * @return true if Groovy functions are disabled in controller config, otherwise returns false.
    */
   public boolean isDisableIngestionGroovy() {
-    return getProperty(DISABLE_GROOVY, false);
+    return getProperty(DISABLE_GROOVY, DEFAULT_DISABLE_GROOVY);
   }
 
   private long convertPeriodToUnit(String period, TimeUnit timeUnitToConvertTo) {

pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java

@@ -161,7 +161,8 @@ public Map<String, Object> getDefaultControllerConfiguration() {
     properties.put(ControllerConf.DATA_DIR, DEFAULT_DATA_DIR);
     properties.put(ControllerConf.ZK_STR, getZkUrl());
     properties.put(ControllerConf.HELIX_CLUSTER_NAME, getHelixClusterName());
-
+    // Enable groovy on the controller
+    properties.put(ControllerConf.DISABLE_GROOVY, false);
     return properties;
   }
 

pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java

@@ -39,6 +39,7 @@
 import org.apache.pinot.plugin.stream.kafka.KafkaStreamConfigProperties;
 import org.apache.pinot.spi.config.table.ColumnPartitionConfig;
 import org.apache.pinot.spi.config.table.FieldConfig;
+import org.apache.pinot.spi.config.table.QueryConfig;
 import org.apache.pinot.spi.config.table.ReplicaGroupStrategyConfig;
 import org.apache.pinot.spi.config.table.RoutingConfig;
 import org.apache.pinot.spi.config.table.SegmentPartitionConfig;
@@ -256,6 +257,11 @@ protected IngestionConfig getIngestionConfig() {
     return null;
   }
 
+  protected QueryConfig getQueryconfig() {
+    // Enable groovy for tables used in the tests
+    return new QueryConfig(null, false, null, null);
+  }
+
   protected boolean getNullHandlingEnabled() {
     return DEFAULT_NULL_HANDLING_ENABLED;
   }
@@ -298,8 +304,9 @@ protected TableConfig createOfflineTableConfig() {
         .setRangeIndexColumns(getRangeIndexColumns()).setBloomFilterColumns(getBloomFilterColumns())
         .setFieldConfigList(getFieldConfigs()).setNumReplicas(getNumReplicas()).setSegmentVersion(getSegmentVersion())
         .setLoadMode(getLoadMode()).setTaskConfig(getTaskConfig()).setBrokerTenant(getBrokerTenant())
-        .setServerTenant(getServerTenant()).setIngestionConfig(getIngestionConfig())
-        .setNullHandlingEnabled(getNullHandlingEnabled()).setSegmentPartitionConfig(getSegmentPartitionConfig()).build();
+        .setServerTenant(getServerTenant()).setIngestionConfig(getIngestionConfig()).setQueryConfig(getQueryconfig())
+        .setNullHandlingEnabled(getNullHandlingEnabled()).setSegmentPartitionConfig(getSegmentPartitionConfig())
+        .build();
   }
 
   /**
@@ -368,8 +375,8 @@ protected TableConfig createRealtimeTableConfig(File sampleAvroFile) {
         .setRangeIndexColumns(getRangeIndexColumns()).setBloomFilterColumns(getBloomFilterColumns())
         .setFieldConfigList(getFieldConfigs()).setNumReplicas(getNumReplicas()).setSegmentVersion(getSegmentVersion())
         .setLoadMode(getLoadMode()).setTaskConfig(getTaskConfig()).setBrokerTenant(getBrokerTenant())
-        .setServerTenant(getServerTenant()).setIngestionConfig(getIngestionConfig()).setLLC(useLlc())
-        .setStreamConfigs(getStreamConfigs()).setNullHandlingEnabled(getNullHandlingEnabled()).build();
+        .setServerTenant(getServerTenant()).setIngestionConfig(getIngestionConfig()).setQueryConfig(getQueryconfig())
+        .setLLC(useLlc()).setStreamConfigs(getStreamConfigs()).setNullHandlingEnabled(getNullHandlingEnabled()).build();
   }
 
   /**

pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java

@@ -909,18 +909,18 @@ public void testDisableGroovyQueryTableConfigOverride()
     String groovyQuery = "SELECT GROOVY('{\"returnType\":\"STRING\",\"isSingleValue\":true}', "
         + "'arg0 + arg1', FlightNum, Origin) FROM myTable";
     TableConfig tableConfig = getOfflineTableConfig();
-    tableConfig.setQueryConfig(new QueryConfig(null, true, null, null));
+    tableConfig.setQueryConfig(new QueryConfig(null, false, null, null));
     updateTableConfig(tableConfig);
 
     TestUtils.waitForCondition(aVoid -> {
       try {
+        // Query should not throw exception
         postQuery(groovyQuery);
-        return false;
-      } catch (Exception e) {
-        // expected
         return true;
+      } catch (Exception e) {
+        return false;
       }
-    }, 60_000L, "Failed to reject Groovy query with table override");
+    }, 60_000L, "Failed to accept Groovy query with table override");
 
     // Remove query config
     tableConfig.setQueryConfig(null);
@@ -929,12 +929,12 @@ public void testDisableGroovyQueryTableConfigOverride()
     TestUtils.waitForCondition(aVoid -> {
       try {
         postQuery(groovyQuery);
-        // Query should not throw exception
-        return true;
-      } catch (Exception e) {
         return false;
+      } catch (Exception e) {
+        // expected
+        return true;
       }
-    }, 60_000L, "Failed to accept Groovy query without query table config override");
+    }, 60_000L, "Failed to reject Groovy query without query table config override");
   }
 
   private void reloadWithExtraColumns()

pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java

@@ -230,6 +230,7 @@ public static class Broker {
     public static final String BROKER_SERVICE_AUTO_DISCOVERY = "pinot.broker.service.auto.discovery";
 
     public static final String DISABLE_GROOVY = "pinot.broker.disable.query.groovy";
+    public static final boolean DEFAULT_DISABLE_GROOVY = true;
 
     // Rewrite potential expensive functions to their approximation counterparts
     // - DISTINCT_COUNT -> DISTINCT_COUNT_SMART_HLL