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 Thu, 20 Oct 2016 22:16:38 GMT
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3725 Support Kudu UPSERT in Impala
......................................................................


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4047/8/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

Line 689:       throw new AnalysisException("UPSERT does not currently support any plan hints.");
> make this a warning instead like we do for unrecognized hints
Done


http://gerrit.cloudera.org:8080/#/c/4047/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java:

Line 3456:   public void TestUpsert() {
> Let's move this into a new AnalyzeUpsertStmt.java file. This file is alread
Done


http://gerrit.cloudera.org:8080/#/c/4047/8/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

Line 1653:       ParsesOk(String.format("upsert into %s t [shuffle] select a from src", optTbl));
> move into TestPlanHints()
Done


http://gerrit.cloudera.org:8080/#/c/4047/8/testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
File testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test:

Line 40:    runtime filters: RF000 -> a.string_col
> add DISTRIBUTEDPLAN here also just to make sure we can generate one
Done


http://gerrit.cloudera.org:8080/#/c/4047/8/testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test:

Line 289: upsert into table tdata (id, valf) values (2, 10), (120, 20), (0, 0)
> throw in a few NULLs somewhere
Great catch! This actually didn't work as written.

The latest version fixes it, but the fix is unfortunately somewhat complicated.


Line 306: upsert into table tdata (valb, name, id)
> add an upsert without a query stmt
I assume that you're talking about how the query statement is optional for the parser when
the permutation is present, for consistency with insert?

If so, its not actually possible to successfully run such a query (at least at the moment),
since either you list some columns in the permutation, in which case you will get the error
that the number of columns in the permutation don't match the number of columns in the query
stmt (which is 0), or else you don't list columns in the permutation, in which case you will
get the error that you must list all of the primary key columns, and we already have parser/analyzer
tests for those situations.


-- 
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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@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