Csaba Ringhofer 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 i Done 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 tex I am still trying different linux text editors. 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 a I refactored this part a bit, now this "if" is no longer duplicated. 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? You are right, a 32 bit int is always enough for the nanoseconds part. I have changed to code accordingly. 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? Done 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 I have added a very similar test and changed the commit message to contain the same example. The new test is useful because there are more than 9 fractional digits, which exercises a different branch in DecimalOperators::ConvertToNanoseconds. -- 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 <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Comment-Date: Wed, 11 Oct 2017 14:43:40 +0000 Gerrit-HasComments: Yes
