Lars Volker has posted comments on this change. Change subject: IMPALA-4513: Promote integer types for ABS() ......................................................................
Patch Set 2: (3 comments) Looks good to me, but I'm no expert in this part of the codebase. http://gerrit.cloudera.org:8080/#/c/8004/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 4216: TestValue("abs(-128)", TYPE_SMALLINT, 128); What does this do with abs(-100 - 27)? How about abs(-200 + 100)? http://gerrit.cloudera.org:8080/#/c/8004/2/be/src/exprs/math-functions-ir.cc File be/src/exprs/math-functions-ir.cc: Line 65: ctx->AddWarning("Expression overflowed, returning NULL"); Do we have a test for this case? Line 73: ONE_ARG_MATH_FN(Abs, BigIntVal, IntVal, llabs); Maybe mention the upcast in a comment here? Otherwise it seems easy to miss. -- To view, visit http://gerrit.cloudera.org:8080/8004 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86cc880e78258d5f90471bd8af4caeb4305eed77 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
