impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Date Mon, 09 Oct 2017 19:58:38 GMT
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 <csringhofer@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:58:38 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message