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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
