[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 19: Code-Review+2

-- 
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: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new patch set (#18).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  query_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
A fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
17 files changed, 665 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/18
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 17: Code-Review+2

Ok thanks, it looked like this was just a rebase just wanted to check there 
wasn't anything new.

FYI this change will conflict with this one which is also in gvo:
https://gerrit.cloudera.org/#/c/4911/3

Depending on which one gets in first, one of us will have to do a rebase and 
fix some conflicts :/

-- 
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: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

Re: [Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall]

On Fri, Nov 4, 2016 at 11:26 AM Matthew Jacobs (Code Review) <
ger...@cloudera.org> wrote:

> Matthew Jacobs has posted comments on this change.
>
> Change subject: IMPALA-3725 Support Kudu UPSERT in Impala
> ..
>
>
> Patch Set 17: Code-Review+2
>
> Ok thanks, it looked like this was just a rebase just wanted to check
> there wasn't anything new.
>
> FYI this change will conflict with this one which is also in gvo:
> https://gerrit.cloudera.org/#/c/4911/3
>
> Depending on which one gets in first, one of us will have to do a rebase
> and fix some conflicts :/
>

Alright, since the gvo I started failed due to the Jenkins issue anyways,
I'll wait until yours is in and do the rebase.


>
> --
> 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: 17
> Gerrit-Project: Impala-ASF
> Gerrit-Branch: master
> Gerrit-Owner: Thomas Tauber-Marshall 
> Gerrit-Reviewer: Alex Behm 
> Gerrit-Reviewer: Dimitris Tsirogiannis 
> Gerrit-Reviewer: Internal Jenkins
> Gerrit-Reviewer: Matthew Jacobs 
> Gerrit-Reviewer: Thomas Tauber-Marshall 
> Gerrit-HasComments: No
>

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 17:

The GVO failed due to some Jenkins issue. Looks like the machine ran out of 
disk space: 
http://sandbox.jenkins.cloudera.com/computer/impala-boost-static-burst-slave-0980.vpc.cloudera.com/

The latest patch was just a rebase.

-- 
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: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

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


Patch Set 17: Verified-1

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/418/

-- 
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: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Hello Matthew Jacobs, Internal Jenkins, Alex Behm,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4047

to look at the new patch set (#17).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  query_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
A fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
17 files changed, 665 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/17
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 16:

(1 comment)

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

PS16, Line 33: // SELECT clause
> coverage of all data types
(These comments in this file were notes I was making and submitted by mistake 
when I left the comment just a moment ago about being able to submit soon. You 
may consider these comments when looking through the overall DML coverage, but 
they don't need to block getting UPSERT in.)


-- 
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: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 16:

(2 comments)

The build is unblocked so you can try to get this in, but I think you'll have 
to rebase and I'm not sure if there will be any conflicts.

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

PS16, Line 29: AnalyzesOk("upsert into table functional_kudu.testtbl 
values(1, 'a', 1)");
 : AnalyzesOk("upsert into table functional_kudu.testtbl(id) 
values(1)");
 : AnalyzesOk("upsert into table functional_kudu.testtbl 
values(1, 'a', 1), " +
 : "(2, 'b', 2), (3, 'c', 3)");
1. coverage of all data types
2. cols list in different order from the tbl


PS16, Line 33: // SELECT clause
coverage of all data types


-- 
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: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Hello Matthew Jacobs, Internal Jenkins, Alex Behm,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4047

to look at the new patch set (#16).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  query_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
A fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
17 files changed, 678 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/16
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 15:

(1 comment)

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

Line 729: if (!planHints_.isEmpty() && table_ instanceof HBaseTable) {
> It already passes through SHUFFLE/NOSHUFFLE for INSERT into Kudu tables. Is
Yeah you're right it was allowing  SHUFFLE/NOSHUFFLE for Kudu before your 
change, but that was a mistake. Sorry to hold this up then, but can you fix it 
while you're here and add the negative test cases in the analyzer tests?


-- 
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 15:

(1 comment)

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

Line 729: if (!planHints_.isEmpty() && table_ instanceof HBaseTable) {
> But now this passes through SHUFFLE/NOSHUFFLE as well...
It already passes through SHUFFLE/NOSHUFFLE for INSERT into Kudu tables. Is 
SHUFFLE/NOSHUFFLE valid for INSERT into Kudu tables but not for UPSERT, or is 
it a mistake that SHUFFLE/NOSHUFFLE works for Kudu INSERT?


-- 
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Hello Matthew Jacobs, Internal Jenkins, Alex Behm,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4047

to look at the new patch set (#15).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  query_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
A fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
17 files changed, 665 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/15
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

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


Patch Set 14: Verified-1

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/368/

-- 
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: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

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


Patch Set 13: Verified+1

-- 
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: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Hello Matthew Jacobs, Alex Behm,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4047

to look at the new patch set (#13).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  query_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
A fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
17 files changed, 656 insertions(+), 84 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/13
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 13
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 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 12: Code-Review+1

(1 comment)

Carrying forward Alex's +2

http://gerrit.cloudera.org:8080/#/c/4047/11/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

Line 174: } else {
> else DCHECK that the referenced columns are empty?
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: 12
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

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

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


Patch Set 11: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4047/11/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

Line 174: }
else DCHECK that the referenced columns are empty?


-- 
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: 11
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

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4047

to look at the new patch set (#10).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  query_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
A fe/src/test/java/org/apache/impala/analysis/AnalyzeUpsertStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
17 files changed, 651 insertions(+), 85 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/10
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 10
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 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 10:

(8 comments)

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

Line 121:   // If this is an INSERT, will contain one Expr for all 
non-partition columns of the
> ... will contain one Expr for all non-partition columns of the target table
Done


Line 124:   // If this is an UPSERT, will contain one Expr per column mentioned 
in the query and
> will contain one Expr per mentioned column
Done


Line 128: 
> As discussed, please clarify the "if (value == NULL)" part in the BE kudu-t
Done


Line 131:   // Only used for UPSERT, set in prepareExpressions().
> mentionedUpsertColumns_?
Done


Line 664: ArrayList columns = table_.getColumnsInHiveOrder();
> ++col
Done


Line 670:   resultExprs_.add(selectListExprs.get(i));
> single line
Done


Line 754: return TableSink.create(table_, isUpsert_ ? TableSink.Op.UPSERT : 
TableSink.Op.INSERT,
> easier to add a Preconditions.checkState(isUpsert_ || referencedColumns_.is
Done


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

Line 307: select false as valb, 'he' as name, id from tdata where id < 2
> let's also add an upsert test to see what happens when a PK column is NULL
It results in the query getting aborted, which as far as I can tell is not 
something that our query test framework can currently handle.


-- 
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: 10
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

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

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


Patch Set 8:

(7 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


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 already 
huge enough :)


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()


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 594
> Yeah, I'm not sure I know what you mean here.
Something like this:

upsert into functional_kudu.testtbl
select v.id, v.int_col, c.cnt from (
  select id, int_col, count(*) cnt from
  functional.alltypes
  group by 1, 2) v
where cnt < 10


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


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


Line 306: upsert into table tdata (valb, name, id)
add an upsert without a query stmt


-- 
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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4047

to look at the new patch set (#8).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  query_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
A testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
16 files changed, 557 insertions(+), 74 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/8
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
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 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

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


Patch Set 7:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/4047/7//COMMIT_MSG
Commit Message:

Line 18:   select_stmt
More precisely this is a query_stmt because you can have a union/values/select 
stmt.


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: upsert_stmt ::=
we also need to support plan hints for upsert


Line 729: LPAREN opt_ident_list:col_perm RPAREN opt_query_stmt:query
Why is the query stmt optional here?


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_ = new InsertStmt(
let's clean up this parameter hell with static createInsert/Upsert() helpers


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


Line 84:   // explicitly mentioned, will be assigned NULL values for inserted 
rows or left as is
There is some minor ambiguity with 'inserted' and 'updated'. It's not clear 
whether 'updated' only refers to those rows that already existed before an 
upsert. I think this can be simplified/clarified.

Maybe something like:

... will be assigned NULL values for INSERTs and left unassigned for UPSERTs.


Line 134:   public InsertStmt(WithClause withClause, TableName targetTable, 
boolean overwrite,
To more easily distinguish the upsert/insert cases let's make the constructor 
protected and add two static functions for creating an insert and upsert stmt 
respectively.

public static InsertStmt createInsert(...);
public static InsertStmt createUpsert(...);


Line 193: throw new AnalysisException("UPSERT is not compatible with 
OVERWRITE");
These cases don't parse, so they should be Preconditions in the c'tor


Line 379: if (partitionKeyValues_ != null && 
!partitionKeyValues_.isEmpty()) {
Not possible to parse this. Add Preconditions check in c'tor instead


Line 683: if (isUpsert_) throw new AnalysisException("UPSERT does not 
support plan hints.");
also doesn't parse, but I think we should just allow plan hints (some useful 
ones are coming pretty soon)


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("upsert into table functional_kudu.testtbl values(1, 
'a', 1)");
We should either wrap these Kudu tests into:

if (RuntimeEnv.INSTANCE.isKuduSupported()) {
// tests go here
}

or we should move these tests into more Kudu-specific places where we can do 
that check wholesale

You can look at the uses of RuntimeEnv.INSTANCE.isKuduSupported()) to find a 
few interesting places.

The problem with leaving this as is is that anyone running a system that does 
not support Kudu will be unable run run tests (I believe they will just hang)

I think it might make sense to just add a new AnalyzeUpsertStmtTest


Line 2848: AnalysisError("insert into functional.alltypes_view 
partition(year, month) " +
add same test for upsert


Line 3493: "All key columns must be specified for Kudu tables. Missing 
columns are: id");
Somewhat misleading error msg because upsert is only supported for Kudu tables. 
Maybe rephrase to something like:

"All primary key columns must be specified for UPSERT. Missing columns are: "


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: # simple upsert with select
let's move this into a separate .test file that is run with 
Assume.assumeTrue(RuntimeEnv.INSTANCE.isKuduSupported()) like the other Kudu 
planner tests


Line 594: upsert into table functional_kudu.testtbl
also add a test where the primary-key columns are fed from the result of an 
inline view (let me know if this needs clarification)


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) values (true), (false)
remove, covered by analysis tests


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 6:

(1 comment)

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

PS6, Line 272: VARCHAR(20)), false),
 : (1, 'known', 1, 0, cast('a' as VARCHAR
> That's just the type of the column in the table, see the other queries in t
The type of the column is STRING, though yes, some queries are casting literals 
to VARCHARs. I guess that's to test implicit casts, which is orthogonal to 
UPSERT. Anyway, fine to leave it.


-- 
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 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

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


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4047/6/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

PS6, Line 734: RESULT = new InsertStmt(w, table, false, null, null, query, 
null, false, true); :}
nit: can you add some newlines to make this spaced like the case above?


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

PS6, Line 75: all non-partition columns in Hive
:   // order.
doesn't quite make sense for the Kudu code. Can you indicate how it's handled 
in that case?


PS6, Line 342: setTargetTable
This does more than sets the table. analyzeTargetTable() would be more accurate.


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

PS6, Line 869: ParsesOk("with t as (select 1) upsert into x with t as 
(select 2) select * from t");
 : ParsesOk("with t(c1) as (select 1) " +
 : "upsert into x with t(c2) as (select 2) select * from 
t");
This removes the insert test cases, I assume you didn't mean to change these 
lines


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

PS6, Line 272: VARCHAR(20)), false),
 : (1, 'known', 1, 0, cast('a' as VARCHAR
why varchars?


-- 
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 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

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 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new patch set (#6).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT and uses all of the analysis,
planning, and execution machinery as INSERT, except that if
there's a primary key collision instead of returning an error an
update is performed.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  select_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in
table_name, if specified, and table_name must be a Kudu table.

This patch also improves the behavior of INSERTing into Kudu
tables without specifying all of the key columns - this now
results in an analysis exception, rather than attempting the
INSERT and receiving an error back from Kudu.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/planner/KuduTableSink.java
M fe/src/main/java/org/apache/impala/planner/TableSink.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
15 files changed, 514 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change.

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


Patch Set 5:

(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. " << sink_action_;
nit: can you fix this long line?


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:query
nit: can you indent this line?


PS5, Line 730: RESULT = new InsertStmt(w, table, false, null, null, query, 
col_perm, false, true); :}
nit: long line


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: Other columns, if not
:   // explicitly mentioned, will be assigned NULL values
Is this true for UPSERT as well?


PS5, Line 462: if (table_ instanceof KuduTable) {
 :   if (partitionKeyValues_ != null) {
 : throw new AnalysisException("PARTITION clause is not 
valid for Kudu tables.");
 :   }
 : } else {
 :   throw new AnalysisException("UPSERT is only supported for 
Kudu tables");
 : }
Hm I don't think this function is very useful. Why don't we inline this in L190 
and change the condition in L376?


PS5, Line 472: Checks that the column permutation + select list + static 
partition exprs + dynamic
 :* partition exprs collectively cover exactly all required 
columns in the target table,
 :* where the required columns are:
 :* - Any partition columns.
 :* - All key columns if the target is a Kudu table.
 :* - The row-key column if the target is an HBase table
This comment is a bit confusing. First of all static and dynamic partition 
exprs don't exist in the context of Kudu tables. Second, as the comment 
suggests, this function has too many responsibilities. Can you make an attempt 
to separate the logic that is specific to each table type while keeping the 
parts that are common (L527-552)?


PS5, Line 731: if (isUpsert_) {
 :   strBuilder.append("UPSERT ");
 : } else {
 :   strBuilder.append("INSERT ");
 : }
Can't you just do strBuilder.append(getOpName() + " ");


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: output.append(detailPrefix);
Why is this not applied to UPSERT?


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: AnalyzesOk("upsert into table functional_kudu.testtbl values(1, 
'a', 1)");
Also with a permutation list?


PS5, Line 2593: AnalyzesOk("upsert into functional_kudu.testtbl with t1 as 
(select * from " +
  : "functional.alltypes) select bigint_col, string_col, 
int_col from t1");
Also one case with permutation list.


Line 3480: 
Do you have any positive test cases with permutation lists?
- only primary keys
- primary keys + some other columns
- columns in permutation list are not in the same order as columns in target 
table


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 * from functional.alltypes where year=2009 and month=05
Can you make this case a bit more interesting so that the plan is not just 
upsert followed by a scan? e.g. add a join/agg in the view


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 
inserts and updates? Also a case where the columns in the permutation list are 
not in the same order as the columns in the target table.


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

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


Patch Set 5:

The latest version moves back to using a single class, InsertStmt, rather than 
having InsertStmt and UpsertStmt, since that separation turned out to be messy. 
Instead, it separates out some of the upsert vs. insert -specific analysis into 
separate methods withing InsertStmt to make it clearer what applies when.

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new patch set (#4).

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

IMPALA-3725 Support Kudu UPSERT in Impala

This patch introduces a new query statement, UPSERT, for Kudu
tables which operates like an INSERT except that if there's a
primary key collision instead of returning an error an update
is performed.

This patch adds an analysis class UpsertStmt, which represents
UPSERT statements, and InsertStmtBase, which is a superclass of
UpsertStmt and InsertStmt and contains logic common to both.

New syntax:
[with_clause] UPSERT INTO [TABLE] table_name [(column list)]
{
  select_stmt
 | VALUES (value [, value...]) [, (value [, (value...)]) ...]
}

where column list must contain all of the key columns in table_name,
if specified, and table_name must be a Kudu table.

Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
---
M be/src/exec/kudu-table-sink.cc
M common/thrift/DataSinks.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java
M fe/src/main/java/com/cloudera/impala/analysis/InsertStmt.java
A fe/src/main/java/com/cloudera/impala/analysis/InsertStmtBase.java
M fe/src/main/java/com/cloudera/impala/analysis/StmtRewriter.java
A fe/src/main/java/com/cloudera/impala/analysis/UpsertStmt.java
M fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java
M fe/src/main/java/com/cloudera/impala/planner/KuduTableSink.java
M fe/src/main/java/com/cloudera/impala/planner/Planner.java
M fe/src/main/java/com/cloudera/impala/planner/TableSink.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/jflex/sql-scanner.flex
M fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/com/cloudera/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/com/cloudera/impala/analysis/AnalyzerTest.java
M fe/src/test/java/com/cloudera/impala/analysis/ParserTest.java
M fe/src/test/java/com/cloudera/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/insert.test
M testdata/workloads/functional-query/queries/QueryTest/kudu_crud.test
21 files changed, 990 insertions(+), 479 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/47/4047/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8df5cea36b642e267f85ff6b163f3dd96b8386e9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall