Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 12: (4 comments) This change is getting close. Overall I'm pretty happy with it. http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@502 PS12, Line 502: Nit: This empty line is in the wrong place, it should be above bool overflow = false http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@503 PS12, Line 503: Decimal16Value range_size = max_range.template Subtract<int128_t>(input_scale, min_range, Nit: long line http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@544 PS12, Line 544: if (UNLIKELY(BitUtil::CountLeadingZeros(abs(buckets.value())) + : BitUtil::CountLeadingZeros(abs(dist_from_min.value())) <= 128)) { : needs_int256 = true; : } : : // Check if scaling up range size overflows and if there is a need to store the : // intermediate results in int256_t to avoid the overflow : if (UNLIKELY(!needs_int256 && BitUtil::CountLeadingZeros(range_size.value()) + : detail::MinLeadingZerosAfterScaling(BitUtil::CountLeadingZeros( : range_size.value()), input_scale) <= 128)) { : needs_int256 = true; These two if statements should be combined into 1. It's weird to check !needs_int256 in the second one. Combine the two conditions with an OR. http://gerrit.cloudera.org:8080/#/c/6023/12/be/src/exprs/math-functions-ir.cc@582 PS12, Line 582: )+1 nit: put spaces around the + -- To view, visit http://gerrit.cloudera.org:8080/6023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f Gerrit-Change-Number: 6023 Gerrit-PatchSet: 12 Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Sat, 31 Mar 2018 00:56:55 +0000 Gerrit-HasComments: Yes
