impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3586: Implement union passthrough
Date Tue, 07 Mar 2017 02:04:59 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3586: Implement union passthrough
......................................................................


Patch Set 9:

(11 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 matrix. this one's
not so bad since the fallback code is needed anyway (when the layout isn't the same), but
still wondering what the cost/benefit of this option is.


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()?


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?


Line 130:     // this)
i don't understand this comment given the dcheck on the next line, which is checking that
the row batch is empty.


PS9, Line 147:  next GetNext() call
is there guaranteed to be another GetNext() call?


Line 148:       row_batch->MarkNeedsDeepCopy();
this doesn't make sense. it only marks the last batch as needing to be copied, by why is the
last batch special?
i think we should really be using RowBatch::AcquireState() to cheaply generate a row batch
that will be unaffected by the state of the child.


Line 151:     return Status::OK();
GetNext() is quite long. how about  moving this code into GetNextPassThrough()


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

PS9, Line 99: isChildPassThrough
IsChildPassThrough


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?


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 prefer to avoid operator
overloading since it's less explicit, so how about just defining Equals() on this class.


PS9, Line 562: IsPrefixOfEquivalentLayout
this name is hard to understand because the object of "of" should be the other_desc, not "equivalent
layout". Also, the "equivalent" seems contrary to "prefix", i.e. this does not test for equality,
but it is a test of prefix.

So how about these names:

Equals()  // logical equality
LayoutEquals() // physical equality
IsPrefixOf() // logical prefix
LayoutIsPrefixOf() // physical prefix


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