Skip to content

feat!: parsing local timestamp #1352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

v0y4g3r
Copy link
Contributor

@v0y4g3r v0y4g3r commented Apr 10, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

This PR changes the behavior of parsing timestamp/datetime literals without explict time zone info, according to system time zone (inherently from TZ env variable when GreptimeDB instance runs).

For example:

  • 1970-01-01 00:00:00+0000 will be parsed to 0 in unix timestamp, since it has explicit time offset 0
  • 1970-01-01 08:00:00+0800 will be parsed to 0 in unix timestamp, since it has explicit time offset +0800
  • 1970-01-01 08:00:00 will be parsed to 0 if GreptimeDB instance is running in CST time zone (UTC+8) and 14400 if running in Asia/Dubai time zone (UTC+4)

This PR also adds time zone identifier to DateTime formatted string.

mysql> CREATE table demo (ts TIMESTAMP(6) TIME INDEX, cnt INT);
Query OK, 0 rows affected (0.05 sec)

# Insert timestamp without time zone info, it will be convert to local timestamp.
mysql> insert into demo(ts,cnt) values ('2023-04-04 08:00:00.52', 1);
Query OK, 1 row affected (0.00 sec)

# queries also format time into local time zone.
mysql> select * from demo;
+------------------------------+------+
| ts                           | cnt  |
+------------------------------+------+
| 2023-04-04 08:00:00.520+0800 |    1 |
+------------------------------+------+

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#1319

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Merging #1352 (93eea3f) into develop (29c6155) will decrease coverage by 0.41%.
The diff coverage is 98.75%.

@@             Coverage Diff             @@
##           develop    #1352      +/-   ##
===========================================
- Coverage    85.48%   85.08%   -0.41%     
===========================================
  Files          514      514              
  Lines        77414    77523     +109     
===========================================
- Hits         66181    65964     -217     
- Misses       11233    11559     +326     

Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍

@sunng87
Copy link
Member

sunng87 commented Apr 11, 2023

For existing users this can be a breaking change: the default time zoned just changed from UTC to local. We need to address this behaviour in changelog and guide them to set TZ if needed.

@sunng87 sunng87 added the breaking-change This pull request contains breaking changes. label Apr 11, 2023
@github-actions github-actions bot removed the breaking-change This pull request contains breaking changes. label Apr 11, 2023
@v0y4g3r v0y4g3r changed the title feat: parsing local timestamp feat!: parsing local timestamp Apr 11, 2023
@github-actions github-actions bot added the breaking-change This pull request contains breaking changes. label Apr 11, 2023
@sunng87 sunng87 merged commit f5cf568 into GreptimeTeam:develop Apr 11, 2023
@evenyag
Copy link
Contributor

evenyag commented Apr 11, 2023

👍 Nice job, my timestamp master @v0y4g3r

paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* fix: parse and display timestamp/datetime in local time zone

* fix display

* fix: unit tests

* change time zone env

* fix: remove useless code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This pull request contains breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants