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, 15 Mar 2017 00:03:03 GMT
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 <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