Henry Robinson has posted comments on this change. Change subject: MT: Planner for multi-threaded execution ......................................................................
Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/2846/5/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 188: mt_num_cores It seems like a bad idea to ship this option. Users will see it (it will turn up in the shell). http://gerrit.cloudera.org:8080/#/c/2846/5/fe/src/main/java/com/cloudera/impala/common/TreeNode.java File fe/src/main/java/com/cloudera/impala/common/TreeNode.java: Line 30: protected TreeNode() {} is this still necessary? http://gerrit.cloudera.org:8080/#/c/2846/5/fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java File fe/src/main/java/com/cloudera/impala/planner/ParallelPlanner.java: Line 32: sequence also 'tree' here, no? Line 35: P remove (you don't refer to P anywhere else) Line 35: In other words, the assignment : * to cohorts represents the tree structure of the pla IIUC, there's one bit of information that would make this more clear: one plan may receive many *different* build sides if it contains more than one join node. Understanding this point makes the whole thing clearer, because now I see why cohorts may a) have more than one member and b) represent some kind of parallel unit of execution. I'd rephrase the previous sentence like this: "All plans that produce intermediate join build sides (one per join node in the recipient) for a single recipient plan are grouped together into a 'cohort'. Since each plan may only produce a build side for at most one recipient plan, each plan belongs to exactly one cohort." and remove the following sentence about representing the tree structure of the plan, which I don't feel adds anything. Sorry to harp on the comments, but from the perspective of someone who doesn't have deep expertise in the FE, the comments are the first place to understand this class. -- 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: 5 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
