Michael Ho has posted comments on this change.

Change subject: IMPALA-4192: Disentangle Expr and ExprContext
......................................................................


Patch Set 17:

(4 comments)

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

Line 93:     build_exprs_.push_back(build_expr);
> Wrap pool_->Add() around the above line. Makes it harder to reintroduce the
Done


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

PS16, Line 163:  
> tab?
Done


http://gerrit.cloudera.org:8080/#/c/5483/16/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

Line 87:     RETURN_IF_ERROR(ScalarExpr::Create(eq_join_conjunct.left, 
child(0)->row_desc(),
> It would be nice if we had a way to make this cleanup more automatic to mak
Done


http://gerrit.cloudera.org:8080/#/c/5483/16/be/src/exprs/anyval-util.h
File be/src/exprs/anyval-util.h:

Line 32: using impala_udf::FunctionContext;
> Using the whole namespace seems fine to me too so long as it's scoped to im
Keep it as is for now. Using the whole namespace in the header seems to bad 
practice in general.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <[email protected]>
Gerrit-Reviewer: Dan Hecht <[email protected]>
Gerrit-Reviewer: Marcel Kornacker <[email protected]>
Gerrit-Reviewer: Michael Ho <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-HasComments: Yes

Reply via email to