Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 13: Code-Review+1 (2 comments) I'm happy with this change. Alex, can you take a look please? 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: TestValue("width_bucket(100000000, 199999.77777777777777777777777777, 99999999999.99999" We should test the two conditions that can lead to requiring int256_t separately, which is what we are trying to do here. Are we sure that one condition is true and the other is false in these two tests? http://gerrit.cloudera.org:8080/#/c/6023/13/be/src/exprs/expr-test.cc@5333 PS13, Line 5333: Maybe add a few more cases where int256_t is used? I would add these tests: - smallest values where int256_t is needed. - largest values where int256_t is not needed - largest values where int256_t is needed and there is no overflow - smallest values where int256_t is needed and there should be an overflow -- 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: 13 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: Mon, 02 Apr 2018 23:41:06 +0000 Gerrit-HasComments: Yes
