TIME/TIMESTAMP W/O TIME ZONE semantics fix (#7122) - continuation #7480
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 1, 2017
fiedukow
force-pushed
the
af/7122/2
branch
2 times, most recently
from
dacc1aa to
85fa207
Compare
findepi
approved these changes
Mar 2, 2017
fiedukow
force-pushed
the
af/7122/2
branch
3 times, most recently
from
30b5c80 to
dc756c9
Compare
|
Rebased to current master. |
fiedukow
force-pushed
the
af/7122/2
branch
2 times, most recently
from
6cae932 to
d4905bf
Compare
|
Rebased to master & fixed conflits. |
fiedukow
force-pushed
the
af/7122/2
branch
7 times, most recently
from
4f41876 to
7bbf6b4
Compare
|
Following action where taken:
Are we at the point to start review or do you need something more @haozhun ? |
fiedukow
force-pushed
the
af/7122/2
branch
3 times, most recently
from
4fa7864 to
14fed7a
Compare
|
Rebased to master. Resolved yet another set of conflicts. |
This is flag for switching between legacy TIME/TIMESTAMP semantic and new one being aligned with ANSI SQL. See: prestodb#7122
+ Improve test coverage
AT TIME ZONE does not take into account the fact, that TIME WITH TIME ZONE does not represent real millisecond UTC in millisUtc field. In fact, this field contains millisUtc assuming offset of TIME ZONE that was valid on 1970-01-01. Such representation allows to simply represent local time with time zone id. That means that TIME WITH TIME ZONE that represents eg. '10:00:00.000 Asia/Kathmandu' will always represent this exact value. However mapping of such value to other TZ (including UTC) may differ over time. Eg. Asia/Kathmandu switched time zone offset on 1986 from +5:30 to +5:45. Result of query like: `SELECT time_with_tz_column FROM table;` Will always be the same, however: `SELECT time_with_tz_column AT TIME ZONE 'UTC' FROM table;` Will yail differnet value in 1980 and 2000 after changes from this commit. This is done to use current offset of TZ as function that stucked in 1970-01-01 offsets seems useless. This is not perfect solution and is not fully aligned with standard, but standard behavior cannot be achieved with current TIME WITH TIME ZONE representation, as we are not able to read TZ offset from TIME WITH TIME ZONE itselve (at least not in all cases).
|
Thanks for the first pass of the review. |
Using this will allow to write simpler tests and avoid intermittent error caused by DST or changes in time zone policies. Without this change some test would require to rewrite to much logic from implementation making them less usefull.
This was referenced Aug 10, 2017
|
Please assign back to me when this is ready for review again. |
fiedukow
commented
Sep 8, 2017
| @@ -39,7 +39,12 @@ public Object getObjectValue(ConnectorSession session, Block block, int position | |||
| return null; | |||
| } | |||
|
|
|||
| return new SqlTimestamp(block.getLong(position, 0), session.getTimeZoneKey()); | |||
| if (session.isLegacyTimestamp()) { | |||
There was a problem hiding this comment.
Comment in header of this class is invalid and should be changed.
|
Superseded by #9385 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is continuation of #7378 and more complete solution for #7122.
Please follow this doc for detailed documentation of changes applied:
https://docs.google.com/document/d/1UUDktZDx8fGwHZV4VyaEDQURorFbbg6ioeZ5KMHwoCk/edit?usp=sharing
Commenting is enabled one this doc, so please feel free to express your concerns.
CC: @martint @haozhun @dain @losipiuk @findepi @ilfrin