impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3586: Implement union passthrough
Date Fri, 17 Mar 2017 01:38:25 GMT
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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message