Support timezone-aware datetime objects #178

butson · 2025-04-25T16:51:31Z

connection:

Added support for explicit TimeZone connection property

datatype:

Use Julian calendar for date/datetime before 1582 Oct 4. When the Gregorian calendar was introduced: . The day after October 4, 1582 (Julian) became October 15, 1582 (Gregorian). . That 10-day jump is not accounted for in C's calendar logic. . Julian calendar handles leap years differently. The utc calendar the engine is using understands this the python functions based on C calendar logic do not.
time.mktime , time.localtime don't support older dates while engine does changing to use calendar above and calculate time, avoid these issues.
use of datetime + timezone should handle daylight savings times and not be off by an hour.

tests:

Use connection property TimeZone to change timezone
Support timezone-aware datetime
Use pytz correctly with localize if pytz is used
Use zoneinfo.ZoneInfo if available

encodedsession:

Manage and verify TimeZone settings on per connection basis
Allows connections with different TimeZone setting from same application

requirements.txt:

Add requirement for tzlocal, should be available on all supported python versions. There is a change in api between releases , this is handled in the code.

connection: - Added support for explicit TimeZone connection property datatype: - Use Julian calendar for date/datetime before 1582 Oct 4. When the Gregorian calendar was introduced: . The day after October 4, 1582 (Julian) became October 15, 1582 (Gregorian). . That 10-day jump is not accounted for in C's calendar logic. . Julian calendar handles leap years differently. The utc calendar the engine is using understands this the python functions based on C calendar logic do not. - time.mktime , time.localtime don't support older dates while engine does changing to use calendar above and calculate time, avoid these issues. - use of datetime + timezone should handle daylight savings times and not be off by an hour. tests: - Use connection property TimeZone to change timezone - Support timezone-aware datetime - Use pytz correctly with localize if pytz is used - Use zoneinfo.ZoneInfo if available encodedsession: - Manage and verify TimeZone settings on per connection basis - Allows connections with different TimeZone setting from same application requirements.txt: - Add requirement for tzlocal, should be available on all supported python versions. There is a change in api between releases , this is handled in the code.

butson · 2025-04-28T16:11:40Z

I expect the cursor test failure is related to the callback to engine to get the real time zone used.

madscientist · 2025-05-12T21:13:59Z

Hi @butson I will try to get to the review proper this week. Just to start out, there are a number of PEP8 violations in the new code.

What I recommend for finding them, if you don't already have an IDE which is capable of enabling flake8 or similar add-ons to do this sort of syntax checking, is to get Visual Studio Code (this is not the full Visual Studio suite, it's a stand-alone, open source editor which runs on all major plaforms: Windows, GNU/Linux, and MacOS): https://code.visualstudio.com/ . Then you should configure it for Python development by installing these extensions: Python (Microsoft), Pylance (Microsoft), and Flake8.

Once you have done that (I recommend restarting after you install the extensions) you can visit the files in your Git repository in the editor, and use CTRL-SHIFT-M to open the "Problems" window and see all the issues reported there. If there are errors not in code you didn't write/update you don't need to worry about it. In particular, the "imported but not used" messages should mostly be ignored, because these tools don't grok the "type:" comments we're using with mypy. But, there are lots of whitespace issues such as, missing spaces after commas etc. in the changed code: these need to be addressed.

madscientist · 2025-05-12T21:19:26Z

Secondly, the error found by circleci in the test_result_set_gets_closed test is due to an incorrect implementation of the new method _set_timezone(). If you write it like this, instead, it will work without errors:

          stmt = self.create_statement()
          self.execute_statement(stmt, "select value from system.connectionproperties where property='TimeZone'")
          rset = self.fetch_result_set(stmt).fetchone()
          self.timezone_name = rset[0]
          self.close_statement(stmt)

Note, you can run these tests locally on your system; you need to set it up but once you do, you can verify changes before you push to GitHub and wait for the CircleCi results. If you're not sure how to do it let me know.

madscientist · 2025-05-15T19:50:26Z

Thanks @butson . Note, there are a lot of mypy and pylint errors as well. You can run make mypy and make pylint to run them. You'll have to install them first (I don't know exactly how to do that on MacOS; I only use Linux myself... let me know if you can't figure it out).

On master, mypy is 100% clean and pylint only complains about a few TODO comments.

madscientist

The thing I'm curious about is that there don't seem to be any NEW tests that test the behavior changes that were made here. It seems like only the existing tests were updated. Put another way, if we kept only the changes to the tests but did NOT add any of the other changes (except the timezone parameter of course), would all the tests pass?

