impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3350: Add some missing StringVal.is_null checks
Date Thu, 14 Apr 2016 07:46:45 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-3350: Add some missing StringVal.is_null checks
......................................................................


Patch Set 1:

(3 comments)

StringVal() can be NULL for two reasons (1) its length exceeding 1GB and (2) malloc failed
for the buffer. Even though the second case is rare, it still needs to be handled.

In the future, if we strictly enforce memory limit in FunctionContext::Allocate(), we may
return NULL if memory limit is exceeded.

http://gerrit.cloudera.org:8080/#/c/2786/1/be/src/exprs/aggregate-functions.cc
File be/src/exprs/aggregate-functions.cc:

Line 280:   if (UNLIKELY(src.is_null)) return DoubleVal::null();
> Is it necessary here? I don't think the agg intermediate value should be NU
Yes because Init() can fail to malloc() in which case we will set the StringVal.is_null =
true. Although it's rare, it's possible. In that sense, we may still call Update() or Serialize()
on the intermediate tuple which is NULL.

Note that analytic eval node can also fail to Init() for some of its evaluators but still
need to call Finalize() on all the AggFnEvaluators so we may need to handle cases in which
the intermediate value is NULL. It may call GetValue() in certain code paths too.


Line 397:   const DecimalAvgState* src_struct =
> Why not check here if we're checking in other places (in general I'm not su
Done.


Line 1257:   DCHECK(!dst->is_null);
> Why check in the update but not the merge?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/2786
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I55777487fff15a521818e39b4f93a8a242770ec2
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message