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.

Sign up for GitHub

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
Open

Fix convert_tz function in Postgresql #91

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
53c7ee9
Fix wrong parameter order
mesolaries Apr 5, 2022
907dbc9
Update tests
mesolaries Oct 18, 2022
3e4143a
Update PostgreSQL test
mesolaries Oct 18, 2022
139fccf
Remove xx:xx timezone tests
mesolaries Oct 18, 2022
a376dcd
Update convert_tz.yml
mesolaries Oct 18, 2022
31e7484
Update MySQL test
mesolaries Oct 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions tests/Oro/Tests/ORM/AST/Query/Functions/fixtures/postgresql/convert_tz.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 •
edited
Loading

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 •
edited
Loading

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

expectedResult:
- "2014-01-04 06:06:07"

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

Expand All @@ -22,6 +22,6 @@
- functions:
- { name: "convert_tz", className: "Oro\\ORM\\Query\\AST\\Functions\\DateTime\\ConvertTz", type: "datetime" }
dql: "SELECT CONVERT_TZ('2014-01-01 00:00:00', '+01:00', '+03:00') FROM Oro\\Entities\\Foo f WHERE f.id = 1"
sql: SELECT "timestamp"('2014-01-01 00:00:00') AT TIME ZONE '+03:00' AT TIME ZONE '+01:00' AS sclr_0 FROM test_foo t0_ WHERE t0_.id = 1
sql: SELECT "timestamp"('2014-01-01 00:00:00') AT TIME ZONE '+01:00' AT TIME ZONE '+03:00' AS sclr_0 FROM test_foo t0_ WHERE t0_.id = 1
expectedResult:
- "2014-01-01 02:00:00"