Zach Amsden has posted comments on this change. Change subject: IMPALA-4513: Promote integer types for ABS() ......................................................................
Patch Set 2: (3 comments) 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)? The types may be promoted or not depending on operator precedence (I believe we have some weirdness with unary minus) but the end result is now the same: [localhost:21000] > select abs(-100 - 27); Query: select abs(-100 - 27) Query submitted at: 2017-09-07 19:30:10 (Coordinator: http://zach-pa:25000) Query progress can be monitored at: http://zach-pa:25000/query_plan?query_id=c42cf1a096e10f3:48ac218600000000 +----------------+ | abs(-100 - 27) | +----------------+ | 127 | +----------------+ Fetched 1 row(s) in 0.01s [localhost:21000] > select abs(-200 + 100); Query: select abs(-200 + 100) Query submitted at: 2017-09-07 19:30:22 (Coordinator: http://zach-pa:25000) Query progress can be monitored at: http://zach-pa:25000/query_plan?query_id=2548f7f5364aa478:70dc0e3e00000000 +-----------------+ | abs(-200 + 100) | +-----------------+ | 100 | +-----------------+ Fetched 1 row(s) in 0.01s 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? Not sure it is possible to test the warning message without adding more machinery, but there is a test for the 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 Done -- 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
