impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala
Date Wed, 12 Oct 2016 02:15:32 GMT
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message