Taras Bobrovytsky has posted comments on this change. Change subject: IMPALA-3586: Implement union passthrough ......................................................................
Patch Set 14: (13 comments) http://gerrit.cloudera.org:8080/#/c/5816/14/be/src/exec/union-node.cc File be/src/exec/union-node.cc: Line 138: while (const_expr_list_idx_ < const_expr_lists_.size() && !row_batch->AtCapacity() && !ReachedLimit()) { > long line Done Line 144: if (const_expr_list_idx_ == const_expr_lists_.size()) *eos = true; > *eos = const_expr_list_idx_ == const_expr_lists_.size(); Done Line 196: if (child_batch_.get() == NULL) { > nullptr Replaced all NULL with nullptr. Line 210: // There are only 3 ways of getting out of this loop: > We also break out if we fetch an empty child batch Fixed. This is no longer the case. Line 214: RETURN_IF_ERROR(QueryMaintenance(state)); > remove Done. So we don't need query maintenance at all? Line 274: if (const_todo_) { > Not a big deal, but I think we should do the const exprs last because most Done Line 275: RETURN_IF_ERROR(GetNextConst(state, row_batch, &done)); > why not pass &const_todo_ directly, and same for the other cases below Done, Changed the way this is handled. Line 278: RETURN_IF_ERROR(GetNextPassThrough(state, row_batch, &done)); > It might be simpler (== less error prone) overall to order the operands bas Done. Changed how this is handled now. Line 282: child_idx_ = 0; > The code here and the setting of child_idx_ in Open() is kind of subtle. Th Done, I'm doing the sorting in the FE. The explain plan should reflect the order now too. Line 286: RETURN_IF_ERROR(GetNextMaterialized(state, row_batch, &done)); > add a DCHECK here that child_eos_ is true if there were any passthrough chi Not sure if that makes sense. The DCHECK would only be true on the first call to GetNextMaterialized. What are we trying to check exactly? Line 290: *eos = ReachedLimit() || (!const_todo_ && !passthrough_todo_ && !materialize_todo_); > Isn't the last condition the same as !materialized_todo_ since we are going No, because we might have const_todo_ = true, passthrough_todo_ = true and materialize_todo_ = false at the very beginning. We set up these variables in Open(). http://gerrit.cloudera.org:8080/#/c/5816/14/be/src/exec/union-node.h File be/src/exec/union-node.h: Line 39: /// and expressions don't need to be evaluated. The UnionNode pulls row batches > Comment says union goes through children sequentially, which makes to maint Not 100% sure what you mean here, but I updated the comment. Line 107: Status GetNextConst(RuntimeState* state, RowBatch* row_batch, bool* eos); > Use a different name then eos, e.g. 'done' to avoid confusion with the real 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: 14 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
