Alex Behm has posted comments on this change. Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ......................................................................
Patch Set 4: (14 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/math-functions.h" order alphabetically Line 430: Decimal16Value width_size, range_size; move declarations closer to where they are used Line 432: //FE casts all the arguments to same time. // FE casts all the arguments to the same type. Line 433: int input_scale = ctx->impl()->GetConstFnAttr(FunctionContextImpl::ARG_TYPE_SCALE,1); formatting, space after ',' Line 434: int input_precision = ctx->impl()->GetConstFnAttr( formatting, space after ',' Line 442: if(expr < min_range) { single line and formatting (please fix formatting everywhere) Line 447: if (num_buckets.val < std::numeric_limits<int32_t>::max()) { How about we make the return value a BIGINT and simplify this code (no need to check for overflow) Line 448: return num_buckets.val + 1; Create an IntVal, that's clearer to read Line 457: range_size = max_range.template Subtract<__int128_t>(input_scale, min_range, use int128_t consistently Line 460: ctx->SetError("Overflow while bucket_width evaluation"); Please make the error more specific, so we can see where the overflow happened. Line 472: int256_t x = ConvertToInt256(width_size.value() * buckets.value()); Don't you need to convert the arguments before the multiplication? Line 475: // to avoid avoid computing reulting scale and precision typo: resulting Line 476: int256_t y = DecimalUtil::MultiplyByScale<int256_t>(ConvertToInt256(range_size.value()), The current approach seems fine, but let's avoid going to int256_t if we can. Operations with that type are very expensive. 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 -- 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: 4 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
