You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2023/06/08 12:51:46 UTC

[GitHub] [pinot] tibrewalpratik17 opened a new pull request, #10873: Use pinot-query-planner utils to extract table names in pinot-controller

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

   Discussed in #10871 
   
   Using pinot-query-planner utils to get the tableNames for a particular query when queried by Pinot controller.
   


-- 
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] abhioncbr commented on a diff in pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1233167206


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -208,6 +210,21 @@ public String explainQuery(String sqlQuery) {
     return explainQuery(sqlQuery, CalciteSqlParser.compileToSqlNodeAndOptions(sqlQuery)).getExplainPlan();
   }
 
+  @VisibleForTesting
+  public List<String> getTableNamesForQuery(String sqlQuery) {
+    try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
+      RelRoot relRoot = compileQuery(CalciteSqlParser.compileToSqlNodeAndOptions(sqlQuery).getSqlNode(),
+          plannerContext);
+      Set<String> tableNames = RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel);
+      return new ArrayList<>(tableNames);
+    } catch (CalciteContextException e) {
+      throw new RuntimeException("Error composing query plan for '" + sqlQuery

Review Comment:
   NIT: we can merge these two catch blocks into one



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1230296445


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   this can be removed from `CalciteSqlParser.extractTableNamesFromNode`



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1237214589


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   sounds good thank you will take another look at #10955 as well. 



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1244098059


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -208,6 +210,24 @@ public String explainQuery(String sqlQuery) {
     return explainQuery(sqlQuery, CalciteSqlParser.compileToSqlNodeAndOptions(sqlQuery)).getExplainPlan();
   }
 
+  @VisibleForTesting

Review Comment:
   this is visible for controller to use as well. so not correct annotation



##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -156,7 +156,7 @@ private void testHardcodedQueriesCommon()
         "SELECT ArrDelay, CarrierDelay, (ArrDelay - CarrierDelay) AS diff FROM mytable WHERE ArrDelay > CarrierDelay "
             + "ORDER BY diff, ArrDelay, CarrierDelay LIMIT 100000";
     testQuery(query);
-    query = "SELECT count(*) FROM mytable WHERE AirlineID > 20355 AND OriginState BETWEEN 'PA' AND 'DE' AND DepTime <> "
+    query = "SELECT count(*) FROM mytable WHERE AirlineID > 20355 AND OriginState BETWEEN 'DE' AND 'PA' AND DepTime <> "

Review Comment:
   revert please



-- 
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] tibrewalpratik17 commented on a diff in pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1237089197


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   Hey @walterddr raised this #10955 to make the change in Pinot client to keep it independent of this change. Will remove the Calcite-util in this PR or the other one which one will merge later. Hope so it's okay! 
   
   Looking into the integration test failure of this one meanwhile.



-- 
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] tibrewalpratik17 commented on pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1607232924

   hey @walterddr i was looking into the integration test failure and this is only happening for queries with `BETWEEN` clause. 
   We lose the table information from RelNode at this method `trimUnusedFields` of Calcite - https://github.com/apache/pinot/blob/6a47311c03fdc020a4e05a3acf5cacb7843e3307/pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java#L292
   
   Digging deeper into the Calcite method shows that the `LogicalTableScan` node gets converted to `LogicalValues` node in this particular scenario. Testing this locally with `BETWEEN` clause on quickstart table works fine for me.
   
   Is this a known issue?


-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10873:
URL: https://github.com/apache/pinot/pull/10873


-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1244103752


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -208,6 +210,21 @@ public String explainQuery(String sqlQuery) {
     return explainQuery(sqlQuery, CalciteSqlParser.compileToSqlNodeAndOptions(sqlQuery)).getExplainPlan();
   }
 
