morningman commented on code in PR #18638:
URL: https://github.com/apache/doris/pull/18638#discussion_r1165252046


##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -2126,6 +2129,61 @@ private void handleCtasRollback(TableName table) {
         }
     }
 
+    private void handleIotsStmt() {
+        InsertOverwriteTableSelect iotsStmt = (InsertOverwriteTableSelect) 
this.parsedStmt;
+        try {
+            // create table
+            DdlExecutor.execute(context.getEnv(), 
iotsStmt.getCreateTableLikeStmt());
+            context.getState().setOk();
+        } catch (Exception e) {
+            // Maybe our bug
+            LOG.warn("IOTS create table error, stmt={}", 
originStmt.originStmt, e);
+            context.getState().setError(ErrorCode.ERR_UNKNOWN_ERROR, 
"Unexpected exception: " + e.getMessage());

Review Comment:
   just return if error happens.
   And no need to check the `context.getState().getStateType()` below.



##########
fe/fe-core/src/main/cup/sql_parser.cup:
##########
@@ -1825,6 +1827,11 @@ create_stmt ::=
     {:
         RESULT = new CreateTableLikeStmt(ifNotExists, name, existed_name, 
null, false);
     :}
+    | KW_INSERT KW_OVERWRITE KW_TABLE table_name:existed_name 
query_stmt:insert_data
+    {:
+        UUID uuid = UUID.randomUUID();
+        RESULT = new InsertOverwriteTableSelect(new CreateTableLikeStmt(false, 
new TableName(null,null,"tmp_"+uuid), existed_name, null, false),insert_data);

Review Comment:
   The name of tmp table should be generate in `analyze` phase, not in cup file 



##########
fe/fe-core/src/main/java/org/apache/doris/analysis/InsertOverwriteTableSelect.java:
##########
@@ -0,0 +1,69 @@
+// 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.doris.analysis;
+
+import org.apache.doris.common.ErrorCode;
+import org.apache.doris.common.ErrorReport;
+import org.apache.doris.common.UserException;
+
+import lombok.Getter;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class InsertOverwriteTableSelect extends DdlStmt {
+    @Getter
+    private final CreateTableLikeStmt createTableLikeStmt;
+
+    @Getter
+    private final InsertStmt insertStmt;
+    @Getter
+    private final AlterTableStmt alterTableStmt;
+    @Getter
+    private QueryStmt queryStmt;
+
+    public InsertOverwriteTableSelect(CreateTableLikeStmt createTableLikeStmt, 
QueryStmt queryStmt) {

Review Comment:
   The constructor of `InsertOverwriteTableSelect` should same as `InsertStmt`.
   And `CreateTableLikeStmt` and `QueryStmt` should be generated in analyze 
phase, not from cup file 



##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -2126,6 +2129,61 @@ private void handleCtasRollback(TableName table) {
         }
     }
 
+    private void handleIotsStmt() {
+        InsertOverwriteTableSelect iotsStmt = (InsertOverwriteTableSelect) 
this.parsedStmt;
+        try {
+            // create table
+            DdlExecutor.execute(context.getEnv(), 
iotsStmt.getCreateTableLikeStmt());
+            context.getState().setOk();
+        } catch (Exception e) {
+            // Maybe our bug
+            LOG.warn("IOTS create table error, stmt={}", 
originStmt.originStmt, e);
+            context.getState().setError(ErrorCode.ERR_UNKNOWN_ERROR, 
"Unexpected exception: " + e.getMessage());
+        }
+        // after success create table insert data
+        if (MysqlStateType.OK.equals(context.getState().getStateType())) {
+            try {
+                parsedStmt = iotsStmt.getInsertStmt();
+                parsedStmt.setUserInfo(context.getCurrentUserIdentity());
+                execute();
+                if 
(MysqlStateType.ERR.equals(context.getState().getStateType())) {
+                    LOG.warn("IOTS insert data error, stmt={}", 
iotsStmt.toSql());
+                    
handleIotsRollback(iotsStmt.getCreateTableLikeStmt().getDbTbl());
+                }
+            } catch (Exception e) {
+                LOG.warn("IOTS insert data error, stmt={}", iotsStmt.toSql(), 
e);
+                
handleIotsRollback(iotsStmt.getCreateTableLikeStmt().getDbTbl());
+            }
+        }
+
+        // overwrite old table with new table
+        try {
+            AlterTableStmt alterTableStmt = iotsStmt.getAlterTableStmt();
+            DdlExecutor.execute(context.getEnv(), alterTableStmt);
+            context.getState().setOk();

Review Comment:
   for user, he is executing an `insert` stmt, not a `create table` or `replace 
table` stmt.
   so the result we return should exactly same as `insert`.
   But here you call `context.getState().setOk()`, so the insert result is 
missing.



##########
fe/fe-core/src/main/cup/sql_parser.cup:
##########
@@ -1825,6 +1827,11 @@ create_stmt ::=
     {:
         RESULT = new CreateTableLikeStmt(ifNotExists, name, existed_name, 
null, false);
     :}
+    | KW_INSERT KW_OVERWRITE KW_TABLE table_name:existed_name 
query_stmt:insert_data
+    {:
+        UUID uuid = UUID.randomUUID();
+        RESULT = new InsertOverwriteTableSelect(new CreateTableLikeStmt(false, 
new TableName(null,null,"tmp_"+uuid), existed_name, null, false),insert_data);

Review Comment:
   And the place you modified is wrong, you should modify `insert_stmt ::=` 
entry, here is a diff for your:
   
   ```
   diff --git a/fe/fe-core/src/main/cup/sql_parser.cup 
b/fe/fe-core/src/main/cup/sql_parser.cup
   index fcb6f53e49..302c125362 100644
   --- a/fe/fe-core/src/main/cup/sql_parser.cup
   +++ b/fe/fe-core/src/main/cup/sql_parser.cup
   @@ -4513,6 +4513,10 @@ insert_stmt ::=
        {:
            RESULT = new InsertStmt(target, label, cols, source, hints);
        :}
   +    | KW_INSERT KW_OVERWRITE KW_INTO insert_target:target 
opt_with_label:label opt_col_list:cols opt_plan_hints:hints insert_source:source
   +    {:
   +        RESULT = new InsertOverwriteStmt(target, label, cols, source, 
hints);
   +    :}
        // TODO(zc) add default value for SQL-2003
        // | KW_INSERT KW_INTO insert_target:target KW_DEFAULT KW_VALUES
        ;
   ```



##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -769,6 +771,10 @@ public class SessionVariable implements Serializable, 
Writable {
     @VariableMgr.VarAttr(name = DROP_TABLE_IF_CTAS_FAILED, needForward = true)
     public boolean dropTableIfCtasFailed = true;
 
+    // Whether drop table when insert overwrite table insert data appear error.
+    @VariableMgr.VarAttr(name = DROP_TABLE_IF_IOTS_FAILED, needForward = true)
+    public boolean dropTableIfIotsFailed = true;

Review Comment:
   I think this config is unnecessary



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

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org

Reply via email to