Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-3725 Support Kudu UPSERT in Impala ......................................................................
Patch Set 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/4047/5/be/src/exec/kudu-table-sink.cc File be/src/exec/kudu-table-sink.cc: PS5, Line 140: DCHECK(sink_action_ == TSinkAction::DELETE) << "Sink type not supported. " > nit: can you fix this long line? Done http://gerrit.cloudera.org:8080/#/c/4047/5/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: PS5, Line 729: LPAREN opt_ident_list:col_perm RPAREN opt_query_stmt:que > nit: can you indent this line? Done PS5, Line 730: > nit: long line Done http://gerrit.cloudera.org:8080/#/c/4047/5/fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java File fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java: PS5, Line 83: : > Is this true for UPSERT as well? Done PS5, Line 462: : : : : : : > Hm I don't think this function is very useful. Why don't we inline this in Done PS5, Line 472: : : : : : > This comment is a bit confusing. First of all static and dynamic partition Done PS5, Line 731: : : : : > Can't you just do strBuilder.append(getOpName() + " "); Done http://gerrit.cloudera.org:8080/#/c/4047/5/fe/src/main/java/com/cloudera/impala/planner/KuduTableSink.java File fe/src/main/java/com/cloudera/impala/planner/KuduTableSink.java: PS5, Line 61: > Why is this not applied to UPSERT? This is basically displaying the value of ignoreDuplicates, which for upsert will always be false so its not really useful to include in the output. http://gerrit.cloudera.org:8080/#/c/4047/5/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java: PS5, Line 2454: > Also with a permutation list? Done PS5, Line 2593: : > Also one case with permutation list. Done Line 3480 > Do you have any positive test cases with permutation lists? Done http://gerrit.cloudera.org:8080/#/c/4047/5/testdata/workloads/functional-planner/queries/PlannerTest/insert.test File testdata/workloads/functional-planner/queries/PlannerTest/insert.test: PS5, Line 593: select string_col, count(*) from functional.alltypes group by > Can you make this case a bit more interesting so that the plan is not just Done http://gerrit.cloudera.org:8080/#/c/4047/5/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test: PS5, Line 289: upsert into table tdata (id, valf) values (2, 10), (120, 20) > Can you also add a case with a permutation list that results in a mix of in 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: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
