impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-3725 Support Kudu UPSERT in Impala
Date Fri, 07 Oct 2016 05:09:48 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-3725 Support Kudu UPSERT in Impala

Patch Set 5:

File be/src/exec/

PS5, Line 140: DCHECK(sink_action_ == TSinkAction::DELETE) << "Sink type not supported.
" << sink_action_;
nit: can you fix this long line?
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
File fe/src/main/java/com/cloudera/impala/analysis/

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
             :       }
             :     } 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 +
             :    * 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() + " ");
File fe/src/main/java/com/cloudera/impala/planner/

PS5, Line 61: output.append(detailPrefix);
Why is this not applied to UPSERT?
File fe/src/test/java/com/cloudera/impala/analysis/

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
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
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
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
To unsubscribe, visit

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

View raw message