Skip to content

Introduce use_legacy_datetime_code option for MySQL. #104

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

Conversation

hiroyuki-sato
Copy link
Member

@hiroyuki-sato hiroyuki-sato commented Apr 25, 2017

Fix #103

Summaries

This is a PR for embulk-input-mysql plugin.

What is the problem to solve.

This change returns correctly time(server side time) even if client and server timezone is different.

How the existing implementation works.

Return incorrectly time if client and server timezone is different.
A user need to set option { useLegacyDatetimeCode: false } explicitly.

Proposed change

  • Introduce new option use_legacy_datetime_code: (default: false)
  • This change does not change current behavior if server and client use same timezone.

Reproduce procedure

If timezone parameter is a difference between server and client,
latest version(0.8.18) return wrong value.

Proposed change return it properly.

Prepare test.

mysql> show variables like '%time_zone%';
+------------------+--------+
| Variable_name    | Value  |
+------------------+--------+
| system_time_zone | UTC    |
| time_zone        | SYSTEM |
+------------------+--------+
2 rows in set (0.00 sec)
mysql> select * from time_test;
+----+---------------------+---------------------+
| id | date_time           | time_stamp          |
+----+---------------------+---------------------+
|  1 | 2017-04-24 09:58:19 | 2017-04-24 09:58:19 |
+----+---------------------+---------------------+
1 row in set (0.01 sec)
create table time_test (id int not null, date_time datetime not null, time_stamp timestamp not null);
insert into time_test values(1,now(),now());
mysql> show fields from time_test;
+------------+-----------+------+-----+-------------------+-----------------------------+
| Field      | Type      | Null | Key | Default           | Extra                       |
+------------+-----------+------+-----+-------------------+-----------------------------+
| id         | int(11)   | NO   |     | NULL              |                             |
| date_time  | datetime  | NO   |     | NULL              |                             |
| time_stamp | timestamp | NO   |     | CURRENT_TIMESTAMP | on update CURRENT_TIMESTAMP |
+------------+-----------+------+-----+-------------------+-----------------------------+
3 rows in set (0.00 sec)

Client timezone is JST (+09:00)

Latest version(0.8.18) behavior

without option

in:
  type: mysql
  table: time_test
  host: localhost
#  options: { useLegacyDatetimeCode: false }
  user: xxxx
  password: xxxx
  database: embulk_test

return wrong time.

+---------+-------------------------+-------------------------+
| id:long |     date_time:timestamp |    time_stamp:timestamp |
+---------+-------------------------+-------------------------+
|       1 | 2017-04-24 00:58:19 UTC | 2017-04-24 00:58:19 UTC |
+---------+-------------------------+-------------------------+

with options

in:
  type: mysql
  table: time_test
  host: localhost
  options: { useLegacyDatetimeCode: false }
  user: xxxx
  password: xxxx
  database: embulk_test
+---------+-------------------------+-------------------------+
| id:long |     date_time:timestamp |    time_stamp:timestamp |
+---------+-------------------------+-------------------------+
|       1 | 2017-04-24 09:58:19 UTC | 2017-04-24 09:58:19 UTC |
+---------+-------------------------+-------------------------+

Proposed change

The useLegacyDatetimeCode option set to false by default.

in:
  type: mysql
  table: time_test
  host: localhost
  user: xxxx
  password: xxxx
  database: embulk_test
+---------+-------------------------+-------------------------+
| id:long |     date_time:timestamp |    time_stamp:timestamp |
+---------+-------------------------+-------------------------+
|       1 | 2017-04-24 09:58:19 UTC | 2017-04-24 09:58:19 UTC |
+---------+-------------------------+-------------------------+

Compatibility option.

in:
  type: mysql
  table: time_test
  host: localhost
  user: xxxx
  password: xxxx
  database: embulk_test
  use_legacy_datetime_code: true
+---------+-------------------------+-------------------------+
| id:long |     date_time:timestamp |    time_stamp:timestamp |
+---------+-------------------------+-------------------------+
|       1 | 2017-04-24 00:58:19 UTC | 2017-04-24 00:58:19 UTC |
+---------+-------------------------+-------------------------+

Hiroyuki Sato added 4 commits April 25, 2017 10:09
@hito4t
Copy link
Contributor

hito4t commented Nov 24, 2017

It’s about time to merge this PR.

@hito4t hito4t added this to the 0.8.7 milestone Nov 24, 2017
@hito4t hito4t merged commit d6f71bb into embulk:master Dec 25, 2017
@hito4t
Copy link
Contributor

hito4t commented Dec 25, 2017

@hiroyuki-sato
I've merged the PR.
Please check.

@hiroyuki-sato
Copy link
Member Author

Thanks! LGTM! 👍

@hiroyuki-sato hiroyuki-sato deleted the add_use_legacy_datetime_code_option branch December 25, 2017 09:54
@hito4t
Copy link
Contributor

hito4t commented Dec 27, 2017

But embulk-input-mysql still can't fetch correct date values (#132).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants