impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4848: Add WIDHT BUCKET() function
Date Tue, 21 Feb 2017 21:38:52 GMT
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 <aphadke@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message