Alex Behm has posted comments on this change. Change subject: IMPALA-4848: Add WIDHT_BUCKET() function ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/6023/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 3456: TestIsNull("width_bucket(NULL, 2, 14, 4)", TYPE_INT); Test NULL for all args. Add tests with extreme values to trigger edge cases. Also see my comments on the code. http://gerrit.cloudera.org:8080/#/c/6023/1/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: Line 427: if (expr.is_null) return IntVal::null(); do all null checks in one if Line 429: if (min_range == max_range) { use min_range.val directly because the == operator will check for null again Line 430: ctx->SetError("lower bound cannot be equal to upper bound"); Lower... Line 434: ctx->SetError("Number of buckets should be greater than zero"); should -> must also print which value was given Line 437: if (expr.val >= max_range.val) return num_buckets.val + 1; Comment that these cases go into the special under/overflow buckets. We need to be mindful of the case where num_buckets is already MAX_INT, adding +1 here will overflow it. We should return null in that case. Also add a test. Line 439: double num_elems = (max_range.val - min_range.val) / num_buckets.val; bucket_width? This could become 0 in extreme cases, and then we'd get infinity in the next line. Casting from infinity to int results in undefined behavior, so I think we should handle this case, and add a test that triggers it. http://gerrit.cloudera.org:8080/#/c/6023/1/be/src/exprs/math-functions.h File be/src/exprs/math-functions.h: Line 128: static IntVal WidthBucket(FunctionContext* ctx, const DoubleVal& expr, Move in the public section like the other functions. -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
