Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8051 )
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8051/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8051/2//COMMIT_MSG@9 PS2, Line 9: The timestamp conversion from negative fractional Decimal types : (interpreted as unix timestamp) was wrong - the whole part : was rounded toward zero, and fractional part was being added : instead of being subtracted. The commit message should include a brief description of how change fixes it. http://gerrit.cloudera.org:8080/#/c/8051/2//COMMIT_MSG@14 PS2, Line 14: Example for the wrong behaivour: Spelling. Please consider setting up an automatic spell checker in your text editor. http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc File be/src/exprs/decimal-operators-ir.cc: http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc@617 PS2, Line 617: if(dv.is_negative()) nanoseconds *= -1; I think it might be easier to read if you extract the sign in in line 610 and then just multiply nanoseconds with the sign during its initialization or in the call to FromUnixTimeNanos(). http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc@625 PS2, Line 625: int64_t why is this 64bit? http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc@640 PS2, Line 640: int128_t why is this 128bit? http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/expr-test.cc@2363 PS2, Line 2363: TestTimestampValue("cast(cast(-123.45 as decimal(9,2)) as timestamp)", If you include a negative example in the commit message, it should also be included in the tests. -- To view, visit http://gerrit.cloudera.org:8080/8051 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0 Gerrit-Change-Number: 8051 Gerrit-PatchSet: 2 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Mon, 09 Oct 2017 19:58:38 +0000 Gerrit-HasComments: Yes
