Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11160 )
Change subject: IMPALA-7412: width_bucket() function overflows too easily ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/11160/4/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/11160/4/be/src/exprs/math-functions-ir.cc@506 PS4, Line 506: abs(min_range_val) I am not sure what happens here if min_range_val is INT_MIN - according to https://en.cppreference.com/w/cpp/numeric/math/abs it is undefined in 2 complement architecture. http://gerrit.cloudera.org:8080/#/c/11160/4/be/src/exprs/math-functions-ir.cc@514 PS4, Line 514: if (BitUtil::CountLeadingZeros(dist_from_min) + : BitUtil::CountLeadingZeros(static_cast<ActualType>(num_buckets.val)) <= : BitUtil::UnsignedWidth<ActualType>() + 1) { optional performance idea: I think that it should be faster to create a bitmask from num_buckets + UnsignedWidth, and & it with dist_from_min. -- To view, visit http://gerrit.cloudera.org:8080/11160 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I728cc05d9aef8d081e6f2da66146f6d7b75dbb57 Gerrit-Change-Number: 11160 Gerrit-PatchSet: 4 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 14 Aug 2018 14:03:59 +0000 Gerrit-HasComments: Yes
