impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
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:

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.
File be/src/exec/

Line 104:   // Figure out which the sources from which we will need to fetch rows. This is
> Figure out which -> Determine
Not relevant any more.
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
> we typically use single quotes

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.
File fe/src/main/java/org/apache/impala/planner/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8f6d5062724ba5b78174c3227a7a796d10d8416
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message