The changes for the timezone parameter are good, but I don't really understand what some of the other changes are accomplishing (like the retrieval of the server zone info etc.) and I was hoping to see some tests that were making that more clear: tests that represented the missing or incorrect facilities and would fail without the changes, and then pass after the changes. Adding/retrieving dates that used to not work, but now work properly, etc.

madscientist · 2025-05-15T19:29:34Z

pynuodb/datatype.py

+                         microsecond=microsecond, tzinfo=TimeZone.utc)
+
+    def timezone_aware(tstamp, tzinfo):
+        return tstamp.replace(tzinfo=tzinfo)


This method is missing a # type: comment

madscientist · 2025-05-15T19:29:51Z

pynuodb/datatype.py

+    def timezone_aware(tstamp, tzinfo):
+        return tzinfo.localize(tstamp, is_dst=None)


Missing a # type: comment

madscientist · 2025-05-15T19:31:11Z

pynuodb/datatype.py

 from .exception import DataError
+from .calendar import ymd2day, day2ymd
+
+try:


It would be good to add a short comment to the two legs of this try/except explaining under what circumstances each one will be used.

madscientist · 2025-05-15T19:33:02Z

pynuodb/calendar.py

@@ -0,0 +1,146 @@
+


Please add copyright info here, whatever you think is correct (for example, the copyright of wherever you copied the code from, if you did, or else for DS if you wrote it yourself from scratch).

It's hard for me to believe that there's no facility that does this same thing already available but I didn't look so maybe not.

madscientist · 2025-05-15T19:40:43Z

pynuodb/datatype.py

+    hours = (seconds // 3600) % 24
+    minutes = (seconds // 60) % 60
+    seconds = seconds % 60
+    microseconds = micro % 1000000


It's a little strange to mod a variable named micro by 1000000; the only way this could have any effect is if the value passed in was >1 second. If that's the case, do we really just want to ignore that silently?

maybe this should be an assert...?

madscientist · 2025-05-16T14:10:28Z

pynuodb/encodedsession.py

+        sql = "select value from system.connectionproperties where property='TimeZone'"
+        self.execute_statement(stmt, sql)
+        rset = self.fetch_result_set(stmt).fetchone()
+        self.timezone_name = rset[0]


I wonder if we shouldn't add this into a try block and put the close_statement() in a finally clause at the end to be sure we close the connection.

I really don't like this idea of having a secret statement run behind the user's back every time they open a connection. Is it absolutely necessary? Do the other drivers like JDBC and C/C++ do this? If not why is it needed here?

madscientist · 2025-05-16T14:15:03Z

pynuodb/encodedsession.py

@@ -931,6 +986,21 @@ def getClob(self):

        raise DataError('Not a clob')

+    __shifters = [1, 10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000]


I still don't like this. We should just compute the value inline using 10 ** scale, it's much simpler to understand. There can't possibly be any performance benefit to this that is noticeable.

madscientist · 2025-05-16T14:39:08Z

tests/nuodb_basic_test.py

-            time.tzset()
-
-            con = self._connect()
+            con = self._connect(options={'TimeZone':'EST5EDT'})


madscientist · 2025-05-16T14:40:20Z

tests/nuodb_basic_test.py

        oldtz = os.environ.get('TZ')
        try:


You should remove oldtz and also the try/finally clause. You don't need to reset the timezone anymore, if you're setting the TZ in the connection.

madscientist · 2025-05-16T14:41:35Z

tests/mock_tzs.py

 #!/usr/bin/env python
 # -*- coding: utf-8 -*-


These are not needed, but there should be a copyright statement on this file.

butson requested a review from madscientist April 25, 2025 16:52

fix PEP-8 violations

5d8add0

madscientist requested changes May 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support timezone-aware datetime objects #178

Support timezone-aware datetime objects #178

butson commented Apr 25, 2025

butson commented Apr 28, 2025

madscientist commented May 12, 2025

madscientist commented May 12, 2025

madscientist commented May 15, 2025

madscientist left a comment

madscientist May 15, 2025

madscientist May 15, 2025

madscientist May 15, 2025

madscientist May 15, 2025

madscientist May 15, 2025

madscientist May 16, 2025

madscientist May 16, 2025

madscientist May 16, 2025

madscientist May 16, 2025

madscientist May 16, 2025

		def timezone_aware(tstamp, tzinfo):
		return tzinfo.localize(tstamp, is_dst=None)

		@@ -931,6 +986,21 @@ def getClob(self):

		raise DataError('Not a clob')

		__shifters = [1, 10, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000]

Support timezone-aware datetime objects #178

Are you sure you want to change the base?

Support timezone-aware datetime objects #178

Conversation

butson commented Apr 25, 2025

butson commented Apr 28, 2025

madscientist commented May 12, 2025

madscientist commented May 12, 2025

madscientist commented May 15, 2025

madscientist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment