Alex Behm has posted comments on this change. Change subject: IMPALA-4848: Add WIDHT_BUCKET() function ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/6023/3/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: Line 437: error_msg << "Number of buckets:" << num_buckets.val << We usually follow this format. Number of buckets should be greater than zero: <given value goes here> Line 445: if (num_buckets.val < INT32_MAX) { use numeric_limits instead of INT32_MAX Line 456: if (bucket_width == numeric_limits<float>::infinity()) { I don't think infinity is possible unless our min/max range are already weird. I suggest we check the min/max range for infinity and NaN earlier and remove the check here. Also, for any such interesting cases we should have tests. Line 460: if(bucket_width == 0) return IntVal::null(); UNLIKELY Line 461: double result = (expr.val - min_range.val) / bucket_width + 1; put parenthesis around the division to make the intended evaluation order clear Line 462: if (result == numeric_limits<float>::infinity()) { Same comment as above for infinity. I don't think we can have infinity here -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke <apha...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-HasComments: Yes
