Tim Armstrong has posted comments on this change. Change subject: IMPALA-4192: Avoid updating Expr's states from ExprContext ......................................................................
Patch Set 1: (14 comments) Looking pretty good, mainly minor 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? 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 function signature and name. Doesn't really matter either way though. Line 129: /// TODO: is this still necessary? It looks like this is a bit of a nasty hack so that output floating-point values from Round() are converted to text in a way that "looks right". Should file a JIRA and reference it. 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 sense to further separate creation of the Expr and creation of the initial ExprContexts. Maybe add a TODO if this is just an intermediate step - it's a little confusing otherwise Line 372: const int fn_context_index_; I think we need some more cleanup of the handling here. Based on the expr subclass, this should always be -1 or not, right? We're checking if it's -1 in a few places, e.g. ScalarFnCall and CaseExpr, where it shouldn't be necessary if I understood correctly. It would be good to also add DCHECK_GE(fn_context_index, 0) in the constructors of the appropriate classes. PS1, Line 378: may does PS1, Line 378: can will PS1, Line 391: Parses Is it parsing the tree or just walking it? PS1, Line 393: RegisterFnContext Maybe RegisterFnContexts? Since it may register multiple. PS1, Line 431: An Exp "Each expr in the tree" just to be clearer that it's recursive. PS1, Line 432: FunctionContex FunctionContext. 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 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 only added in c++11 but I think it's helpful. http://gerrit.cloudera.org:8080/#/c/5483/1/be/src/service/fe-support.cc File be/src/service/fe-support.cc: Line 148: status = expr_ctx->GetFnContextError(); This is much cleaner. -- 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: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
