Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610070509.u9759nOi000467@ip-10-146-233-104.ec2.internal>
Date: Fri, 7 Oct 2016 05:09:48 +0000
From: "Dimitris Tsirogiannis (Code Review)" <gerrit@cloudera.org>
To: Thomas Tauber-Marshall <tmarshall@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>
Reply-To: dtsirogiannis@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3725_Support_Kudu_UPSERT_in_Impala=0A?=
In-Reply-To: <gerrit.1471540247000.I8df5cea36b642e267f85ff6b163f3dd96b8386e9@gerrit.cloudera.org>
References: <gerrit.1471540247000.I8df5cea36b642e267f85ff6b163f3dd96b8386e9@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Fri, 07 Oct 2016 05:09:56 -0000

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