Jackie-Jiang commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r926113601
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -100,7 +100,7 @@ public void processSqlQueryGet(@ApiParam(value = "Query",
required = true) @Quer
try {
ObjectNode requestJson = JsonUtils.newObjectNode();
requestJson.put(Request.SQL, query);
- String queryOptions = constructSqlQueryOptions();
+ String queryOptions = constructSqlQueryOptions(requestJson);
Review Comment:
This line is redundant. We can skip putting the query options here
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -1449,6 +1449,9 @@ private boolean forceLog(BrokerResponse brokerResponse,
long totalTimeMs) {
@VisibleForTesting
static void setOptions(PinotQuery pinotQuery, long requestId, String query,
JsonNode jsonRequest) {
Map<String, String> queryOptions = new HashMap<>();
+ // TODO: Remove the SQL query options after releasing 0.11.0
Review Comment:
Let's put it in the end (just before setting the query options) to ensure
they don't get overridden. The query engine will break if these 2 options are
missing during version upgrade
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -133,7 +133,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();
+ String queryOptions = constructSqlQueryOptions(requestJson);
Review Comment:
Similarly, we can remove this and the next line
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BrokerRequestHandlerDelegate.java:
##########
@@ -74,9 +74,11 @@ 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")) {
+ String queryOptions = request.get("queryOptions").asText();
+ if
(queryOptions.contains(CommonConstants.Broker.Request.QueryOptionKey.USE_MULTISTAGE_ENGINE))
{
Review Comment:
We should probably check `USE_MULTISTAGE_ENGINE + "=true"` in case someone
set it to false
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
