Jackie-Jiang commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r927101369
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BrokerRequestHandlerDelegate.java:
##########
@@ -74,9 +75,12 @@ public BrokerResponse handleRequest(JsonNode request,
@Nullable RequesterIdentit
RequestContext requestContext)
throws Exception {
if (_isMultiStageQueryEngineEnabled && _multiStageWorkerRequestHandler !=
null) {
- JsonNode node =
request.get(CommonConstants.Broker.Request.QueryOptionKey.USE_MULTISTAGE_ENGINE);
- if (node != null && node.asBoolean()) {
- return _multiStageWorkerRequestHandler.handleRequest(request,
requesterIdentity, requestContext);
+ if (request.has("queryOptions")) {
+ Map<String, String> queryOptionMap =
BaseBrokerRequestHandler.getOptionsFromJson(request, "queryOptions");
+ if ("true".equalsIgnoreCase(queryOptionMap.getOrDefault(
+
CommonConstants.Broker.Request.QueryOptionKey.USE_MULTISTAGE_ENGINE, "false")))
{
Review Comment:
```suggestion
if (Boolean.parseBoolean(queryOptionMap.get(
CommonConstants.Broker.Request.QueryOptionKey.USE_MULTISTAGE_ENGINE))) {
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1477,6 +1477,10 @@ static void setOptions(PinotQuery pinotQuery, long
requestId, String query, Json
if (!queryOptions.isEmpty()) {
LOGGER.debug("Query options are set to: {} for request {}: {}",
queryOptions, requestId, query);
}
+ // TODO: Remove the SQL query options after releasing 0.11.0
Review Comment:
Suggest moving it before `pinotQuery.setQueryOptions(queryOptions);`
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -175,10 +171,12 @@ private BrokerResponse executeSqlQuery(ObjectNode
sqlRequestJson, HttpRequesterI
}
}
- // TODO: Remove the SQL query options after releasing 0.11.0
- private String constructSqlQueryOptions() {
- return Request.QueryOptionKey.GROUP_BY_MODE + "=" + Request.SQL + ";" +
Request.QueryOptionKey.RESPONSE_FORMAT + "="
- + Request.SQL;
+ private String constructSqlQueryOptions(JsonNode requestJson) {
Review Comment:
This can be removed
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -133,9 +131,7 @@ public void processSqlQueryPost(String query, @Suspended
AsyncResponse asyncResp
if (!requestJson.has(Request.SQL)) {
throw new IllegalStateException("Payload is missing the query string
field 'sql'");
}
- String queryOptions = constructSqlQueryOptions();
- // the only query options as of now are sql related. do not allow any
custom query options in sql endpoint
- ObjectNode sqlRequestJson = ((ObjectNode)
requestJson).put(Request.QUERY_OPTIONS, queryOptions);
+ ObjectNode sqlRequestJson = (ObjectNode) requestJson;
Review Comment:
(nit) inline it
--
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]
