Jackie-Jiang commented on a change in pull request #8196:
URL: https://github.com/apache/pinot/pull/8196#discussion_r807392247
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -858,6 +858,28 @@ public void testDefaultColumns()
_tableSizeAfterRemovingIndex = getTableSize(getTableName());
}
+ @Test
+ public void testDisableGroovyQueryTableConfigOverride()
+ throws Exception {
+ TableConfig tableConfig = getOfflineTableConfig();
+ tableConfig.setQueryConfig(new QueryConfig(5L, Boolean.TRUE));
+ updateTableConfig(tableConfig);
+
+ reloadOfflineTable(getTableName());
+ try {
+ postSqlQuery("SELECT
GROOVY('{\"returnType\":\"STRING\",\"isSingleValue\":true}', "
+ + "'arg0 + arg1', FlightNum, Origin) FROM myTable");
+ fail("Failed to reject Groovy query with table override");
+ } catch (Exception e) {
+ // expected
+ }
+
+ // Remove query config
+ tableConfig.setQueryConfig(null);
+ updateTableConfig(tableConfig);
+ reloadOfflineTable(getTableName());
Review comment:
Similarly here, we can keep sending queries util the query succeeds
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -858,6 +858,28 @@ public void testDefaultColumns()
_tableSizeAfterRemovingIndex = getTableSize(getTableName());
}
+ @Test
+ public void testDisableGroovyQueryTableConfigOverride()
+ throws Exception {
+ TableConfig tableConfig = getOfflineTableConfig();
+ tableConfig.setQueryConfig(new QueryConfig(5L, Boolean.TRUE));
+ updateTableConfig(tableConfig);
+
+ reloadOfflineTable(getTableName());
Review comment:
We don't need to reload the table because that won't update the table
config on broker, which is ZK watcher based. We can keep sending queries using
`TestUtils.waitForCondition()` until the query fails.
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1174,6 +1180,26 @@ private static void handleHLLLog2mOverride(PinotQuery
pinotQuery, int hllLog2mOv
}
}
+ private boolean isDisableGroovy(@Nullable TableConfig offlineTableConfig,
@Nullable TableConfig realtimeTableConfig) {
+ Boolean offlineTableDisableGroovyQuery = null;
+ if (offlineTableConfig != null && offlineTableConfig.getQueryConfig() !=
null) {
+ offlineTableDisableGroovyQuery =
offlineTableConfig.getQueryConfig().getDisableGroovyQuery();
+ }
+
+ Boolean realtimeTableDisableGroovyQuery = null;
+ if (realtimeTableConfig != null && realtimeTableConfig.getQueryConfig() !=
null) {
+ realtimeTableDisableGroovyQuery =
realtimeTableConfig.getQueryConfig().getDisableGroovyQuery();
+ }
+
+ if (offlineTableDisableGroovyQuery == null &&
realtimeTableDisableGroovyQuery == null) {
+ return _disableGroovy;
+ }
+
+ // If offline or online table config disables Groovy, then Groovy should
be disabled
+ return (offlineTableDisableGroovyQuery == null ? false :
offlineTableDisableGroovyQuery) || (
+ realtimeTableDisableGroovyQuery == null ? false :
realtimeTableDisableGroovyQuery);
Review comment:
(minor) Can be simplified to
```suggestion
return Boolean.TRUE.equals(offlineTableDisableGroovyQuery) ||
Boolean.TRUE.equals(realtimeTableDisableGroovyQuery);
```
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/QueryConfig.java
##########
@@ -37,14 +37,27 @@
// because by the time the server times out, the broker should already timed
out and returned the response.
private final Long _timeoutMs;
+ // Table config override for disable Groovy broker property
+ private final Boolean _disableQueryGroovy;
+
@JsonCreator
- public QueryConfig(@JsonProperty("timeoutMs") @Nullable Long timeoutMs) {
+ public QueryConfig(@JsonProperty("timeoutMs") @Nullable Long timeoutMs,
+ @JsonProperty("disableQueryGroovy") @Nullable Boolean
disableQueryGroovy) {
Review comment:
Since this is query config, I think we can simplify the config key to
`disableGroovy`?
##########
File path:
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java
##########
@@ -251,6 +251,10 @@ public static void validateIngestionConfig(TableConfig
tableConfig, @Nullable Sc
*/
@VisibleForTesting
public static void validateIngestionConfig(TableConfig tableConfig,
@Nullable Schema schema, boolean disableGroovy) {
+ if (tableConfig.getQueryConfig() != null &&
tableConfig.getQueryConfig().getDisableGroovyQuery() != null) {
Review comment:
We should not use the query config to skip the check for ingestion
groovy.
After a second thought, we probably should not allow overriding ingestion
groovy using table config as the ingestion groovy itself is configured within
the table config.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
