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

[GitHub] [pinot] abhioncbr opened a new pull request, #11052: [multistage] support physical plan in Explain queries

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

   As per the discussion for [issue](https://github.com/apache/pinot/issues/10901), This PR supports the physical plan in the `Explain` SQL queries 
   
   Here are the implementation details
   - We will add the physical plan for the query if the `With Implementation` option is set for the `Explain` SQL query.
   - By default, as per the Calcite SQL parser, if the `With` option is not provided in the query, it will be treated as `With Implementation`
   
   labels
      -  `feature`
      


-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   yes,  by default it's setting `SqlExplain.Depth.PHYSICAL`. 
   Please check class `org.apache.calcite.sql.parser.impl.SqlParserImpl` method `ExplainDepth()`, line number 5013. It's a generated file so I am missing the link here. I did check the calcite project, it's the same for Babel and a couple of other parsers impl. 
   
   That's why I was asking what type of default behaviour we want. 



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -185,16 +185,25 @@ public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAn
    * @param sqlNodeAndOptions parsed SQL query.
    * @return QueryPlannerResult containing the explained query plan and the relRoot.
    */
-  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions) {
+  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {
     try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
       SqlExplain explain = (SqlExplain) sqlNodeAndOptions.getSqlNode();
       plannerContext.setOptions(sqlNodeAndOptions.getOptions());
       RelRoot relRoot = compileQuery(explain.getExplicandum(), plannerContext);
       SqlExplainFormat format = explain.getFormat() == null ? SqlExplainFormat.DOT : explain.getFormat();
       SqlExplainLevel level =
           explain.getDetailLevel() == null ? SqlExplainLevel.DIGEST_ATTRIBUTES : explain.getDetailLevel();
-      Set<String> tableNames = RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel);
-      return new QueryPlannerResult(null, PlannerUtils.explainPlan(relRoot.rel, format, level), tableNames);
+      boolean showPhysicalPlan = explain.withImplementation();
+      String logicalPlan = PlannerUtils.explainPlan(relRoot.rel, format, level);
+      if (showPhysicalPlan) {
+        DispatchableSubPlan dispatchableSubPlan = toDispatchableSubPlan(relRoot, plannerContext, requestId);
+        String physicalPlan = ExplainPlanPlanVisitor.explain(dispatchableSubPlan);
+        String completePlan = String.format("%s\nPhysical Plan\n %s", logicalPlan, physicalPlan);

Review Comment:
   should only return the physicalPlan str instead of contacting both logical and physical



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   Right, default value of the Enum is PHYSICAL.
   
   All the explain queries `without implementation` or `with type` will result the physical plan. Is it good?



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -185,16 +185,25 @@ public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAn
    * @param sqlNodeAndOptions parsed SQL query.
    * @return QueryPlannerResult containing the explained query plan and the relRoot.
    */
-  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions) {
+  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {

Review Comment:
   i was wondering if it make more sense to separate the planQuery method into 2 individual utils 
   - planQueryLogical() --> produce `RelRoot` 
       - maybe even better change `compileQuery` to be named planQueryLogical()
   - planQueryDispatchable() --> produce `QueryPlanResult`
   
   and then both the planQuery and explainQuery can leverage the 2 method. it is becoming hard to track the differences between the explain and actual run part and they are largely the same



##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   can you share an example explain results on both approaches? 



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   I was going over to see how we can set the default to LOGICAL, which requires some hacks. There are two options I can think of,
   1. Maintaining `Parser.jj` copy in Pinot instead of extracting it from the calcite.jar. The parser has the [code](https://github.com/apache/calcite/blob/main/core/src/main/codegen/templates/Parser.jj#L1455) for setting the default value as Physical. I tried to put a similar function in our `parserImpls.ftl` to override the function, but it seems like `fmpp` has no such overriding functionality, and copying both methods failed while generating a parser class file.
   2. Before Parsing the `Explain` SQL query, check whether the `with` option is provided. If not, add `WITHOUT IMPLEMENTATION` in the query. 
   
   Please suggest a preferable option you have in mind or among the above two. I will try to dig more if I can get some other trick. Thanks



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   Added this temporary function to strip the physical plan from the output for unit tests. 
   
   @walterddr question, do we want physical and logical plans as default output based on Calcite default implementation, or do we want to change it? I will remove this temp function based on the feedback.



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   Thanks for mentioning the option and code; this is much better than the other two options I have suggested. Updated the changes.



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   thanks I will take another look later



-- 
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 pull request #11052: [multistage] support physical plan in Explain queries

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

   Added the [issue](https://github.com/apache/pinot/issues/11039) to support multiple formats for the physical plan so that the `Explain` query output will be of the same format.


-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   just updated the queries with the physical plan. 



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   see https://github.com/apache/pinot/commit/bd51dda7983e8332ed9ff16541502035e9fb0e11



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   ok now I understand the problem.. sorry for the confusion, it seems like we should set the default to LOGICAL instead of PHYSICAL



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   hmm. at this stage i think it is easier to not follow the calcite standard. and create a extension `SqlPhysicalExplain` node and a special parsing logic in `parserImpls.ftl` to directly use a different logic. WDYT?



-- 
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 #11052: [multistage] support physical plan in Explain queries

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

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11052?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#11052](https://app.codecov.io/gh/apache/pinot/pull/11052?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (38422be) into [master](https://app.codecov.io/gh/apache/pinot/commit/f7a076496ae6e07a42207cb2c978dc74562cf0cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f7a0764) will **increase** coverage by `0.00%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@           Coverage Diff           @@
   ##           master   #11052   +/-   ##
   =======================================
     Coverage    0.11%    0.11%           
   =======================================
     Files        2200     2200           
     Lines      118829   118808   -21     
     Branches    18023    17992   -31     
   =======================================
     Hits          137      137           
   + Misses     118672   118651   -21     
     Partials       20       20           
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1temurin11 | `?` | |
   | integration1temurin17 | `0.00% <0.00%> (ø)` | |
   | integration1temurin20 | `0.00% <0.00%> (ø)` | |
   | integration2temurin17 | `?` | |
   | integration2temurin20 | `0.00% <0.00%> (ø)` | |
   | unittests1temurin11 | `?` | |
   | unittests1temurin17 | `?` | |
   | unittests1temurin20 | `0.00% <0.00%> (ø)` | |
   | unittests2temurin11 | `0.11% <0.00%> (+<0.01%)` | :arrow_up: |
   | unittests2temurin17 | `0.11% <0.00%> (+<0.01%)` | :arrow_up: |
   | unittests2temurin20 | `0.11% <0.00%> (+<0.01%)` | :arrow_up: |
   
   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/11052?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...requesthandler/MultiStageBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/11052?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [.../java/org/apache/pinot/query/QueryEnvironment.java](https://app.codecov.io/gh/apache/pinot/pull/11052?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%> (ø)` | |
   
   ... and [22 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11052/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] walterddr commented on a diff in pull request #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   hmm. I am confused. do you mean `EXPLAIN PLAN FOR ...` by default also create the physical plan? that doesn't seem right



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   Depth is an enum with 3 values
   ```
     public static enum Depth implements Symbolizable {
       TYPE,
       LOGICAL,
       PHYSICAL;
   
       private Depth() {
       }
     }
   ```
   it is 3 different enum so the result should be just 3 different format and should not mix and match



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/parser/SqlPhysicalExplain.java:
##########
@@ -0,0 +1,32 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.sql.parsers.parser;
+
+import org.apache.calcite.sql.SqlExplain;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
+
+public class SqlPhysicalExplain extends SqlExplain {

Review Comment:
   This class is used to differentiate the physical plan level in SqlExplain vs the Pinot specific implementation plan so a java doc is probably needed to explain it
   



##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -61,6 +61,7 @@
 import org.apache.pinot.common.config.provider.TableCache;
 import org.apache.pinot.query.context.PlannerContext;
 import org.apache.pinot.query.planner.DispatchableSubPlan;
+import org.apache.pinot.query.planner.ExplainPlanPlanVisitor;

Review Comment:
   consider renaming this class to `PhysicalExplainPlanVisitor`? 



##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -158,12 +160,11 @@ public QueryEnvironment(TypeFactory typeFactory, CalciteSchema rootSchema, Worke
   public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {
     try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
       plannerContext.setOptions(sqlNodeAndOptions.getOptions());
-      RelRoot relRoot = compileQuery(sqlNodeAndOptions.getSqlNode(), plannerContext);
-      SubPlan subPlanRoot = toSubPlan(relRoot);
+      RelRoot relRoot = planQueryLogical(sqlNodeAndOptions.getSqlNode(), plannerContext);

Review Comment:
   since we use SqlPhysicalExplain, w no longer need the renamings, please revert back to `compileQuery` and `toDispatchableSubPlan`



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -185,16 +185,25 @@ public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAn
    * @param sqlNodeAndOptions parsed SQL query.
    * @return QueryPlannerResult containing the explained query plan and the relRoot.
    */
-  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions) {
+  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {

Review Comment:
   Implemented the changes.



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -185,16 +185,25 @@ public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAn
    * @param sqlNodeAndOptions parsed SQL query.
    * @return QueryPlannerResult containing the explained query plan and the relRoot.
    */
-  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions) {
+  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {
     try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
       SqlExplain explain = (SqlExplain) sqlNodeAndOptions.getSqlNode();
       plannerContext.setOptions(sqlNodeAndOptions.getOptions());
       RelRoot relRoot = compileQuery(explain.getExplicandum(), plannerContext);
       SqlExplainFormat format = explain.getFormat() == null ? SqlExplainFormat.DOT : explain.getFormat();
       SqlExplainLevel level =
           explain.getDetailLevel() == null ? SqlExplainLevel.DIGEST_ATTRIBUTES : explain.getDetailLevel();
-      Set<String> tableNames = RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel);
-      return new QueryPlannerResult(null, PlannerUtils.explainPlan(relRoot.rel, format, level), tableNames);
+      boolean showPhysicalPlan = explain.withImplementation();
+      String logicalPlan = PlannerUtils.explainPlan(relRoot.rel, format, level);
+      if (showPhysicalPlan) {
+        DispatchableSubPlan dispatchableSubPlan = toDispatchableSubPlan(relRoot, plannerContext, requestId);
+        String physicalPlan = ExplainPlanPlanVisitor.explain(dispatchableSubPlan);
+        String completePlan = String.format("%s\nPhysical Plan\n %s", logicalPlan, physicalPlan);

Review Comment:
   see the test in https://github.com/apache/calcite/blob/main/core/src/test/java/org/apache/calcite/test/JdbcTest.java
   ```
       with.query("explain plan as json for values (1, 'ab', TIMESTAMP '2013-04-02 00:00:00', 0.01)")
           .returns(expectedJson);
       with.query("explain plan with implementation for values (1, 'ab')")
           .returns("PLAN=EnumerableValues(tuples=[[{ 1, 'ab' }]])\n\n");
       with.query("explain plan without implementation for values (1, 'ab')")
           .returns("PLAN=LogicalValues(tuples=[[{ 1, 'ab' }]])\n\n");
       with.query("explain plan with type for values (1, 'ab')")
           .returns("PLAN=EXPR$0 INTEGER NOT NULL,\n"
               + "EXPR$1 CHAR(2) NOT NULL\n");
   ```
   for each enum (TYPE, LOGICAL, PHYSICAL) the result is just the part of the enum e.g. 
   - type is only the result types of each (EXPR$0, EXPR$1)
   - logical is LogicalValues(...)
   - physical is EnumerableValues(...)
   
   each is not containing the other



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   Right, so default value is PHYSICAL.
   
   All the explain queries `without implementation` or `with type` will result the physical plan. Is it good?



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   Added this temporary function to strip the physical plan from the output. 
   
   @walterddr question, do we want physical and logical plans per the default Calcite default implementation, or do we want to change it? I will remove this temp function based on the feedback.



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -185,16 +185,25 @@ public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAn
    * @param sqlNodeAndOptions parsed SQL query.
    * @return QueryPlannerResult containing the explained query plan and the relRoot.
    */
-  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions) {
+  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {

Review Comment:
   That makes sense, let me do the changes. 



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/test/java/org/apache/pinot/query/QueryEnvironmentTestBase.java:
##########
@@ -202,4 +205,10 @@ public String toString() {
       }
     }
   }
+
+  // temporary function to strip the Physical plan from the explain query plan.
+  // physical plan is present for the explain queries by default.

Review Comment:
   Added this temporary function to strip the physical plan from the output for unit tests. 
   
   @walterddr question, do we want physical and logical plans per the default Calcite default implementation, or do we want to change it? I will remove this temp function based on the feedback.



-- 
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 #11052: [multistage] support physical plan in Explain queries

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


##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -185,16 +185,25 @@ public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAn
    * @param sqlNodeAndOptions parsed SQL query.
    * @return QueryPlannerResult containing the explained query plan and the relRoot.
    */
-  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions) {
+  public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {
     try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
       SqlExplain explain = (SqlExplain) sqlNodeAndOptions.getSqlNode();
       plannerContext.setOptions(sqlNodeAndOptions.getOptions());
       RelRoot relRoot = compileQuery(explain.getExplicandum(), plannerContext);
       SqlExplainFormat format = explain.getFormat() == null ? SqlExplainFormat.DOT : explain.getFormat();
       SqlExplainLevel level =
           explain.getDetailLevel() == null ? SqlExplainLevel.DIGEST_ATTRIBUTES : explain.getDetailLevel();
-      Set<String> tableNames = RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel);
-      return new QueryPlannerResult(null, PlannerUtils.explainPlan(relRoot.rel, format, level), tableNames);
+      boolean showPhysicalPlan = explain.withImplementation();
+      String logicalPlan = PlannerUtils.explainPlan(relRoot.rel, format, level);
+      if (showPhysicalPlan) {
+        DispatchableSubPlan dispatchableSubPlan = toDispatchableSubPlan(relRoot, plannerContext, requestId);
+        String physicalPlan = ExplainPlanPlanVisitor.explain(dispatchableSubPlan);
+        String completePlan = String.format("%s\nPhysical Plan\n %s", logicalPlan, physicalPlan);

Review Comment:
   Why do we only want the physical plan? IMO, the `with implementation` clause suggests logical & physical both.



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