+  @VisibleForTesting
+  public List<String> getTableNamesForQuery(String sqlQuery) {
+    try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
+      RelRoot relRoot = compileQuery(CalciteSqlParser.compileToSqlNodeAndOptions(sqlQuery).getSqlNode(),
+          plannerContext);
+      Set<String> tableNames = RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel);
+      return new ArrayList<>(tableNames);
+    } catch (CalciteContextException e) {
+      throw new RuntimeException("Error composing query plan for '" + sqlQuery

Review Comment:
   +1 let's merge the 2 error message



-- 
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] abhioncbr commented on a diff in pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1233167243


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   LGTM.



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1235850983


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   CC @tibrewalpratik17 can we do that in the Pinot client in this PR as well? if not i can create a follow up once this one is merged to remove the util



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1607977608

   > Yes @walterddr exactly! I have updated the QueryGenerator logic to swap values in `BETWEEN` clause if this case arises. But let me know if we want to follow any other route to solve this.
   
   what you mentioned here can have other corner cases so changing this is not a good solution


-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1235848505


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   i dont know if there's a SqlParser level util that can do that in calcite. but yes i think for now we can do the single broker selector. 



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1233352886


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   what's your thought on this @abhioncbr ? do you think we have a better way to handle table look up? ideally speaking shouldn't keep this hacky SqlNode parser level table finder b/c there's corner cases we can't really do on the syntactic parsing level.
   What i can think about is 
   - only allow simple broker selector if dealing with V2 engine queries for now. (so there's no need to extract table name, we blindly select a broker)
   - we should support controller based broker selector to ask controller to return the broker --> this way we avoid the table metadata problem b/c controller have access to all table metadata.
   
   Thoughts?



-- 
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] tibrewalpratik17 commented on pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1608257834

   > for now, I think it is better to fix the routing issue (e.g. if you can't find any table, route it to any random tenant)
   > later we should fix this issue --> we should not even dispatch this query out to begin with
   
   Sounds good! Updated based on this.
   Please review @walterddr 


-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1595597854

   the integration test failure seems legit. could you help take a look?


-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1607950700

   for now, I think it is better to fix the routing issue (e.g. if you can't find any table, route it to any random tenant)
   later we should fix this issue --> we should not even dispatch this query out to begin with


-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1582625293

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10873?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#10873](https://app.codecov.io/gh/apache/pinot/pull/10873?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c079fc7) into [master](https://app.codecov.io/gh/apache/pinot/commit/24b50ed65f5ff1405479cd4136fda85ffa99f9c1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (24b50ed) will **decrease** coverage by `70.12%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #10873       +/-   ##
   =============================================
   - Coverage     70.22%    0.11%   -70.12%     
   =============================================
     Files          2174     2132       -42     
     Lines        116871   114864     -2007     
     Branches      17702    17430      -272     
   =============================================
   - Hits          82078      137    -81941     
   - Misses        29058   114707    +85649     
   + Partials       5735       20     -5715     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `?` | |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | unittests2 | `?` | |
   | unittests2temurin17 | `0.11% <0.00%> (?)` | |
   
   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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10873?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...t/controller/api/resources/PinotQueryResource.java](https://app.codecov.io/gh/apache/pinot/pull/10873?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90UXVlcnlSZXNvdXJjZS5qYXZh) | `0.00% <0.00%> (-57.53%)` | :arrow_down: |
   | [...ntroller/helix/core/PinotHelixResourceManager.java](https://app.codecov.io/gh/apache/pinot/pull/10873?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-71.49%)` | :arrow_down: |
   | [.../java/org/apache/pinot/query/QueryEnvironment.java](https://app.codecov.io/gh/apache/pinot/pull/10873?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvUXVlcnlFbnZpcm9ubWVudC5qYXZh) | `0.00% <0.00%> (-94.12%)` | :arrow_down: |
   
   ... and [1982 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10873/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   


-- 
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] abhioncbr commented on a diff in pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "abhioncbr (via GitHub)" <gi...@apache.org>.
abhioncbr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1235478235


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   Yes, I agree `CalciteSqlParser.extractTableNamesFromNode` is quite hacky, and we often get exceptions related to it. 
   I think, for now, we can go with the first approach you have mentioned (blindly selecting a broker) and later figure out the long-term approach.
   
   Honestly, as for the second approach, I am not sure yet. I have to look into it. Also, I don't mind rewriting the hacky code in a better way(if possible) or using some static method from the Caclite(needs exploration).



-- 
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] tibrewalpratik17 commented on pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1607848525

   > ah. i think that's b/c when you run a query similar to
   > 
   > ```
   > SELECT * FROM tbl WHERE longCol BETWEEN 0 AND -1
   > ```
   > 
   > ^ this query is simplified by calcite into `SELECT * FROM emptyTable` --> b/c `BETWEEN 0 AND -1` is a constant false condition and thus it is returning empty row with the schema matching the initial table `tbl`
   > 
   > in this case you will have no table to find and thus no where to route.
   
   Yes @walterddr exactly! I have updated the QueryGenerator logic to swap values in `BETWEEN` clause if this case arises. But let me know if we want to follow any other route to solve this.


-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1232943998


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   hmm. ok. that is harder to incorporate b/c client doesnt really have full access to table names. 
   CC @abhioncbr 



-- 
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] tibrewalpratik17 commented on a diff in pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1232864254


##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -185,8 +190,10 @@ private String getMultiStageQueryResponse(String query, String queryOptions, Htt
       return QueryException.ACCESS_DENIED_ERROR.toString();
     }
 
-    SqlNodeAndOptions sqlNodeAndOptions = RequestUtils.parseQuery(query);
-    List<String> tableNames = CalciteSqlParser.extractTableNamesFromNode(sqlNodeAndOptions.getSqlNode());

Review Comment:
   Hey @walterddr i think we are still using this method in pinot-java-client which got introduced in #10692. 



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1603405893

   #10955 is merged. could you update this PR and clean up the util usage for the table name extractor. thank you!


-- 
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] xiangfu0 commented on a diff in pull request #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10873:
URL: https://github.com/apache/pinot/pull/10873#discussion_r1235942012


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java:
##########
@@ -253,6 +254,104 @@ public void testQueryWithHint()
     }
   }
 
