Henry Robinson has posted comments on this change. Change subject: MT: Planner for multi-threaded execution ......................................................................
Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/common/TreeNode.java File fe/src/main/java/com/cloudera/impala/common/TreeNode.java: Line 25: Only concrete subclasses of this can be instantiated. Why not mark the class as abstract then? http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java File fe/src/main/java/com/cloudera/impala/planner/DistributedPlanner.java: Line 625: create created http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java File fe/src/main/java/com/cloudera/impala/planner/HashJoinNode.java: Line 145: indent two spaces http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java File fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java: Line 32: sequence 'sequence' is misleading. Whether or not the fragments *can* be sequenced by their ID, the logical structure is a tree. Line 34: The plans that produce intermediate join build sides for a particular plan are This could be clearer: what's the "particular plan" in this context? Does this mean plans that produce the build sides for the same join node? Line 35: cohort Comment on why the cohort abstraction is relevant (is it a single schedulable entity?). Line 78: LOG.info Leave TODO to remove debug logging. Line 135: parentFragment parentFragment isn't defined anywhere Line 141: c It's more readable if you capitalise sentences. Line 145: com.google.common.base.Predicate import com.google.common.base.Predicate ? Line 161: JoinBuildSink JoinBuildsSink Line 161: materialization : // output 'materialization output' is unclear here: is it materialized before it gets to the sink? Line 167: new PlanFragment( : ctx_.getNextFragmentId(), join.getChild(1), : join.getFragment().getDataPartition()); This seems like it could go on two lines? http://gerrit.cloudera.org:8080/#/c/2846/4/fe/src/main/java/com/cloudera/impala/planner/PlanFragment.java File fe/src/main/java/com/cloudera/impala/planner/PlanFragment.java: Line 44: PlanFragments form a tree structure via their ExchangeNodes. A tree of fragments : * connected in that way forms a plan. The output of a plan is produced by the root : * fragment and is either the result of the query or an intermediate result : * needed by a different plan (such as a hash table). : * : * Plans are grouped into cohorts based on the consumer of their output: all : * plans that materialize intermediate results for a particular consumer plan : * are grouped into a single cohort. : * : * A PlanFragment encapsulates the specific tree of execution nodes that : * are used to produce the output of the plan fragment, as well as output exprs, : * destination node, etc. If there are no output exprs, the full row that is : * is produced by the plan root is marked as materialized. I think these relationships would be easier to understand if they were made more explicit in code. For example a cohort is only indirectly identified via a cohort ID. If, instead, there was Cohort set abstraction to which plans belonged, the relationship between plans would be clearer. Line 59: and the output sent to a specific instance of the destination : * fragment not true if the output is repartitioned (or at least, not clear that this is allowed) since you're talking about fragment instances here. Line 371: // all ExchangeNodes have registered input fragments this seems more like a well-formedness property of a plan, rather than a plan fragment. It's not critical that these things be fixed, but it does feel like there's some creaking around the edges of the current class structure. -- To view, visit http://gerrit.cloudera.org:8080/2846 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic3c34dd3f9190a131e6f03d901b4bfcd164a5174 Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
