Lars Volker has posted comments on this change. Change subject: IMPALA-2521: Add clustered hint to insert statements ......................................................................
Patch Set 1: (12 comments) Thanks for the reviews, please see PS2 http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: Line 121: private boolean hasClusteredHint_ = false; > might be better to use ternary logic here with a single Boolean (which can We actually only need the ternary logic to detect conflicting hints, so I made it a boolean and used another local variable in analyzePlanHints() to detect that error. Line 631: if (hasNoShuffleHint_) { > did you mean noclustered? Yes, done. Line 632: throw new AnalysisException("Conflicting INSERT hint: " + hint); > list both conflicting hints (otherwise it's hard to tell how to fix the pro Done http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/analysis/SortInfo.java File fe/src/main/java/org/apache/impala/analysis/SortInfo.java: Line 68: * sorted. Its source exprs are update to those in tupleSlotExprs. > second sentence incomprehensible. Done Line 123: * the exprs used by the SortNode refer to the materialized tuples instead of the > don't refer to the sort node here, SortInfo shouldn't have to know about th Done http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: Line 269: * clustered/noclustered plan hint. The clustering re-order the data in 'inputFragment' > Better to be specific: Say that it adds a sort node that orders on the part Done Line 272: // TODO: this function shares some code with QueryStmt::createSortTupleInfo(). However > You could move most of the code into SortInfo.createSortTupleInfo(List<Expr Done, however I couldn't see how that signature would fit in, so now I'm passing the set of source slot refs instead. I also had to modify the StatementBase class to make the factored-out code work with both QueryStmt and InsertStmt objects. Line 275: public PlanFragment addClusteringFragment(PlanFragment inputFragment, > you should move the logic (minus the createSortTupleInfo part that alex bro createInsertFragment has several early exits (return inputFragment) and it isn't that clear to me, how adding this orthogonal functionality would fit in nicely. We need to support both cases, adding shuffling and/or clustering independently. Should I replace them with nested if-else-clauses? For now I moved it to the Planner as suggested by Alex and enabled it for single-node-plans, too. Line 275: public PlanFragment addClusteringFragment(PlanFragment inputFragment, > createInsertFragment() is only called for distributed plans. Should cluster Done Line 309: sortTupleDesc.setIsMaterialized(true); > To indicate that this tuple is a physical tuple needed during runtime execu Done Line 324: // TODO Why is this needed here but not in QueryStmt.java? If it is omitted, the query > Before plan generation we call QueryStmt.materializeRequiredSlots() to make Done http://gerrit.cloudera.org:8080/#/c/4745/1/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 144: rootFragment = distributedPlanner.addClusteringFragment( > 1. we should also do this for single-node plans, so adding the clustering d 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-HasComments: Yes
