anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 12: (5 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@505 PS10, Line 505: f > Suppose min_range = -8888... and max_range=8888..., so the subtraction over I am fine with it. I have moved the check above. 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: : Decimal16Val > nit: why not combine the two lines? Done http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@524 PS11, Line 524: > Why do we need to declare total_leading_zeros here? Also, make this UNLIKEL Done http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@529 PS11, Line 529: e > nit: capital letter Done http://gerrit.cloudera.org:8080/#/c/6023/11/be/src/exprs/math-functions-ir.cc@531 PS11, Line 531: return result; > You should use MinLeadingZerosAfterScaling() here. Also, make this UNLIKELY Done -- 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 <apha...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: anujphadke <apha...@cloudera.com> Gerrit-Comment-Date: Tue, 27 Mar 2018 20:59:52 +0000 Gerrit-HasComments: Yes
