Michael Ho has posted comments on this change. Change subject: IMPALA-4192: Avoid updating Expr's states from ExprContext ......................................................................
Patch Set 2: (13 comments) As far I understand, FunctionContext is part of the public UDF/UDA interface so changing cannot happen until C6. Expr is the shared state which represents an expression. ExprContext is mostly a place holder for evaluation result (which allows for multiple threads to evaluate the same expression) and FunctionContext (which again appears to be thread private). I agree its names may not be too meaningful but hopefully the newly added comments will help clarify things. Let's discuss more about the design of expression on Tuesday. http://gerrit.cloudera.org:8080/#/c/5483/2//COMMIT_MSG Commit Message: Line 29: FunctionContext, ignorning the error set by its descendants. > is there a way to regression test that? There should be. Working on a test case for it. http://gerrit.cloudera.org:8080/#/c/5483/2/be/src/exprs/expr-context.h File be/src/exprs/expr-context.h: Line 44: /// concurrently. A single ExprContext is not thread-safe. > is it time to clean up the terminology as well? i (and apparently almost ev Added some class level comments in expr.h and added pointer to it here. Line 52: static ExprContext* Create(ObjectPool* pool, Expr* expr); > how is this different from the preceding c'tor and when would i call one vs This is the preferred interface for most users which will leave the ExprContext object in an object pool. We may still need the ExprContext() ctor if we want to create an ExprContext on the stack (e.g. for be-test). Line 99: /// have any error. ['start', 'end') is the range in 'fn_contexts_' for 'root_' and > don't reference internal state in the comments for public functions. Done Line 109: /// Creates a FunctionContext and stores it at 'fn_contexts_[fn_context_index]'. > public interfaces shouldn't be explained in terms of private members. is th Done Line 110: /// Called by Expr::Prepare() for exprs which need FunctionContexts (i.e. its > there still seems to be some entanglement of expr and exprctx. if exprs are Done Line 130: int output_scale() const { return output_scale_; } > why is this part of the exprctx and not the expr itself? this is static dat Yes, ideally, this should be part of the Expr. However, we want to keep Expr static after it has been initialized so by the time this field is available, it may be a bit late with the way the constant arguments to expressions are computed now (in ScalarFnCall::Open()). In the long run, we should consider moving the constant argument computation logic out to ScalarFnCall::Prepare() function so it's computed once at the Expr's initialization time. In this way, we can also benefit constant folding in codegen too. I filed IMPALA-4943 to track the effort. http://gerrit.cloudera.org:8080/#/c/5483/2/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 158: inline int fn_context_index() const { return fn_context_index_; } > this index used to be more opaque to this class -- it was determined by Fun Added a class level comment. Please let me know if more elaboration / clarification is needed. I thought about moving the allocation of fn_context_index_ to ExprContext but the problem is that we only have one ExprContext per Expr tree but there can be multiple fn_context_index_ in a tree of Expr, which makes it more natural to store it in Expr. Line 159: inline int NumFnContexts() const { > why is this needed outside this class? This is needed by ExprContext::Prepare() to size the vector of FunctionContext correctly. Line 184: /// TODO: Separate the creation of Expr from ExprContext. > does this TODO also impact the next two methods? ExprContext in those arguments will be replaced with Expr. TODO added. Line 196: static Status CreateInputExprTrees(ObjectPool* pool, const TExpr& texpr, > why is this public? This is used in AggFnEvaluator(). Line 394: void RegisterFnContexts(ExprContext* ctx, RuntimeState* state); > i wonder if it'd be clearer/simpler to just move this code into ExprContext Done PS2, Line 440: int* node_idx, bool use_all_nodes > explain these 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: 2 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
