Skip to content
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

Fix convert_tz function in Postgresql #91

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mesolaries
Copy link

$toTz parameter was passed before $fromTz. It causes convert_tz function to work not as expected. This pr fixes this bug.

@mesolaries
Copy link
Author

@x86demon please review

@paulferrett
Copy link

@x86demon This issue tripped me up for quite a while as well.

@x86demon
Copy link
Collaborator

@mesolaries thank you for your PR and fix.
Please update tests to show the bug and correctness of the fix.

Sorry for delay in the answer.

@x86demon
Copy link
Collaborator

I've checked CONVERT_TZ and it works as expected.
As a source of this implementation MySQL CONVERT_TZ was taken. https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_convert-tz
DQL function should return same results for both supported platforms (MySQL and PostgreSQL)
I've used next example
MySQL

mysql> SELECT CONVERT_TZ('2004-01-01 12:00:00','UTC','Europe/Kiev');
+-------------------------------------------------------+
| CONVERT_TZ('2004-01-01 12:00:00','UTC','Europe/Kiev') |
+-------------------------------------------------------+
| 2004-01-01 14:00:00                                   |
+-------------------------------------------------------+

Current implementation for PostgreSQL

postgres=# SELECT "timestamp"('2004-01-01 12:00:00' AT TIME ZONE 'Europe/Kiev' AT TIME ZONE 'UTC');
      timestamp      
---------------------
 2004-01-01 14:00:00

I'm closing the PR

@mesolaries, @paulferrett thank you for paying attention.

@x86demon x86demon closed this Oct 18, 2022
@mesolaries
Copy link
Author

mesolaries commented Oct 18, 2022

@x86demon Your example is not the same as current PostgreSQL implementation. Parenthesis are in wrong place.
Your example: SELECT "timestamp"('2004-01-01 12:00:00' AT TIME ZONE 'Europe/Kiev' AT TIME ZONE 'UTC');
Actual implementation: SELECT "timestamp"('2004-01-01 12:00:00') AT TIME ZONE 'Europe/Kiev' AT TIME ZONE 'UTC';

return '"timestamp"('
. $this->getExpressionValue($value, $sqlWalker)
. ')'

So, let's assume I want to convert datetime from UTC timezone to Europe/Kiev timezone. I will take current implementation as example. Expected output: 2004-01-01T14:00:00.000Z

postgres=# SELECT "timestamp"('2004-01-01 12:00:00') AT TIME ZONE 'Europe/Kiev' AT TIME ZONE 'UTC';
      timestamp      
---------------------
2004-01-01T10:00:00.000Z

As you can see the output is wrong. This is because $fromTz parameter is passed after $toTz as I mentioned before.

@x86demon x86demon reopened this Oct 18, 2022
@x86demon
Copy link
Collaborator

@mesolaries you are right, I've missed parentheses position.
Please update test so I'll be able to merge the PR

@@ -1,14 +1,14 @@
- functions:
- { name: "convert_tz", className: "Oro\\ORM\\Query\\AST\\Functions\\DateTime\\ConvertTz", type: "datetime" }
dql: "SELECT CONVERT_TZ(f.createdAt, '+00:00', '+01:00') FROM Oro\\Entities\\Foo f WHERE f.id = 1"
sql: SELECT "timestamp"(t0_.created_at) AT TIME ZONE '+01:00' AT TIME ZONE '+00:00' AS sclr_0 FROM test_foo t0_ WHERE t0_.id = 1
sql: SELECT "timestamp"(t0_.created_at) AT TIME ZONE '+00:00' AT TIME ZONE '+01:00' AS sclr_0 FROM test_foo t0_ WHERE t0_.id = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, check expectedResult. Query changed but result is still the same.

Copy link
Author

Choose a reason for hiding this comment

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

t0_.created_at is 2014-01-04 05:06:07. When converting from +00:00 to +01:00 expected result should be 2014-01-04 06:06:07

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have applied your changes and run tests. Here is an output:

1) Oro\Tests\ORM\AST\Query\Functions\FunctionsTest::testDqlFunction with data set #26 (array(array('convert_tz', 'Oro\ORM\Query\AST\Functions\D...vertTz', 'datetime')), 'SELECT CONVERT_TZ(f.createdAt...id = 1', 'SELECT "timestamp"(t0_.create...id = 1', array('2014-01-04 06:06:07'))
Unexpected result for "SELECT CONVERT_TZ(f.createdAt, '+00:00', '+01:00') FROM Oro\Entities\Foo f WHERE f.id = 1"
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '2014-01-04 06:06:07'
+    0 => '2014-01-04 04:06:07'
 )

/var/www/projects/oro-dev/doctrine-extensions/tests/Oro/Tests/ORM/AST/Query/Functions/FunctionsTest.php:48

2) Oro\Tests\ORM\AST\Query\Functions\FunctionsTest::testDqlFunction with data set #27 (array(array('convert_tz', 'Oro\ORM\Query\AST\Functions\D...vertTz', 'datetime')), 'SELECT CONVERT_TZ('2014-01-01...id = 1', 'SELECT "timestamp"('2014-01-0...id = 1', array('2014-01-01 01:00:00'))
Unexpected result for "SELECT CONVERT_TZ('2014-01-01 00:00:00', '+00:00', '+01:00') FROM Oro\Entities\Foo f WHERE f.id = 1"
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '2014-01-01 01:00:00'
+    0 => '2013-12-31 23:00:00'
 )

/var/www/projects/oro-dev/doctrine-extensions/tests/Oro/Tests/ORM/AST/Query/Functions/FunctionsTest.php:48

3) Oro\Tests\ORM\AST\Query\Functions\FunctionsTest::testDqlFunction with data set #29 (array(array('convert_tz', 'Oro\ORM\Query\AST\Functions\D...vertTz', 'datetime')), 'SELECT CONVERT_TZ('2014-01-01...id = 1', 'SELECT "timestamp"('2014-01-0...id = 1', array('2014-01-01 02:00:00'))
Unexpected result for "SELECT CONVERT_TZ('2014-01-01 00:00:00', '+01:00', '+03:00') FROM Oro\Entities\Foo f WHERE f.id = 1"
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => '2014-01-01 02:00:00'
+    0 => '2013-12-31 22:00:00'
 )

You may check MySQL version of tests. DQL and expectedResult must be the same for MySQL and PostgreSQL and tests should pass.
Check .travis.yml for details on how to execute tests for this project

Copy link
Author

@mesolaries mesolaries Oct 18, 2022

Choose a reason for hiding this comment

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

The problem is in timezone confusion. AT TIME ZONE supports timezones in POSIX style

https://github.com/eggert/tz/blob/2018g/etcetera#L38-L44

I suggest to replace '+xx:00' type timezones to locality based timezone 'UTC', 'Europe/Kiev' for example.
Thus, the tests will be passed and less confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with such change in tests if these changes will be applied for both DBs and will show that translated SQL produces same results for a given DQL.
Is +01:00 behaves differently for MySQL and PostgreSQL? If so CONVERT_TZ should have logic to prevent confusion (may be added as a separate issue)

Copy link
Author

@mesolaries mesolaries Oct 18, 2022

Choose a reason for hiding this comment

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

Yes, +01:00 behaves differently. I will add necessary tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, do not forget about DST and issues it may bring to tests in different periods of year

AT TIME ZONE supports timezones represented in POSIX style. So, timezones that passed in +xx:xx format will be confusing.
@mesolaries mesolaries requested a review from x86demon October 18, 2022 13:41
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.

3 participants