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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
