You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/07/19 16:50:49 UTC

[GitHub] [pinot] walterddr opened a new pull request, #9072: [UI] Add controller UI to use multi-stage engine

walterddr opened a new pull request, #9072:
URL: https://github.com/apache/pinot/pull/9072

   This PR enables access to multi-stage query engine on Controller UI. See:
   - Normal query UI doesn't change 
   
   ![Normal Query](https://user-images.githubusercontent.com/3581352/179805453-4c573a41-6d88-45ab-a7f4-0673aafbaad3.png)
   - Querying with JOIN syntax throws exception as expected when not using multi-stage engine
   
   ![JOIN query with checked MSE box](https://user-images.githubusercontent.com/3581352/179805684-84ddd23d-f17c-424d-bc44-b93518bce950.png)
   - Querying with JOIN syntax works with multi-stage engine checkbox
   - Query result will also show a warning banner for the experimental feature.
   
   ![JOIN query without checking MSE box](https://user-images.githubusercontent.com/3581352/179805674-8117e648-ae7f-4dd5-9eee-4004f9eddcbd.png)
   


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] walterddr merged pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
walterddr merged PR #9072:
URL: https://github.com/apache/pinot/pull/9072


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] codecov-commenter commented on pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9072:
URL: https://github.com/apache/pinot/pull/9072#issuecomment-1190460923

   # [Codecov](https://codecov.io/gh/apache/pinot/pull/9072?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#9072](https://codecov.io/gh/apache/pinot/pull/9072?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae3e4ed) into [master](https://codecov.io/gh/apache/pinot/commit/f0a4b443699bd550b1608e4dea7cd68bc9bc6a61?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f0a4b44) will **decrease** coverage by `54.74%`.
   > The diff coverage is `0.00%`.
   
   > :exclamation: Current head ae3e4ed differs from pull request most recent head 60c5516. Consider uploading reports for the commit 60c5516 to get more accurate results
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #9072       +/-   ##
   =============================================
   - Coverage     70.06%   15.31%   -54.75%     
   + Complexity     4743      170     -4573     
   =============================================
     Files          1832     1784       -48     
     Lines         96971    94879     -2092     
     Branches      14620    14383      -237     
   =============================================
   - Hits          67939    14533    -53406     
   - Misses        24305    79313    +55008     
   + Partials       4727     1033     -3694     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `15.31% <0.00%> (-0.04%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9072?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...pinot/broker/api/resources/PinotClientRequest.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdENsaWVudFJlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-58.50%)` | :arrow_down: |
   | [...r/requesthandler/BrokerRequestHandlerDelegate.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQnJva2VyUmVxdWVzdEhhbmRsZXJEZWxlZ2F0ZS5qYXZh) | `38.09% <0.00%> (-41.91%)` | :arrow_down: |
   | [...t/controller/api/resources/PinotQueryResource.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90UXVlcnlSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (-59.06%)` | :arrow_down: |
   | [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <ø> (-28.58%)` | :arrow_down: |
   | [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [1388 more](https://codecov.io/gh/apache/pinot/pull/9072/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] walterddr commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r925737126


##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -175,10 +175,14 @@ 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;

Review Comment:
   ok checked the codebase uage, none found for these 3 common constants. removing



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r925972885


##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -175,10 +175,14 @@ 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;

Review Comment:
   @walterddr No, we cannot remove it before cutting the next release. Let's keep it for a while because the previous version server relies on this to correctly process the query



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r925973468


##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -273,14 +273,6 @@ public static class QueryOptionKey {
         public static final String EXPLAIN_PLAN_VERBOSE = "explainPlanVerbose";
         public static final String USE_MULTISTAGE_ENGINE = "useMultistageEngine";
         public static final String ENABLE_NULL_HANDLING = "enableNullHandling";
-
-        // TODO: Remove these keys (only apply to PQL) after releasing 0.11.0

Review Comment:
   Don't remove these fields. `0.11.0` is not cut yet



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] walterddr commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r925995136


##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -175,10 +175,14 @@ 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;

Review Comment:
   sad.. ok. I will wait for 0.11 release cut before I merge this one then
   



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] kishoreg commented on pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
kishoreg commented on PR #9072:
URL: https://github.com/apache/pinot/pull/9072#issuecomment-1192175523

   instead of use multi-stage-engine, can we say enable-join-support?


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] KKcorps commented on pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #9072:
URL: https://github.com/apache/pinot/pull/9072#issuecomment-1189994790

   Tests seem to be running for a lot longer than user. 


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] walterddr commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r925688080


##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -175,10 +175,14 @@ 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;

Review Comment:
   can I delete this one now @Jackie-Jiang ^



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] walterddr commented on pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9072:
URL: https://github.com/apache/pinot/pull/9072#issuecomment-1190388180

   > Curious how was this leading to tears running longer ?
   
   b/c of the "Failed to meet condition in 600000ms" 
   


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] walterddr commented on pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9072:
URL: https://github.com/apache/pinot/pull/9072#issuecomment-1190369737

   > Tests seem to be running for a lot longer than user.
   
   found the issue was one of the legacy PQL query options.
   
   


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] siddharthteotia commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on code in PR #9072:
URL: https://github.com/apache/pinot/pull/9072#discussion_r925700562


##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -175,10 +175,14 @@ 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;

Review Comment:
   This should not be used anymore especially if legacy PQL code has been deleted 



-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] siddharthteotia commented on pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #9072:
URL: https://github.com/apache/pinot/pull/9072#issuecomment-1190381869

   > > Tests seem to be running for a lot longer than user.
   > 
   > found the issue was one of the legacy PQL query options.
   
   Curious how was this leading to tears running longer ? 


-- 
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #9072: [UI] Add controller UI to use multi-stage engine

Posted by GitBox <gi...@apache.org>.
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-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org