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-4008: Don't bake ExprContext pointers into IR code
Date Tue, 20 Sep 2016 22:56:52 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4008: Don't bake ExprContext pointers into IR code
......................................................................


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:

Line 90:       // TODO: Fix CodegenUpdateSlot() to handle AggFnEvaluator with > 1 ExprContexts.
is this comment talking about the DCHECK? i.e. is it saying that if we had an aggregator with
more than one ExprContexts that we thought we knew how to codgen, then we'd hit this DCHECK?
I'm not really sure what this comment is trying to tell me. 

Also this code seems to be implying that agg_expr_ctxs_ can only be used by the codgen path,
but is that actually stated or enforced somewhere?


Line 387:   }
why is this change needed?  shouldn't fn_ctxs always be the same as &this->agg_fn_ctxs_?


http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/aggregation-node.h
File be/src/exec/aggregation-node.h:

PS4, Line 75: for each agg fn
this is the same correspondence as your new agg_expr_ctxs_, right? So let's word the comment
similarly so there's no question about that.


PS4, Line 146: of AggFnEvaluator
this comment sounds like there are many expression contexts per single AggFnEvaluator. clarify
it.


http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

PS4, Line 707: PointerType::get
why do we use this directly rather than codegen->GetPtrType()?  Let's be consistent one
way or the other.  (And we don't even need tuple_row_type here).


Line 1061:   PointerType* this_ptr_type = PointerType::get(this_type, 0);
same (and looks like we do this a lot in this file -- okay to cleanup as a second step).


http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS4, Line 1510: agg_expr_ctx = evaluator->input_expr_ctxs()[0];
how does this make sense? shouldn't we be codegening based on something that's not thread-specific?


http://gerrit.cloudera.org:8080/#/c/4390/4/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:

PS4, Line 527:  of the AggFnEvaluator.
same


-- 
To view, visit http://gerrit.cloudera.org:8080/4390
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I42039eed803a39fa716b9ed647510b6440974ae5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message