Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 11: (7 comments) http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@455 PS10, Line 455: a > Done This wasn't fixed. http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions-ir.cc@505 PS10, Line 505: f > If expr < min_range, we return 0 Suppose min_range = -8888... and max_range=8888..., so the subtraction overflows. This function would return only 3 possible values then. If value=min_range-1, then we return 0. If value = max_range+1, then we return num_buckets. Otherwise, we return null. This seems like odd behavior to me. I think we should always return null if there is an overflow when computing range_size. What do you think? (btw, if we're going to do this, this needs to be moved up to the top of the function) http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@502 PS11, Line 502: Decimal16Value range_size; : range_size = nit: why not combine the two lines? http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@524 PS11, Line 524: if (int total_leading_zeros = BitUtil::CountLeadingZeros(abs(buckets.value())) + Why do we need to declare total_leading_zeros here? Also, make this UNLIKELY http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@529 PS11, Line 529: c nit: capital letter http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@531 PS11, Line 531: if (!needs_int256 && BitUtil::CountLeadingZeros(range_size.value()) + You should use MinLeadingZerosAfterScaling() here. Also, make this UNLIKELY http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h File be/src/exprs/math-functions.h: http://gerrit.cloudera.org:8080/#/c/6023/10/be/src/exprs/math-functions.h@115 PS10, Line 115: static BigIntVal WidthBucket(FunctionContext* ctx, const DecimalVal& expr, > I made this private. Not sure, any pointers where I can move this function I think the place you moved it to is ok. -- 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: 11 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: Fri, 23 Mar 2018 00:27:16 +0000 Gerrit-HasComments: Yes
