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 21:55:12 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message