Michael Ho has posted comments on this change. Change subject: IMPALA-4192: Avoid updating Expr's states from ExprContext ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/5483/1/be/src/exprs/agg-fn-evaluator.cc File be/src/exprs/agg-fn-evaluator.cc: PS1, Line 80: CreateInputsExprTrees > CreateInputExprTrees? Done http://gerrit.cloudera.org:8080/#/c/5483/1/be/src/exprs/expr-context.h File be/src/exprs/expr-context.h: Line 51: /// The newly created ExprContext is returned. > Could probably omit the second sentence - should be obvious from the functi Done Line 129: /// TODO: is this still necessary? > It looks like this is a bit of a nasty hack so that output floating-point v Done http://gerrit.cloudera.org:8080/#/c/5483/1/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 184: static Status CreateExprTree(ObjectPool* pool, const TExpr& texpr, ExprContext** ctx); > Are you going to change this as a later step? It seems like it would make s Yes, this signature is going to be removed in upcoming patches. I avoided its removal in this patch to reduce the scope of changes. TODO added. Line 372: const int fn_context_index_; > I think we need some more cleanup of the handling here. Based on the expr s Done PS1, Line 378: can > will Done PS1, Line 378: may > does Done PS1, Line 391: Parses > Is it parsing the tree or just walking it? Done PS1, Line 393: RegisterFnContext > Maybe RegisterFnContexts? Since it may register multiple. Done PS1, Line 431: An Exp > "Each expr in the tree" just to be clearer that it's recursive. Done PS1, Line 432: FunctionContex > FunctionContext. Done http://gerrit.cloudera.org:8080/#/c/5483/1/be/src/exprs/hive-udf-call.h File be/src/exprs/hive-udf-call.h: Line 68: > Extra line Done http://gerrit.cloudera.org:8080/#/c/5483/1/be/src/exprs/scalar-fn-call.h File be/src/exprs/scalar-fn-call.h: Line 66: virtual int ComputeVarArgsBufferSize() const; > Add an override specifier. We don't use it universally yet since it was onl Done -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
