impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "anujphadke (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function
Date Wed, 07 Jun 2017 00:04:15 GMT
anujphadke has posted comments on this change.

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
......................................................................


Patch Set 5:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/6023/4/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

Line 31: #include "exprs/decimal-operators.h"
> order alphabetically
Done


Line 430:     const T1& max_range, const IntVal& num_buckets) {
> move declarations closer to where they are used
Done


Line 432:   bool overflow = false;
> // FE casts all the arguments to the same type.
Done


Line 433:   // FE casts all the arguments to the same type.
> formatting, space after ','
Done


Line 434:   int input_scale = ctx->impl()->GetConstFnAttr(FunctionContextImpl::ARG_TYPE_SCALE,
1);
> formatting, space after ','
Done


Line 442: 
> single line and formatting (please fix formatting everywhere)
Done


Line 447:     result.val = num_buckets.val;
> How about we make the return value a BIGINT and simplify this code (no need
Done


Line 457:       input_scale, input_precision, input_scale, false, &overflow);
> use int128_t consistently
Done


Line 460:     error_msg << "Overflow while evaluating the difference between min_range:
" <<
> Please make the error more specific, so we can see where the overflow happe
Done


Line 472:     ctx->SetError(error_msg.str().c_str());
> Don't you need to convert the arguments before the multiplication?
Done


Line 475: 
> typo: resulting
Done


Line 476:   // resulting scale should be 2 * input_scale as per multiplication rules
> I think we'll need to be more stingy with the types. For example, the input
Discussed this with Alex.

For this  particular scenario where we might need to store results with decimal8Values inputs
into int256_t.

In this particular case- where expr and min_range are decimal8Values. This subtraction can
overflow
into a decimal16Values 

ex:
expr = max(decimal8Value) 
min_range = -1 (or any negative value)


width_size = expr.template Subtract<__int128_t>(input_scale, min_range, input_scale,
   input_precision, input_scale, false, &overflow);

width_size has to be decimal16values (to store this overflowed value) even for decimal8values.

Should we templetize  this function? Since we convert intermediate results to decimal16value
.


http://gerrit.cloudera.org:8080/#/c/6023/4/be/src/exprs/math-functions.h
File be/src/exprs/math-functions.h:

Line 111:       const IntVal& num_buckets);
> weird indentation
Done


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <aphadke@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message