Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6023 )
Change subject: IMPALA-4848: Add WIDTH_BUCKET() function ...................................................................... Patch Set 15: (3 comments) This looks ok to me. I trust Taras has a deeper understanding of the decimal logic than I do. Is there some way we can add this to the decimal fuzz test? http://gerrit.cloudera.org:8080/#/c/6023/15/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/6023/15/be/src/exprs/expr-test.cc@582 PS15, Line 582: void TestErrorString(const string& expr, const string& error_string) { Can you document briefly what the function does? E.g. "Execute 'expr' and check that the returned error ends with 'error_string'" http://gerrit.cloudera.org:8080/#/c/6023/15/be/src/exprs/expr-test.cc@587 PS15, Line 587: status = executor_->FetchResult(&result_row); ASSERT_FALSE(status.ok()) Otherwise we'll deference the null message below and crash. http://gerrit.cloudera.org:8080/#/c/6023/15/be/src/exprs/expr-test.cc@588 PS15, Line 588: ASSERT_TRUE(status.msg().msg().compare(status.msg().msg().size() - This is basically EndsWith(), right? Can you factor out into a utility in StringUtil, just to make it more obvious what the code is doing. -- 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 <[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: Tim Armstrong <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Fri, 15 Jun 2018 00:48:48 +0000 Gerrit-HasComments: Yes
