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 Wed, 08 Mar 2017 04:22:28 GMT
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-3586: Implement union passthrough

Patch Set 9:

Commit Message:

Line 15: as a precaution and for testing purposes.
> is this really needed? the more query options we have the larger the test m
We made this decision with Alex in person a while ago. There are 2 reasons:
1. In the very unlikely event that this patch introduces a bug for some customers, the query
option is a workaround.
2. We will lose existing test coverage (in most of our existing union tests, the union node
will be acting as a pass through node). This option allows us to run tests in both passthrough
and non-passthrough mode. See  tests/query_test/ in this patch.
File be/src/exec/

PS9, Line 196: IsPrefixOfEquivalentLayout
> why is this the right check rather than IsPrefixOf()?
We just want to know if the layout of the received batch is acceptable. The isPrefixOf would
fail here if the child is a union node and is passing through a batch from it's child.
File be/src/exec/

PS9, Line 101:  Ensures that rows are available for clients to fetch after this Open() has
             :   // succeeded.
> what does this comment mean now that we don't do GetNext() here?
This means the same thing. We open the first child, which means that fetching from the first
child in get next should be fast. (which means that fetching from this union node should be

Line 130:     // this)
> i don't understand this comment given the dcheck on the next line, which is
Yeah, I think it makes sense to remove it.

PS9, Line 147:  next GetNext() call
> is there guaranteed to be another GetNext() call?
Yes I think so. But even if there is no get next, all children get closed in UnionNode::Close()

Line 148:       row_batch->MarkNeedsDeepCopy();
> Yeah IMPALA-4179 covers that. IMPALA-4179 lists a few examples. One of the 
Added a todo

Line 151:     return Status::OK();
> GetNext() is quite long. how about  moving this code into GetNextPassThroug
File be/src/exec/union-node.h:

Line 67:   // Which children can be passed through, without evaluating and materializing their
> single line, 3 slashes

PS9, Line 99: isChildPassThrough
> IsChildPassThrough
File be/src/runtime/

Line 478:   }
> how about just calling prefix routine rather than duplicating this?
File be/src/runtime/descriptors.h:

Line 104:   }
> do we need these overloads? (i.e. is this now used in stl)?  if not, we pre

PS9, Line 562: IsPrefixOfEquivalentLayout
> this name is hard to understand because the object of "of" should be the ot

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8f6d5062724ba5b78174c3227a7a796d10d8416
Gerrit-PatchSet: 9
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