Dimitris Tsirogiannis has posted comments on this change. Change subject: PREVIEW: IMPALA-3725 Support Kudu UPSERT in Impala ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/4047/1//COMMIT_MSG Commit Message: PS1, Line 22: all of the columns in table_name Why is that? This is not a requirement for INSERT. Until we have nullable primary keys I believe the only requirement is that column list must contain the primary key columns. Also, it would be nice to say what happens to the columns that are not referenced in the column list. http://gerrit.cloudera.org:8080/#/c/4047/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS1, Line 363: InsertStmt upsert_stmt I believe you can write nonterminal InsertStmt insert_stmt, upsert_stmt; in L361 instead PS1, Line 662: upsert.setIsExplain(); : RESULT = upsert; nit: I think indentation is off by 1 :) PS1, Line 679: {: RESULT = new InsertStmt(w, table, true, list, hints, query, col_perm, ignore, false); :} nit: long lines http://gerrit.cloudera.org:8080/#/c/4047/1/fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java File fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java: Line 127: // True if this is an UPSERT operation. You may want to mention that USERT is only supported for Kudu tables. PS1, Line 447: All columns if this is an UPSERT. See my comment in the commit msg. I am not sure this restriction is correct. -- To view, visit http://gerrit.cloudera.org:8080/4047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
