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, 19 Oct 2016 23:06:47 GMT
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 <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