[GitHub] [ignite] korlov42 commented on a change in pull request #9119: IGNITE-14555 Create table from query result

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

korlov42 commented on a change in pull request #9119:
URL: https://github.com/apache/ignite/pull/9119#discussion_r713153348



##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
     final SqlIdentifier id;
     final SqlNodeList columnList;
     final SqlNodeList optionList;
+    final SqlNode query;
 }
 {
     <TABLE>
     ifNotExists = IfNotExistsOpt()
     id = CompoundIdentifier()
-    columnList = TableElementList()
+    (
+        columnList = TableElementList()

Review comment:
       AFAIK column list in case of `CREATE TABLE ... AS` should contain only aliases without any types specs or constraints

##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
     final SqlIdentifier id;
     final SqlNodeList columnList;
     final SqlNodeList optionList;
+    final SqlNode query;
 }
 {
     <TABLE>
     ifNotExists = IfNotExistsOpt()
     id = CompoundIdentifier()
-    columnList = TableElementList()
+    (
+        columnList = TableElementList()
+    |
+        { columnList = null; }
+    )
+    (
+        <AS> query = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)

Review comment:
       it's better to have `AS` part as a last one to not to mess to which part of a statement a `WITH` clause belongs to. For example:
   
   ```
   1) CREATE TABLE my_t AS SELECT a, b FROM another_T WITH backups=3
   2) CREATE TABLE my_t WITH backups=3 AS SELECT a, b FROM another_T 
   ```
   The second option seems better to me.

##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/PlannerHelper.java
##########
@@ -107,6 +121,72 @@ public static IgniteRel optimize(SqlNode sqlNode, IgnitePlanner planner, IgniteL
         }
     }
 
+    /**
+     * Creates physical plan for "INSERT INTO table SELECT ..." based on "CREATE TABLE table AS SELECT ..." statement.
+     */
+    public static IgniteRel makeCreateTableAsSelectPlan(CreateTableCommand createTableCmd, PlanningContext ctx,

Review comment:
       Honestly speaking, I don't like the idea to create execution plan by hand. First, the table internals has leaked to the the planner level (row type format to feed to ModifyNode). And second, in such case we need to reimplement here all optimizations which planner would possibly apply. For example like this one https://issues.apache.org/jira/browse/IGNITE-12692.
   
   Have you considered an implicit rewriting of query with AS clause to two distinct statements (`CREATE` + `INSERT`)?

##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -148,19 +148,29 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
     final SqlIdentifier id;
     final SqlNodeList columnList;
     final SqlNodeList optionList;
+    final SqlNode query;
 }
 {
     <TABLE>
     ifNotExists = IfNotExistsOpt()
     id = CompoundIdentifier()
-    columnList = TableElementList()
+    (
+        columnList = TableElementList()
+    |
+        { columnList = null; }
+    )
+    (
+        <AS> query = OrderedQueryOrExpr(ExprContext.ACCEPT_QUERY)
+    |
+        { query = null; }
+    )
     (
         <WITH> { s.add(this); } optionList = CreateTableOptionList()
     |
         { optionList = null; }
     )
     {

Review comment:
       Please extend `org.apache.ignite.internal.processors.query.calcite.sql.SqlDdlParserTest` with tests to verify the new clause 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: notifications-unsubscribe@ignite.apache.org

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