impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
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:

File be/src/exprs/

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.
File be/src/exprs/

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");

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;

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: anujphadke <>
Gerrit-HasComments: Yes

View raw message