Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-3725 Support Kudu UPSERT in Impala ......................................................................
Patch Set 2: (5 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 p This was Matt's suggestion. We don't currently know what the primary key columns are during analysis, as far as I can tell, so all we can do is pass the values off to Kudu and see if it returns an error. If the number of values to upsert is large, we might run into an error in the middle and end up in an inconsistent state, with some of the upsert values applied and some not. Of course, we can relax this requirement if we start storing info about primary keys so that we could throw an error during analysis, but its better to err on the side of being more restrictive for now, since its easier to add functionality than to take it away. There's also the issue of the semantics of leaving out columns, eg. do they get overwritten with null or are they left alone. I don't personally feel strongly about it either way, though, especially since as you point out we don't require this for insert. 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: DeleteStmt delete_stmt > I believe you can write nonterminal InsertStmt insert_stmt, upsert_stmt; in Done PS1, Line 662: RESULT = upsert; : :} > nit: I think indentation is off by 1 :) Done PS1, Line 679: RESULT = new InsertStmt(w, table, true, list, hints, query, col_perm, ignore, false); > nit: long lines Done 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. Only supported for Kudu tables. > You may want to mention that USERT is only supported for Kudu tables. Done -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
