anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 15: (2 comments) http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc@5331 PS13, Line 5331: ", 40)", TYPE_BIGINT, 1); > We should test the two conditions that can lead to requiring int256_t separ Yes, I tested individual scenarios by having 2 separate loops. I dont have the exact change. But something like this. Made sure we hit only one case is true. + if(BitUtil::CountLeadingZeros(abs(buckets.value())) + + BitUtil::CountLeadingZeros(abs(dist_from_min.value())) <= 128) { + + log(INFO)<< "Overflow while multiplying buckets.value * dist_from_min.value")"; + } + + if(BitUtil::CountLeadingZeros(range_size.value()) + + detail::MinLeadingZerosAfterScaling(BitUtil::CountLeadingZeros( + range_size.value()), input_scale) <= 128) { + + LOG(INFO) << "Overflow while scaling up range size"; + } + http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc@5333 PS13, Line 5333: // evaluated with int128_t. Incrementing one of them will require using int256_t for > Maybe add a few more cases where int256_t is used? I would add these tests: Done. smallest values where int256_t is needed and there should be an overflow - Since the result of the subtraction dist_from_min = (expr - min_val) is stored in decimal16Val Since, dist_from_min * num_buckets might need to be stored in int256_t to avoid overflowing int128_t. Multiplying Decimal16Val by intVal should not cause overflows when we store results in int256_t -- 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: 15 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: Wed, 09 May 2018 00:01:46 +0000 Gerrit-HasComments: Yes
