Michael Ho has posted comments on this change. Change subject: IMPALA-4120: Incorrect results with LEAD() analytic function ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/4740/1//COMMIT_MSG Commit Message: PS1, Line 18: AggFnEvaluatorthese > ? Oops. Fixed. http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exec/analytic-eval-node.cc File be/src/exec/analytic-eval-node.cc: PS1, Line 389: AggFnEvaluator::GetValue(evaluators_, fn_ctxs_, curr_tuple_, result_tuple, : cur_tuple_pool); > wrt my comment in AggFnEvaluator, what if we didn't change GetValue() and i Done http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exprs/agg-fn-evaluator.cc File be/src/exprs/agg-fn-evaluator.cc: PS1, Line 499: v = StringVal::null(); > this means the allocation failed, right? should we be propagating errors? s Fixed in the new patch. http://gerrit.cloudera.org:8080/#/c/4740/1/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: PS1, Line 149: /// Puts the finalized value from Tuple* src in Tuple* dst just as Finalize() does. : /// 'pool' is the MemPool used for allocating memory for the finalized value (e.g. : /// StringVal). However, unlike Finalize(), GetValue() does not clean up state in src. : /// GetValue() can be called repeatedly with the same src. Only used internally for : /// analytic fn builtins. : void GetValue(FunctionContext* agg_fn_ctx, Tuple* src, Tuple* dst, MemPool* pool); > I see how this and the below changes to the GetValue() path (all the way th That makes sense. What you are suggesting is similar to what the PAGG node is doing although I suspect that it would probably have poorer cache locality. I wonder if we really should update the PAGG node instead too ? -- To view, visit http://gerrit.cloudera.org:8080/4740 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I85bb1745232d8dd383a6047c86019c6378ab571f Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-HasComments: Yes
