Michael Ho has posted comments on this change. Change subject: IMPALA-3350: Add some missing StringVal.is_null checks ......................................................................
Patch Set 1: (1 comment) 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(); > Yes because Init() can fail to malloc() in which case we will set the Strin A counter argument to my point could be that we should check for Init() failure and not even proceed to call Update() or Merge(). That's possible for PAGG but I am less confident in getting everything right for AnalyticEval node and even then, we won't be able to do away with the if check in Finalize() or GetValue(). On the other hand, most of these Update() functions already check if the incoming tuple (src) is null to skip the update so it's not terribly bad to also check if dst is null. Worst case, we can use bit-wise or to avoid the extra branch. That said, codegen may get rid of the if (src.is_null) check if the tuple is not nullable for some reason but I don't think we are quite there yet :-). -- 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: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
