Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-4120: Incorrect results with LEAD() analytic function
Patch Set 1:
PS1, Line 18: AggFnEvaluatorthese
PS1, Line 389: AggFnEvaluator::GetValue(evaluators_, fn_ctxs_, curr_tuple_,
wrt my comment in AggFnEvaluator, what if we didn't change GetValue() and
instead we called a helper fn here that moved any backing memory from
result_tuple into cur_tuple_pool, e.g.
// Copies memory backing Strings (in analytic result slots) from result_tuple
into memory allocated from cur_tuple_pool.
(or something like that)
PS1, Line 499: v = StringVal::null();
this means the allocation failed, right? should we be propagating errors? seems
like that'd lead to incorrect results under low-memory conditions.
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
through SerializeOrFinalize) fix the issue, but I'm worried it's going to be
more confusing when the interface isn't symmetrical, e.g. other Serialize and
Finalize can't allocate mem out of pool even though it wouldn't be crazy for
that to be allowed.
What if the caller handled copying GetValue memory instead of having GetValue()
handle the string copy? We could have a helper fn somewhere (maybe just in
AggFnEvaluator, or maybe there's a better place) which moved backing memory
into a specified pool. Does that make sense?
To view, visit http://gerrit.cloudera.org:8080/4740
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>