impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <>
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:

Commit Message:

Line 18:   query_stmt
> More precisely this is a query_stmt because you can have a union/values/sel
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

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.
File fe/src/main/java/org/apache/impala/analysis/

Line 83:     insertStmt_ = InsertStmt.createInsert(
> let's clean up this parameter hell with static createInsert/Upsert() helper
File fe/src/main/java/org/apache/impala/analysis/

Line 48:  * Representation of a single insert or upsert statement, including the select statement
> remove parens

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

Line 134:   public static InsertStmt createInsert(WithClause withClause, TableName targetTable,
> To more easily distinguish the upsert/insert cases let's make the construct

Line 193:     table_ = null;
> These cases don't parse, so they should be Preconditions in the c'tor

Line 379:             "(%s) because the column '%s' has an unsupported type '%s'.",
> Not possible to parse this. Add Preconditions check in c'tor instead

Line 683:     }
> also doesn't parse, but I think we should just allow plan hints (some usefu
File fe/src/test/java/org/apache/impala/analysis/

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:

Line 2848:       AnalyzesOk(String.format("load data inpath '%s' %s into table tpch.lineitem",
> add same test for upsert

Line 3493: 
> Somewhat misleading error msg because upsert is only supported for Kudu tab
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

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.
File testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test:

Line 306: upsert into table tdata (valb, name, id)
> remove, covered by analysis tests

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-HasComments: Yes

View raw message