Followup: Return TIMESTAMP columns as native Python datetime objects #437

amotl · 2022-07-18T13:29:26Z

apologies for the late reply.

On the patch you submitted at #395 the other day, I recently exercised #426 separately. Your other improvements from there have been converged into this very patch, which has been slightly cleaned up to reflect the changes without the gist of #426 and other spurious commits introduced by merging from the master branch.

I've tried to keep your original commits for now, but I might finally squash them together while working on the patch. The next steps are rebasing upon master and adding eventual fixup commits.

Feel free to also add additional comments or suggestions, specifically if you can spot a place where I missed to reflect the improvements from your original patch #395 correctly.

With kind regards,
Andreas.

Backlog

Also convert timestamps within ARRAYs. -- Followup: Return TIMESTAMP columns as native Python datetime objects #437 (comment)
Enable this functionality only with a feature flag.

matriv · 2022-07-19T15:01:44Z

src/crate/client/cursor.py

+    @staticmethod
+    def _transform_date_columns(row, gen_flags):
+        """
+        Generates iterates over each value from a row and converts timestamps to pandas TIMESTAMP


Can we do something more elegant with lambdas? (I have basic python knowledge though).

I think using the generator here is fine. Let me know if you think otherwise.

When looking at this again, specifically after cleaning up a bit with f355164, I tend to agree with you. It is probably enough that _transform_result_types, which iterates all records, is a generator, in order to save memory.

On the other hand, _transform_datetime_columns, which iterates all columns per record, does not necessarily have to be a generator. I will keep the original patch unchanged on this matter yet, in order to not change too much prematurely, and will await the official review.

This returns the data type of the columns in a query, to allow us to transform the dates from cratedb (long int) to python datetime

In execute(), transform the columns with type timestamp and timestamp without time zone to python datetime, this will correctly display dates in apache superset

amotl · 2022-07-26T13:20:42Z

src/crate/client/cursor.py

+                if value < 0:
+                    yield None


Not sure about this guard? Was it thought to protect against negative timestamps? In practice, they would translate to Python well, no?

>>> datetime.fromtimestamp(-2342342342415 / 1e3) datetime.datetime(1895, 10, 10, 14, 20, 57, 585000)

amotl · 2022-07-26T15:06:44Z

src/crate/client/sqlalchemy/doctests/itests.txt

-    >>> (now - location.datetime_tz).seconds < 4
-    True
-


Did this test case protect against anything significant? Currently, it croaks on my machine with a difference of 79600s (22.11 h), and I can not make much other sense of it. Most probably, I am missing the point.

Otherwise, the values of the datetime objects will depend on the time zone setting of the system, making it difficult to compare deterministically.

- Naming things - Slight structural changes - Use iterator instead of generator for column type flagging - Improve inline documentation

amotl · 2022-07-27T08:25:03Z

src/crate/client/cursor.py

+
+        Currently, only converting to native `datetime` objects is implemented.
+        """
+        datetime_column_types = [11, 15]


Does someone have a resource at hand where those column code types are enumerated? The table at ¹ coincidentally lists _timestamp without time zone as 1115, but here the code is apparently expecting two-digit integer numbers.

I will be happy to receive further pointers for better educating myself on this topic.

Footnotes

https://crate.io/docs/crate/reference/en/5.0/interfaces/postgres.html#pg-type ↩

https://crate.io/docs/crate/reference/en/5.0/interfaces/http.html#column-types

The code here will probably also need to handle arrays of timestamps.

Thanks. I will add the conversion for container types as well.

Reading up on your reference, I am asking myself whether it is appropriate that type=15 (Unchecked object) is handled here as well?

mfussenegger · 2022-07-27T10:23:27Z

src/crate/client/cursor.py

+                break
+
+            if flag and value is not None:
+                value = datetime.fromtimestamp(value / 1e3)


I think using utcfromtimestamp is the less error prone option here.

There are two scenarios:

The user stores UTC on the server:

fromtimestamp would be wrong, or at least confusing as it would convert to localtime but without attaching a timezone info.

utcfromtimestamp would at least not be wrong.

The user stores local timestamps on the server. Unless they also store the timezone in a separate field this is error prone but

utcfromtimestamp would at least preserve the value used on the server

fromtimestamp would be wrong, because it would apply another offset.

It's important that we don't attach a tzinfo given that we don't have one.

Another option could be to to let users configure the timezone on the client (with default UTC) and always create timezone aware objects.

The default could turn out to be wrong, but at least the user has an easy option to fix it, and it's easier to spot a incorrectly attached tzone then it is to figure out that a conversion happened and why/where some conversion happend.
It's also way more convenient/less error prone to continue working with tz aware datetimes.

Thanks. With 3154d32, I've switched to use datetime.utcfromtimestamp instead.

Another option could be to to let users configure the timezone on the client (with default UTC) and always create timezone aware objects.

