diff --git a/compatibility-verifier/sample-test-suite/config/BrokerConfig.properties b/compatibility-verifier/sample-test-suite/config/BrokerConfig.properties index 35dc7b485b82..e56f23c287b6 100644 --- a/compatibility-verifier/sample-test-suite/config/BrokerConfig.properties +++ b/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 diff --git a/compatibility-verifier/sample-test-suite/config/ControllerConfig.properties b/compatibility-verifier/sample-test-suite/config/ControllerConfig.properties index 86d14c2346a0..9949b2519ea6 100644 --- a/compatibility-verifier/sample-test-suite/config/ControllerConfig.properties +++ b/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 diff --git a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java b/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java index 5cf17eb19e11..1718e61ca488 100644 --- a/pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java +++ b/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); } diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java b/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java index a7f54b39c638..f23e7e6ae2ef 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java +++ b/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) { diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java index 31d18d619f6b..fc23f1d5f6c7 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java @@ -161,7 +161,8 @@ public Map 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; } diff --git a/pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java b/pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java index c035cf8667ca..f8fe1a42d6cf 100644 --- a/pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java +++ b/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(); } /** diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java index 0e7ae56a4821..30873869e549 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java +++ b/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() diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java index 24f1d93a1fbf..2fbb323bd4b4 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java +++ b/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