You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org> on 2017/02/10 21:03:55 UTC
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Taras Bobrovytsky has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5969
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 107 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/1
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#6).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 114 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/6
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
> Sure, returning null seems reasonable in these cases. It would just be nice
Yes, I checked, the ambiguity is checked in the constructor of local_date_time. An exception is not thrown because we set local_date_time::NOT_DATE_TIME_ON_ERROR.
There is only 1 ambiguous case that we have to treat specially. I don't think it's a good idea to create a function IsAmbiguous(ts_value, timezone) to handle this one case. The rest of the cases would still be handled in the constructor, so it wouldn't be in 1 place.
In any case, the ambiguity is handled 1 location today: the ToUtc() function.
What do you think?
PS6, Line 134: DCHECK
> DCHECK_LT
Done
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 21:59:59"), "2014-10-26 01:59:59");
> Why the different behavior? Is there a more canonical source for this?
I think different behavior is OK because this is a completely different situation (timezone rules change vs a routine DST transition).
I'm not sure what the canonical source for this is. I don't think we ever used one when working on timestamp.
http://gerrit.cloudera.org:8080/#/c/5969/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
PS3, Line 3694: 3
> Make parallel with 2:59:59.999 below by using 3 both times or neither time
Done
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS1, Line 194: January 19, 1992
> Still open
Cleared it up a bit. Let me know if you think the comment is clear now.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
> why is it better for this to be handled here, rather than inside of FindTim
Alex and I came to this decision together. If we do this check inside FindTimezone, then the function would need to also be able to return a additional boolean flag "timestamp should be null" as well. If it simply returns null, then that means "timezone not found". Changing the interface of the function seemed too messy.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5969
to look at the new patch set (#7).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 114 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/7
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#7).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 115 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/7
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5969
to look at the new patch set (#7).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 115 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/7
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#5).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 114 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/5
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 195: // March 27, 2011 Moscow time transitioned to UTC+4 with no DST. NOTE: We currently
> we do not handle them (error/NULL?), or do we not handle them correctly?
We do not handle correctly (see patch 4)
Line 199: return TIMEZONE_MSK_PRE_2011_DST;
> mention the time also 22:00:000 UTC because we check for that below
Done
Line 209: }
> might be clearer to have const variable here int transition_hour_utc = 22 a
Done
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3685: #define UTC_TO_MSC(X) ("cast(from_utc_timestamp('" X "', 'Europe/Moscow') as string)")
Can you use https://gcc.gnu.org/onlinedocs/gcc/Push_002fPop-Macro-Pragmas.html for extra safety?
PS2, Line 3693: 3
2:59:59.999...
PS2, Line 3700: 59
Either use 59 seconds wherever possible or never.
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 22:00:00"), "2014-10-26 01:00:00");
Can you test the reverse direction for ambiguity? MSC 2014-10-26 01:30:00 should have two UTCs that correspond to it: UTC 2014-10-25 21:30:00 and UTC 2014-10-25 22:30:00
PS2, Line 3726: No
"Still no"
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS1, Line 194: January 19, 1992
> Added comment. In general, Timestamp timezone conversions are broken.
Can you be a little more specific than "we do not handle" -- is that http://catb.org/jargon/html/N/nasal-demons.html , a crash, invalid output that is still a valid time_zone_ptr, etc.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#3).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 94 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/3
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
This is okay with me, though I still think it would be nice to group the code that does ambiguity checking close together (or even factor into a separate subroutine). If you do that, I can take another look. Otherwise, okay to go with this change, but please fix the nit with DCHECK_LT.
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
> For some context, you can take a look at this discussion: https://gerrit.cl
Sure, returning null seems reasonable in these cases. It would just be nice to factor the code in a way such that all the code that determines these ambiguous cases is factored into a single location. Is it the local_date_time constructor that does the validation in other cases?
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 22:00:00"), "2014-10-26 01:00:00");
> We could emulate "JDK 8u31 or greater", according to the JIRA. We could als
I verified that Impala behaves exactly as Postgres 9.5 for all cases above, but incorrectly for the null cases:
http://rextester.com/VLVB51632
So the behavior on line 3723 is correct. It's incorrect to return NULL on line 3695.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#4).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 96 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/4
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 8: Code-Review+2
Forwarding the +2. Somehow the DHECK_LT fix was not included in the last patch.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Reviewed-on: http://gerrit.cloudera.org:8080/5969
Reviewed-by: Taras Bobrovytsky <tb...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 115 insertions(+), 45 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Taras Bobrovytsky: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
> Yes, I checked, the ambiguity is checked in the constructor of local_date_t
My suggestion was to move this code to be close to the constructor, since that's where the other validation is handled. i.e. at least move the if-stmt at line 138 up above this block so that the related code is grouped together. And then, if we were to pull it out, that subroutine would include this block along with some of the block at line 146. But you don't have to do that.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 8:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/327/
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 3:
(3 comments)
Thanks for your patience with these little nits.
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 21:59:59"), "2014-10-26 01:59:59");
> It actually is being tested on line 3723 (except it's 1am instead of 1:30am
Why the different behavior? Is there a more canonical source for this?
http://gerrit.cloudera.org:8080/#/c/5969/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
PS3, Line 3694: 3
Make parallel with 2:59:59.999 below by using 3 both times or neither time
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS1, Line 194: January 19, 1992
> Can you be a little more specific than "we do not handle" -- is that http:
Still open
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
> My suggestion was to move this code to be close to the constructor, since t
Moved the if statement as you suggested. Also added an UNLIKELY to the if statement.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3588: // IMPALA-4209: Moscow time change in 2011.
> Moscow could use a test on its own, rather than adding so many lines to Tim
Done
PS1, Line 3590: cast(from_utc_timestamp('2010-10-30 22:59:00', "
: "'Europe/Moscow') as string)
> This could be turned into a template of calling TestStringValue with "cast(
Done
Line 3596: TestIsNull("cast(to_utc_timestamp('2010-10-31 02:00:00', "
> Each one of these lines could use a short comment like "spring forward into
Done
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS1, Line 194: January 19, 1992
> What about in 1991?
Added comment. In general, Timestamp timezone conversions are broken.
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timezone_db.h
File be/src/exprs/timezone_db.h:
PS1, Line 41: "tz"
> 'tz', because the parameter tz is a std::string and so "tz" is a valid valu
Done
Line 45: /// Moscow timezone UTC+3 with DST, for use before 27 March 2011.
> Between 27 March 2011 and 26 October 2014, which timezone was Moscow in?
Moscow was in TIMEZONE_MSK_PRE_2014 timezone between 2011 and 2014, which is UTC+4
PS1, Line 48: October 26 2014
> "26 October 2014" for parallelism with above comment.
I changed both to Month, Day, Year.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3588: // IMPALA-4209: Moscow time change in 2011.
Moscow could use a test on its own, rather than adding so many lines to TimestampFunctions.
PS1, Line 3590: cast(from_utc_timestamp('2010-10-30 22:59:00', "
: "'Europe/Moscow') as string)
This could be turned into a template of calling TestStringValue with "cast(to_utc_timestamp('" and "'Europe/Moscow') as string)" appearing just once in this file to make reading what each test is supposed to do easier.
Line 3596: TestIsNull("cast(to_utc_timestamp('2010-10-31 02:00:00', "
Each one of these lines could use a short comment like "spring forward into DST, so 2am to 3am did not exist" or "TZs with ST/DST transitions were discontiguous on this region"
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS1, Line 194: January 19, 1992
What about in 1991?
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timezone_db.h
File be/src/exprs/timezone_db.h:
PS1, Line 41: "tz"
'tz', because the parameter tz is a std::string and so "tz" is a valid value of 'tz'.
Line 45: /// Moscow timezone UTC+3 with DST, for use before 27 March 2011.
Between 27 March 2011 and 26 October 2014, which timezone was Moscow in?
PS1, Line 48: October 26 2014
"26 October 2014" for parallelism with above comment.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/5/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 215: int msk_transition_day = 2456956;
> const for these three vars
AND_CAPS
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 90 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/2
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3685: #define UTC_TO_MSC(X) ("cast(from_utc_timestamp('" X "', 'Europe/Moscow') as string)")
> Can you use https://gcc.gnu.org/onlinedocs/gcc/Push_002fPop-Macro-Pragmas.h
Done
PS2, Line 3693: 3
> 2:59:59.999...
Done
PS2, Line 3700: 59
> Either use 59 seconds wherever possible or never.
Done
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 22:00:00"), "2014-10-26 01:00:00");
> Can you test the reverse direction for ambiguity? MSC 2014-10-26 01:30:00 s
It actually is being tested on line 3723 (except it's 1am instead of 1:30am, but it's testing the same thing).
This is the same behavior as timeanddate:
https://www.timeanddate.com/worldclock/converter.html?iso=20141025T205900&p1=166&p2=1440
https://www.timeanddate.com/worldclock/converter.html?iso=20141025T220000&p1=166&p2=1440
(In this case we don't resolve the ambiguity by returning null)
PS2, Line 3726: No
> "Still no"
Done
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#5).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 114 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/5
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 22:00:00"), "2014-10-26 01:00:00");
> What do you think should be done about this?
After speaking to Greg, we decided that we should return NULL for that 1 hour to be consistent.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
why is it better for this to be handled here, rather than inside of FindTimezone()?
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#2).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 90 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/2
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
> Okay, I guess what you're saying is that for the ts_val's in this range, th
For some context, you can take a look at this discussion: https://gerrit.cloudera.org/#/c/5969/2/be/src/exprs/expr-test.cc@3720
Since in other ambiguous cases (during DST changes), Impala returns null, (this is what boost does), we decided to do the same thing here.
This case is different than 2011 because in 2011 the timezone rule change happened at the same time as DST transition.
PS6, Line 205: March 27, 2011
> this doesn't seem to match the code for March 28-30th... what's the story w
The March 28th transition happened at the same time as a DST change.
PS6, Line 205: NOTE: We currently
: // do not handle Moscow time conversions for dates before January 19, 1992
: // correctly (Impala incorrectly thinks the Moscow timezone is UTC+3 with DST
: // instead of UTC+2 with DST for those dates).
> is this a pre-existing bug? is there a jira?
The preexisting bug is that we do not support timezone rule changes in general. This is a known issue. For example, Moscow time rules changed 8 times in the last 100 years (So there can be several JIRAS, one of which addresses the 1992 rule change).
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/5/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS5, Line 228: }
This function can fall off the bottom without returning anything, which I think is technically http://catb.org/jargon/html/N/nasal-demons.html
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/5/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 226: tv.time().hours() < (msk_transition_hour_utc + msk_utc_offset) % 24)) {
Is this correct? Before, we had tv.time().hours() < 1. Now we have 22 + 4 % 24 which is 2.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/5/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS5, Line 228: }
> This function can fall off the bottom without returning anything, which I t
The function does not end here. It ends on line 245.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/5/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 215: int msk_transition_day = 2456956;
const for these three vars
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 21:59:59"), "2014-10-26 01:59:59");
> I think different behavior is OK because this is a completely different sit
We could emulate "JDK 8u31 or greater", according to the JIRA. We could also try and emulate postgres. FInally, the official Standard may have something to say. I didn't see it addressed in http://standards.iso.org/ittf/PubliclyAvailableStandards/c060394_ISO_IEC_TR_19075-2_2015.zip , though.
My intuition is that it should be NULL, just like when DST happens.
http://gerrit.cloudera.org:8080/#/c/5969/1/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS1, Line 194: January 19, 1992
> Cleared it up a bit. Let me know if you think the comment is clear now.
Perfect!
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5969/6/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 133: // NULL".
> Alex and I came to this decision together. If we do this check inside FindT
Okay, I guess what you're saying is that for the ts_val's in this range, the timezone is definitely MSK_PRE_2014, but you can't convert it to UTC since it's ambiguous.
But, doesn't that happen for other timezone/timestamps as well? Oh, I guess not since the timezone is specified and usually differentiates between daylight savings and standard.
Though in the tests for the 2011 cases where there are ambiguity, what cases are those and what code path does it take?
PS6, Line 134: DCHECK
DCHECK_LT
PS6, Line 205: March 27, 2011
this doesn't seem to match the code for March 28-30th... what's the story with that?
PS6, Line 205: NOTE: We currently
: // do not handle Moscow time conversions for dates before January 19, 1992
: // correctly (Impala incorrectly thinks the Moscow timezone is UTC+3 with DST
: // instead of UTC+2 with DST for those dates).
is this a pre-existing bug? is there a jira?
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5969/5/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 215: int msk_transition_day = 2456956;
> AND_CAPS
Done
Line 215: int msk_transition_day = 2456956;
> const for these three vars
Done
Line 226: tv.time().hours() < (msk_transition_hour_utc + msk_utc_offset) % 24)) {
> Is this correct? Before, we had tv.time().hours() < 1. Now we have 22 + 4 %
Yes, this is correct. The old (pre 2014) timezone is returned also for times between 1 and 2 now (before it was returned only for times before 1. We handle this case property by returning null on line 129.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#4).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 96 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/4
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#6).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 114 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/6
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 22:00:00"), "2014-10-26 01:00:00");
> I verified that Impala behaves exactly as Postgres 9.5 for all cases above,
What do you think should be done about this?
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#8).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 115 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/8
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 8: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Hello Jim Apple, Alex Behm, Dan Hecht,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5969
to look at the new patch set (#8).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 115 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/8
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has uploaded a new patch set (#3).
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
IMPALA-4546: Fix Moscow timezone conversion after 2014
In 2014 Moscow timezone rules changed from UTC+3 with no DST to UTC+4
with no DST. A special case has been added to timestamp functions to
handle this.
Testing:
Added BE Expr tests.
Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/exprs/timezone_db.cc
M be/src/exprs/timezone_db.h
4 files changed, 94 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/69/5969/3
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 6: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/5/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
PS5, Line 228: }
> The function does not end here. It ends on line 245.
Oops, sorry about that!
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/timestamp-functions.cc
File be/src/exprs/timestamp-functions.cc:
Line 195: // March 27, 2011 Moscow time transitioned to UTC+4 with no DST. NOTE: We currently
we do not handle them (error/NULL?), or do we not handle them correctly?
Line 199: return TIMEZONE_MSK_PRE_2011_DST;
mention the time also 22:00:000 UTC because we check for that below
Line 209: }
might be clearer to have const variable here int transition_hour_utc = 22 and int msk_utc_offset = 3, and then below you can (transition_hour_utc + msk_utc_offset) % 24 to make it clear where these constant numbers come from
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4546: Fix Moscow timezone conversion after 2014
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4546: Fix Moscow timezone conversion after 2014
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5969/2/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
Line 3720: TestStringValue(UTC_TO_MSC("2014-10-25 22:00:00"), "2014-10-26 01:00:00");
> After speaking to Greg, we decided that we should return NULL for that 1 ho
Just to clarify. We want to be consistent with the existing rule of "in case of ambiguity return NULL". The reason for the ambiguity is special for Moscow (permanent timezone change vs. DST change), but its still an ambiguity.
--
To view, visit http://gerrit.cloudera.org:8080/5969
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id6e3f2c9f6ba29749a26bc1087e664637bc02528
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes