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 <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
