You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2017/09/13 15:28:39 UTC
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Csaba Ringhofer has uploaded a new change for review.
http://gerrit.cloudera.org:8080/8051
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
......................................................................
IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
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.
Example for the wrong behaivour:
+-------------------------------------------------+
| cast(cast(-0.1 as decimal(20,10)) as timestamp) |
+-------------------------------------------------+
| 1970-01-01 00:00:00.100000000 |
+-------------------------------------------------+
while casting to double works correctly:
+-----------------------------------------+
| cast(cast(-0.1 as double) as timestamp) |
+-----------------------------------------+
| 1969-12-31 23:59:59.900000000 |
+-----------------------------------------+
Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
---
M be/src/exprs/decimal-operators-ir.cc
1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8051/1
--
To view, visit http://gerrit.cloudera.org:8080/8051
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
......................................................................
Patch Set 1:
As discussed via email, let's add tests after IMPALA-5664 has been fixed.
--
To view, visit http://gerrit.cloudera.org:8080/8051
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8051
to look at the new patch set (#3).
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
......................................................................
IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
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. This is fixed by subtracting the
fractional part in case of negative decimals.
Example for the wrong behaviour:
+-------------------------------------------------+
| cast(cast(-0.1 as decimal(18,10)) as timestamp) |
+-------------------------------------------------+
| 1970-01-01 00:00:00.100000000 |
+-------------------------------------------------+
while casting to double works correctly:
+-----------------------------------------+
| cast(cast(-0.1 as double) as timestamp) |
+-----------------------------------------+
| 1969-12-31 23:59:59.900000000 |
+-----------------------------------------+
Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/decimal-operators.h
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.h
4 files changed, 43 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8051/3
--
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: newpatchset
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613: if (seconds < numeric_limits<int64_t>::min() ||
: seconds > numeric_limits<int64_t>::max()) {
: // TimeStampVal() takes int64_t.
: return TimestampVal::null();
Is this branch now also evaluated for Decimal[4/8]Values? If so, will it have a perf impact?
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h
File be/src/exprs/decimal-operators.h:
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h@166
PS3, Line 166: //
nit: Keep comment formatting consistent with the rest of the file.
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h@168
PS3, Line 168: TDecimal
This naming convention usually indicates Thrift structures throughout the codebase. Did you follow some other example here? Otherwise "const T& decimal_value" seems more consistent.
--
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: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 15:21:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 4: Code-Review+1
--
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: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:36:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613: if (seconds < numeric_limits<int64_t>::min() ||
: seconds > numeric_limits<int64_t>::max()) {
: // TimeStampVal() takes int64_t.
: return TimestampVal::null();
> Good question - my guess is that the compiler is smart enough to eliminate
Looks like it should, but it may be worth confirming, e.g. with the compiler explorer.
--
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: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:36:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613: if (seconds < numeric_limits<int64_t>::min() ||
: seconds > numeric_limits<int64_t>::max()) {
: // TimeStampVal() takes int64_t.
: return TimestampVal::null();
> I have checked with compiler explorer, and both clang 3.9.1 and gcc 4.9.2
Yes
--
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: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Oct 2017 16:42:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 5: Code-Review+2
--
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: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 18:39:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8051
to look at the new patch set (#4).
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
......................................................................
IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
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. This is fixed by subtracting the
fractional part in case of negative decimals.
Example for the wrong behaviour:
+-------------------------------------------------+
| cast(cast(-0.1 as decimal(18,10)) as timestamp) |
+-------------------------------------------------+
| 1970-01-01 00:00:00.100000000 |
+-------------------------------------------------+
while casting to double works correctly:
+-----------------------------------------+
| cast(cast(-0.1 as double) as timestamp) |
+-----------------------------------------+
| 1969-12-31 23:59:59.900000000 |
+-----------------------------------------+
Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/decimal-operators.h
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.h
4 files changed, 44 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8051/4
--
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: newpatchset
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 14:43:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 4:
(2 comments)
Thanks for fixing this. Just had a couple of minor requests then I'll +2.
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc@604
PS4, Line 604: DCHECK(nanoseconds >= numeric_limits<int32_t>::min()
We generally prefer DCHECK_GE() and DCHECK_LE() so that the out-of-range value is printed if this gets hit.
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc@622
PS4, Line 622: FromUnixTimeNanos
Can you add a note to this function's comment that it's expected to work for negative nanoseconds (I don't think it's obvious when looking at the function definition).
--
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: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Oct 2017 22:00:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1357/
--
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: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 18:39:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8051
to look at the new patch set (#5).
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
......................................................................
IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
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. This is fixed by subtracting the
fractional part in case of negative decimals.
Example for the wrong behaviour:
+-------------------------------------------------+
| cast(cast(-0.1 as decimal(18,10)) as timestamp) |
+-------------------------------------------------+
| 1970-01-01 00:00:00.100000000 |
+-------------------------------------------------+
while casting to double works correctly:
+-----------------------------------------+
| cast(cast(-0.1 as double) as timestamp) |
+-----------------------------------------+
| 1969-12-31 23:59:59.900000000 |
+-----------------------------------------+
Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/decimal-operators.h
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/timestamp-value.h
5 files changed, 45 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8051/5
--
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: newpatchset
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613: if (seconds < numeric_limits<int64_t>::min() ||
: seconds > numeric_limits<int64_t>::max()) {
: // TimeStampVal() takes int64_t.
: return TimestampVal::null();
> Looks like it should, but it may be worth confirming, e.g. with the compile
I have checked with compiler explorer, and both clang 3.9.1 and gcc 4.9.2 eliminates the branch from -O2. Can I assume that release builds and runtime code generation are never below -O2?
--
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: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Oct 2017 13:30:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8051
to look at the new patch set (#2).
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
......................................................................
IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
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.
Example for the wrong behaivour:
+-------------------------------------------------+
| cast(cast(-0.1 as decimal(20,10)) as timestamp) |
+-------------------------------------------------+
| 1970-01-01 00:00:00.100000000 |
+-------------------------------------------------+
while casting to double works correctly:
+-----------------------------------------+
| cast(cast(-0.1 as double) as timestamp) |
+-----------------------------------------+
| 1969-12-31 23:59:59.900000000 |
+-----------------------------------------+
Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/expr-test.cc
2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/8051/2
--
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: newpatchset
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 6: Code-Review+2
--
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: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 18:39:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
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 <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:58:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Verified+1
--
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: 6
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 22:38:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8051 )
Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
......................................................................
IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
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. This is fixed by subtracting the
fractional part in case of negative decimals.
Example for the wrong behaviour:
+-------------------------------------------------+
| cast(cast(-0.1 as decimal(18,10)) as timestamp) |
+-------------------------------------------------+
| 1970-01-01 00:00:00.100000000 |
+-------------------------------------------------+
while casting to double works correctly:
+-----------------------------------------+
| cast(cast(-0.1 as double) as timestamp) |
+-----------------------------------------+
| 1969-12-31 23:59:59.900000000 |
+-----------------------------------------+
Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Reviewed-on: http://gerrit.cloudera.org:8080/8051
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/decimal-operators-ir.cc
M be/src/exprs/decimal-operators.h
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/timestamp-value.h
5 files changed, 45 insertions(+), 37 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc@604
PS4, Line 604: DCHECK(nanoseconds >= numeric_limits<int32_t>::min()
> We generally prefer DCHECK_GE() and DCHECK_LE() so that the out-of-range va
DCHECK_GL/E() lead to compile error, because int128_t cannot be handled by operator <<. ( see https://stackoverflow.com/questions/25114597/how-to-print-int128-in-g ). It is possible to write such an operator, but I am not sure that it is a good idea, unless it is really necessary.
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc@622
PS4, Line 622: FromUnixTimeNanos
> Can you add a note to this function's comment that it's expected to work fo
Done
--
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: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 18:38:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613: if (seconds < numeric_limits<int64_t>::min() ||
: seconds > numeric_limits<int64_t>::max()) {
: // TimeStampVal() takes int64_t.
: return TimestampVal::null();
> Is this branch now also evaluated for Decimal[4/8]Values? If so, will it ha
Good question - my guess is that the compiler is smart enough to eliminate this branch for 4/8.
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h
File be/src/exprs/decimal-operators.h:
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h@166
PS3, Line 166: //
> nit: Keep comment formatting consistent with the rest of the file.
Done
http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h@168
PS3, Line 168: TDecimal
> Did you follow some other example h
No, I just wanted to highlight that this T is a kind of decimal.
--
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: 3
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 15:44:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/8051/4/be/src/exprs/decimal-operators-ir.cc@604
PS4, Line 604: DCHECK(nanoseconds >= numeric_limits<int32_t>::min()
> DCHECK_GL/E() lead to compile error, because int128_t cannot be handled by
Oh got it. Seems fine then.
--
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: 4
Gerrit-Owner: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Oct 2017 18:38:53 +0000
Gerrit-HasComments: Yes