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 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:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/5816/9//COMMIT_MSG
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/test_queries.py in this patch.


http://gerrit.cloudera.org:8080/#/c/5816/9/be/src/exec/exchange-node.cc
File be/src/exec/exchange-node.cc:

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.


http://gerrit.cloudera.org:8080/#/c/5816/9/be/src/exec/union-node.cc
File be/src/exec/union-node.cc:

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
fast).


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()
anyways.


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
Done


http://gerrit.cloudera.org:8080/#/c/5816/9/be/src/exec/union-node.h
File be/src/exec/union-node.h:

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


PS9, Line 99: isChildPassThrough
> IsChildPassThrough
Done


http://gerrit.cloudera.org:8080/#/c/5816/9/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

Line 478:   }
> how about just calling prefix routine rather than duplicating this?
Done


http://gerrit.cloudera.org:8080/#/c/5816/9/be/src/runtime/descriptors.h
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
Done


PS9, Line 562: IsPrefixOfEquivalentLayout
> this name is hard to understand because the object of "of" should be the ot
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: 9
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