[GitHub] [druid] LakshSingla commented on a change in pull request #12163: Add syntax support for PARTITIONED BY/CLUSTERED BY in INSERT queries

[GitBox <gi...@apache.org>]

LakshSingla commented on a change in pull request #12163:
URL: https://github.com/apache/druid/pull/12163#discussion_r796102894



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -765,13 +786,65 @@ static ParsedNodes create(final SqlNode node) throws ValidationException
       if (query.getKind() == SqlKind.INSERT) {
         insert = (SqlInsert) query;
         query = insert.getSource();
+
+        // Processing to be done when the original query has either of the PARTITION BY or CLUSTER BY clause
+        if (insert instanceof DruidSqlInsert) {

Review comment:
       Hi, as mentioned in the caveats section, what this PR achieves is adding a new top-level rule that is druid's explain the syntax. The issue is that explain is another top-level rule, which doesn't reference the druid's explain rule. Therefore `EXPLAIN PLAN FOR <druid's syntax>` is a syntactical error. 
   
   To make it work, we can create a Druid's explain rule like: 
   ```
   +SqlNode DruidSqlExplain() :
   +{
   +    SqlNode stmt;
   +    SqlExplainLevel detailLevel = SqlExplainLevel.EXPPLAN_ATTRIBUTES;
   +    SqlExplain.Depth depth;
   +    final SqlExplainFormat format;
   +}
   +{
   +    <EXPLAIN> <PLAN>
   +    [ detailLevel = ExplainDetailLevel() ]
   +    depth = ExplainDepth()
   +    (
   +        LOOKAHEAD(2)
   +        <AS> <XML> { format = SqlExplainFormat.XML; }
   +    |
   +        <AS> <JSON> { format = SqlExplainFormat.JSON; }
   +    |
   +        { format = SqlExplainFormat.TEXT; }
   +    )
   +    <FOR> stmt = SqlQueryOrDmlOrDruidQuery() {
   +        return new SqlExplain(getPos(),
   +            stmt,
   +            detailLevel.symbol(SqlParserPos.ZERO),
   +            depth.symbol(SqlParserPos.ZERO),
   +            format.symbol(SqlParserPos.ZERO),
   +            nDynamicParams);
   +    }
   +}
   +
   +
   +SqlNode SqlQueryOrDmlOrDruidQuery() :
   +{
   +    SqlNode stmt;
   +}
   +{
   +    (
   +        stmt = DruidSqlInsert()
   +    |
   +        stmt = SqlQueryOrDml()
   +    ) { return stmt; }
   +}
   ```
   i.e. the `SqlQueryOrDmlOrDruidQuery` clause is new thing. But this should IMO be for another PR, since this does copy syntax from the `Parser.jj`. (That's why I refrained from adding it in the first place here).
   




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

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



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