Jackie-Jiang commented on code in PR #8711:
URL: https://github.com/apache/pinot/pull/8711#discussion_r874168340
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -94,6 +94,7 @@ public abstract class BaseBrokerRequestHandler implements
BrokerRequestHandler {
private static final Logger LOGGER =
LoggerFactory.getLogger(BaseBrokerRequestHandler.class);
private static final String IN_SUBQUERY = "insubquery";
private static final String IN_ID_SET = "inidset";
+ private static final boolean DEFAULT_DISABLE_GROOVY = true;
Review Comment:
Let's put this constant along with the `DISABLE_GROOVY` in
`CommonConstants.Broker`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -867,7 +867,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, true);
Review Comment:
Add a default constant for this
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1033,8 +1034,18 @@ private HandlerContext getHandlerContext(@Nullable
TableConfig offlineTableConfi
}
}
- // Disable Groovy if either offline or realtime table config disables
Groovy
- boolean disableGroovy = offlineTableDisableGroovyQuery |
realtimeTableDisableGroovyQuery;
+ // Disable Groovy in the following cases:
Review Comment:
Good findings. Actually `useApproximateFunction` has the same issue, and we
can fix them all together.
I feel it might be easier to read and understand if we separate the override
from the default, and use override if exists:
```
private HandlerContext getHandlerContext(@Nullable TableConfig
offlineTableConfig,
@Nullable TableConfig realtimeTableConfig) {
Boolean disableGroovyOverride = null;
Boolean useApproximateFunctionOverride = null;
if (offlineTableConfig != null && offlineTableConfig.getQueryConfig() !=
null) {
QueryConfig offlineTableQueryConfig =
offlineTableConfig.getQueryConfig();
Boolean disableGroovyOfflineTableOverride =
offlineTableQueryConfig.getDisableGroovy();
if (disableGroovyOfflineTableOverride != null) {
disableGroovyOverride = disableGroovyOfflineTableOverride;
}
Boolean useApproximateFunctionOfflineTableOverride =
offlineTableQueryConfig.getUseApproximateFunction();
if (useApproximateFunctionOfflineTableOverride != null) {
useApproximateFunctionOverride =
useApproximateFunctionOfflineTableOverride;
}
}
if (realtimeTableConfig != null && realtimeTableConfig.getQueryConfig()
!= null) {
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 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;
}
}
}
boolean disableGroovy = disableGroovyOverride != null ?
disableGroovyOverride : _disableGroovy;
boolean useApproximateFunction =
useApproximateFunctionOverride != null ?
useApproximateFunctionOverride : _useApproximateFunction;
return new HandlerContext(disableGroovy, useApproximateFunction);
}
```
--
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]