+  @Test
+  public void testGetTableNamesForQuery() {
+    // A simple filter query with one table
+    String query = "Select * from a where col1 = 'a'";
+    List<String> tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 1);
+    Assert.assertEquals(tableNames.get(0), "a");
+
+    // query with IN / NOT IN clause
+    query = "SELECT COUNT(*) FROM a WHERE col1 IN (SELECT col1 FROM b) "
+        + "and col1 NOT IN (SELECT col1 from c)";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 3);
+    Collections.sort(tableNames);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+    Assert.assertEquals(tableNames.get(2), "c");
+
+    // query with JOIN clause
+    query = "SELECT a.col1, b.col2 FROM a JOIN b ON a.col3 = b.col3 WHERE a.col1 = 'a'";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 2);
+    Collections.sort(tableNames);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+
+    // query with WHERE clause JOIN
+    query = "SELECT a.col1, b.col2 FROM a, b WHERE a.col3 = b.col3 AND a.col1 = 'a'";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 2);
+    Collections.sort(tableNames);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+
+    // query with JOIN clause and table alias
+    query = "SELECT A.col1, B.col2 FROM a AS A JOIN b AS B ON A.col3 = B.col3 WHERE A.col1 = 'a'";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 2);
+    Collections.sort(tableNames);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+
+    // query with UNION clause
+    query = "SELECT * FROM a UNION ALL SELECT * FROM b UNION ALL SELECT * FROM c";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 3);
+    Collections.sort(tableNames);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+    Assert.assertEquals(tableNames.get(2), "c");
+
+    // query with UNION clause and table alias
+    query = "SELECT * FROM (SELECT * FROM a) AS t1 UNION SELECT * FROM ( SELECT * FROM b) AS t2";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 2);
+    Collections.sort(tableNames);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+
+    // query with UNION clause and table alias using WITH clause
+    query = "WITH tmp1 AS (SELECT * FROM a), \n"
+        + "tmp2 AS (SELECT * FROM b) \n"
+        + "SELECT * FROM tmp1 UNION ALL SELECT * FROM tmp2";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 2);
+    Collections.sort(tableNames);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+
+    // query with aliases, JOIN, IN/NOT-IN, group-by
+    query = "with tmp as (select col1, sum(col3) as col3, count(*) from a where col1 = 'a' group by col1), "
+        + "tmp2 as (select A.col1, B.col3 from b as A JOIN c AS B on A.col1 = B.col1) "
+        + "select sum(col3) from tmp where col1 in (select col1 from tmp2) and col1 not in (select col1 from d)";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 4);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+    Assert.assertEquals(tableNames.get(2), "c");
+    Assert.assertEquals(tableNames.get(3), "d");
+
+    // query with aliases, JOIN, IN/NOT-IN, group-by and explain
+    query = "explain plan for with tmp as (select col1, sum(col3) as col3, count(*) from a where col1 = 'a' "
+        + "group by col1), tmp2 as (select A.col1, B.col3 from b as A JOIN c AS B on A.col1 = B.col1) "
+        + "select sum(col3) from tmp where col1 in (select col1 from tmp2) and col1 not in (select col1 from d)";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 4);
+    Assert.assertEquals(tableNames.get(0), "a");
+    Assert.assertEquals(tableNames.get(1), "b");
+    Assert.assertEquals(tableNames.get(2), "c");
+    Assert.assertEquals(tableNames.get(3), "d");
+
+    // test for self join queries
+    query = "SELECT a.col1 FROM a JOIN(SELECT col2 FROM a) as self ON a.col1=self.col2 ";
+    tableNames = _queryEnvironment.getTableNamesForQuery(query);
+    Assert.assertEquals(tableNames.size(), 1);
+    Assert.assertEquals(tableNames.get(0), "a");
+  }

Review Comment:
   Please also add lateral join example introduced from: https://github.com/apache/pinot/pull/10933 



-- 
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 #10873: Use pinot-query-planner utils to extract table names in pinot-controller

Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10873:
URL: https://github.com/apache/pinot/pull/10873#issuecomment-1607758742

   ah. i think that's b/c when you run a query similar to 
   ```
   SELECT * FROM tbl WHERE longCol BETWEEN 0 AND -1
   ```
   ^ this query is simplified by calcite into `SELECT * FROM emptyTable` --> b/c BETWEEN 0 AND -1 is a constant false condition and thus it is returning empty row with the schema matching the initial table `tbl`
   
   in this case you will have no table to find and thus no where to route. 


-- 
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