Lars Volker has posted comments on this change. Change subject: IMPALA-2521: Add clustered hint to insert statements ......................................................................
Patch Set 8: (7 comments) Thanks for the review, please see PS8, as well as my comment regarding expr substitution. http://gerrit.cloudera.org:8080/#/c/4745/5/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: Line 132: private ArrayList<Expr> primaryKeyExprs_ = Lists.newArrayList(); > how about primaryKeyExprs_? Done Line 517: * expressions. > Mention that as point 4 Done Line 601: } > Kudu (it's a proper noun) Done Line 602: } > You also need to substitute the kuduExprs_ in InsertStmt.substituteResultEx I added a test for this, however it also seems to work without the substitution. What am I missing? Also, are c_i and k_j particular columns? I tried reordering them, but it still does not fail when omitting the substitution. Line 604: if (matchFound && table_ instanceof KuduTable) { > add helper KuduTable.isPrimaryKeyColumn() Done, though after finding that List supports contains I might as well inline it as it is just one line. Inline or Keep? http://gerrit.cloudera.org:8080/#/c/4745/5/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 499: * (key columns for Kudu tables), so that partitions can be written sequentially in the > Kudu tables Done http://gerrit.cloudera.org:8080/#/c/4745/5/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu.test: Line 248: insert into table functional_kudu.alltypes /*+ clustered */ > shuffle/noshuffle don't do anything for Kudu tables, so I don't think this Done -- To view, visit http://gerrit.cloudera.org:8080/4745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
