Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-3586: Implement union passthrough ......................................................................
Patch Set 16: (7 comments) http://gerrit.cloudera.org:8080/#/c/5816/9//COMMIT_MSG Commit Message: Line 15: handle all passthrough children before non-passthrough children in the > I don't think it's unusual to have "feature flags" for disabling invasive c Done. Removed the flag. Also discussed with Alex in person about testing this with patch with the query generator after this patch is checked in. http://gerrit.cloudera.org:8080/#/c/5816/16/be/src/exec/union-node.cc File be/src/exec/union-node.cc: Line 104: // Figure out which the sources from which we will need to fetch rows. This is being > Figure out which -> Determine Not relevant any more. http://gerrit.cloudera.org:8080/#/c/5816/16/be/src/exec/union-node.h File be/src/exec/union-node.h: Line 91: bool const_todo_; > Rather than having these variables, how about just saving the index of the Followed Dan's suggestion and removed state. Line 107: /// call on the child. Sets "passthrough_todo_" to false when all passthrough children > we typically use single quotes Done Line 109: Status GetNextPassThrough(RuntimeState* state, RowBatch* row_batch); > I liked your original idea of passing a 'has_more' output parameter better. No longer relevant because those members were removed. http://gerrit.cloudera.org:8080/#/c/5816/16/fe/src/main/java/org/apache/impala/planner/UnionNode.java File fe/src/main/java/org/apache/impala/planner/UnionNode.java: Line 215: isChildPassthrough_.clear(); > PlanNode.init() generally needs to be idempotent because it could be called After thinking about it some more, I don't think calling init() should have any effect on unionResultExprs_. The order of slots in unionResultExprs_ should not be affected by reordering the children. Line 221: // Order the children, such that all passthrough children come before the > move this reordering into a private helper function Done -- To view, visit http://gerrit.cloudera.org:8080/5816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia8f6d5062724ba5b78174c3227a7a796d10d8416 Gerrit-PatchSet: 16 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
