This is an automated email from the ASF dual-hosted git repository.
snlee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new b672af1b9f Disable Groovy function by default (#8711)
b672af1b9f is described below
commit b672af1b9f2ffdea1312207f63b5f8b90bdd1b56
Author: Seunghyun Lee <sn...@linkedin.com>
AuthorDate: Tue May 17 00:00:53 2022 -0700
Disable Groovy function by default (#8711)
This is the last step for #7966 to disable groovy function by default.
---
.../config/BrokerConfig.properties | 1 +
.../config/ControllerConfig.properties | 1 +
.../requesthandler/BaseBrokerRequestHandler.java | 54 +++++++++++++---------
.../apache/pinot/controller/ControllerConf.java | 6 ++-
.../pinot/controller/helix/ControllerTest.java | 3 +-
.../tests/BaseClusterIntegrationTest.java | 15 ++++--
.../tests/OfflineClusterIntegrationTest.java | 18 ++++----
.../apache/pinot/spi/utils/CommonConstants.java | 1 +
8 files changed, 60 insertions(+), 39 deletions(-)
diff --git
a/compatibility-verifier/sample-test-suite/config/BrokerConfig.properties
b/compatibility-verifier/sample-test-suite/config/BrokerConfig.properties
index 35dc7b485b..e56f23c287 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 86d14c2346..9949b2519e 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 5cf17eb19e..1718e61ca4 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.transport.ServerInstance;
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 abstract class BaseBrokerRequestHandler implements
BrokerRequestHandler {
_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 @@ public abstract class BaseBrokerRequestHandler
implements BrokerRequestHandler {
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 a7f54b39c6..f23e7e6ae2 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 @@ public class ControllerConf extends PinotConfiguration {
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 class ControllerConf extends PinotConfiguration {
* @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 31d18d619f..fc23f1d5f6 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 abstract class ControllerTest {
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 c035cf8667..f8fe1a42d6 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.common.utils.config.TagNameUtils;
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 @@ public abstract class BaseClusterIntegrationTest extends
ClusterTest {
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 @@ public abstract class BaseClusterIntegrationTest extends
ClusterTest {
.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 @@ public abstract class BaseClusterIntegrationTest extends
ClusterTest {
.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 0e7ae56a48..30873869e5 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 class OfflineClusterIntegrationTest extends
BaseClusterIntegrationTestSet
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 class OfflineClusterIntegrationTest extends
BaseClusterIntegrationTestSet
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 24f1d93a1f..2fbb323bd4 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 class CommonConstants {
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