Shall we bring in the timezone awareness (#359, #361) with a subsequent patch right after this one?

amotl · 2022-09-29T18:03:16Z

I am closing this in favor of other patches / issues, where the corresponding details have been converged into.

Add a generic data type converter to the Cursor object #442
Return TIMESTAMP types as timezone-aware native Python datetime objects #445
Enable the DefaultTypeConverter using an SQLAlchemy DBURI connection parameter sqlalchemy-cratedb#89

amotl mentioned this pull request Jul 18, 2022

Tests: Also use datetime column within locations test fixtures #438

Merged

amotl force-pushed the amo/aymaru-datetime-format branch from afee2cc to c364c4a Compare July 18, 2022 16:31

amotl mentioned this pull request Jul 18, 2022

Tests: Explicitly use both TIMESTAMP {WITH,WITHOUT} TIME ZONE types #439

Merged

amotl force-pushed the amo/aymaru-datetime-format branch from c364c4a to 2c7e75f Compare July 19, 2022 11:51

matriv reviewed Jul 19, 2022

View reviewed changes

amotl mentioned this pull request Jul 21, 2022

Adjust usage of utcnow() and utcfromtimestamp() for Python 3 #440

Closed

amotl force-pushed the amo/aymaru-datetime-format branch 2 times, most recently from 1b8f0ad to f667f14 Compare July 22, 2022 00:55

amotl mentioned this pull request Jul 22, 2022

Tests: Use UTC-based utcnow().date() instead of localtime-based date.today() #441

Merged

amotl force-pushed the amo/aymaru-datetime-format branch 2 times, most recently from 0ce46fd to db91b3d Compare July 22, 2022 23:38

Aymaru added 17 commits July 25, 2022 10:25

Changed the sql_path

312eb24

This returns the data type of the columns in a query, to allow us to transform the dates from cratedb (long int) to python datetime

Transform dates from crate to python datetime

41a881d

In execute(), transform the columns with type timestamp and timestamp without time zone to python datetime, this will correctly display dates in apache superset

Datetime conversion implemented using map and generator

49076da

Updated tests

c053da3

Using generators to work with large datasets

feebd45

fix

6566879

test

a38860b

updated datetime transformation using generators and updated test cases

e4fa0fa

cleaning debug prints

77b6550

Passing a generator of the flags instead of passing the list of values

ee77ec1

Removed tests

5b33589

updated conversion of timestamps

7e39244

Added pandas dependency

f750966

Changed pandas timestamp to python datetime && deleted pandas dependecy

e6facf3

fixed - E226 missing whitespace around arithmetic operator

e6887f4

Changed yield value

a8ffdc3

Validate date type in processors

1c46079

amotl force-pushed the amo/aymaru-datetime-format branch from db91b3d to ea4f9c7 Compare July 25, 2022 08:40

amotl commented Jul 26, 2022

View reviewed changes

amotl force-pushed the amo/aymaru-datetime-format branch from 2a9b4c2 to 6522e43 Compare July 26, 2022 15:00

amotl commented Jul 26, 2022

View reviewed changes

amotl added 5 commits July 26, 2022 17:07

Fix tests: 979e82a adjusted the timestamps in the test fixture data

8ce9209

Fix tests: Use defined time zone when validating naive datetime objects

ccc00a1

Otherwise, the values of the datetime objects will depend on the time zone setting of the system, making it difficult to compare deterministically.

Remove spurious print statement

a8532c5

Fix error when using _both_ types=true and error_trace=true options

2cc1b90

Polish datetime conversion implementation

f355164

- Naming things - Slight structural changes - Use iterator instead of generator for column type flagging - Improve inline documentation

amotl force-pushed the amo/aymaru-datetime-format branch from 6522e43 to f355164 Compare July 26, 2022 15:07

amotl commented Jul 27, 2022

View reviewed changes

mfussenegger reviewed Jul 27, 2022

View reviewed changes

Use datetime.utcfromtimestamp

3154d32

amotl mentioned this pull request Aug 5, 2022

Add a generic data type converter to the Cursor object #442

Merged

3 tasks

amotl mentioned this pull request Sep 16, 2022

Return TIMESTAMP types as timezone-aware native Python datetime objects #445

Merged

amotl mentioned this pull request Jun 16, 2024

Enable the DefaultTypeConverter using an SQLAlchemy DBURI connection parameter crate/sqlalchemy-cratedb#89

Open

amotl closed this Sep 29, 2022

amotl deleted the amo/aymaru-datetime-format branch September 29, 2022 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Followup: Return TIMESTAMP columns as native Python datetime objects #437

Followup: Return TIMESTAMP columns as native Python datetime objects #437

amotl commented Jul 18, 2022 •

edited

Loading

matriv Jul 19, 2022 •

edited by amotl

Loading

amotl Jul 25, 2022

amotl Jul 26, 2022 •

edited

Loading

amotl Jul 26, 2022 •

edited

Loading

amotl Jul 26, 2022 •

edited

Loading

amotl Jul 27, 2022

mfussenegger Jul 27, 2022 •

edited

Loading

amotl Jul 27, 2022 •

edited

Loading

mfussenegger Jul 27, 2022 •

edited

Loading

mfussenegger Jul 27, 2022

amotl Jul 27, 2022 •

edited

Loading

amotl commented Sep 29, 2022

Followup: Return TIMESTAMP columns as native Python datetime objects #437

Followup: Return TIMESTAMP columns as native Python datetime objects #437

Conversation

amotl commented Jul 18, 2022 • edited Loading

Backlog

matriv Jul 19, 2022 • edited by amotl Loading

Choose a reason for hiding this comment

amotl Jul 25, 2022

Choose a reason for hiding this comment

amotl Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

amotl Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

amotl Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

amotl Jul 27, 2022

Choose a reason for hiding this comment

Footnotes

mfussenegger Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

amotl Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

mfussenegger Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

mfussenegger Jul 27, 2022

Choose a reason for hiding this comment

amotl Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

amotl commented Sep 29, 2022

amotl commented Jul 18, 2022 •

edited

Loading

matriv Jul 19, 2022 •

edited by amotl

Loading

amotl Jul 26, 2022 •

edited

Loading

amotl Jul 26, 2022 •

edited

Loading

amotl Jul 26, 2022 •

edited

Loading

mfussenegger Jul 27, 2022 •

edited

Loading

amotl Jul 27, 2022 •

edited

Loading

mfussenegger Jul 27, 2022 •

edited

Loading

amotl Jul 27, 2022 •

edited

Loading