Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-3725 Support Kudu UPSERT in Impala ......................................................................
Patch Set 8: (16 comments) http://gerrit.cloudera.org:8080/#/c/4047/7//COMMIT_MSG Commit Message: Line 18: query_stmt > More precisely this is a query_stmt because you can have a union/values/sel Done http://gerrit.cloudera.org:8080/#/c/4047/7/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 727: list.add(new Pair(slot, e)); > we also need to support plan hints for upsert Done Line 729: :} > Why is the query stmt optional here? For consistency with our insert stmt, though I'm not sure why insert does it that way. http://gerrit.cloudera.org:8080/#/c/4047/7/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: Line 83: insertStmt_ = InsertStmt.createInsert( > let's clean up this parameter hell with static createInsert/Upsert() helper Done http://gerrit.cloudera.org:8080/#/c/4047/7/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: Line 48: * Representation of a single insert or upsert statement, including the select statement > remove parens Done Line 84: // explicitly mentioned, will be assigned NULL values for INSERTs and left unassigned > There is some minor ambiguity with 'inserted' and 'updated'. It's not clear Done Line 134: public static InsertStmt createInsert(WithClause withClause, TableName targetTable, > To more easily distinguish the upsert/insert cases let's make the construct Done Line 193: table_ = null; > These cases don't parse, so they should be Preconditions in the c'tor Done Line 379: "(%s) because the column '%s' has an unsupported type '%s'.", > Not possible to parse this. Add Preconditions check in c'tor instead Done Line 683: } > also doesn't parse, but I think we should just allow plan hints (some usefu Done http://gerrit.cloudera.org:8080/#/c/4047/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: Line 2454: AnalyzesOk("values((1.0, 2, NULL), (2.0, 3, 4)) union all values(1, 2.0, 3)"); > We should either wrap these Kudu tests into: Done Line 2848: AnalyzesOk(String.format("load data inpath '%s' %s into table tpch.lineitem", > add same test for upsert Done Line 3493: > Somewhat misleading error msg because upsert is only supported for Kudu tab Done http://gerrit.cloudera.org:8080/#/c/4047/7/testdata/workloads/functional-planner/queries/PlannerTest/insert.test File testdata/workloads/functional-planner/queries/PlannerTest/insert.test: Line 573 > let's move this into a separate .test file that is run with Assume.assu Done Line 594 > also add a test where the primary-key columns are fed from the result of an Yeah, I'm not sure I know what you mean here. http://gerrit.cloudera.org:8080/#/c/4047/7/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test: Line 306: upsert into table tdata (valb, name, id) > remove, covered by analysis tests 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: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
