Skip to content

ref(reader): Return date and datetimes instead of strings#414

Merged
tkaemming merged 2 commits into
masterfrom
reader-date-formatting
Aug 14, 2019
Merged

ref(reader): Return date and datetimes instead of strings#414
tkaemming merged 2 commits into
masterfrom
reader-date-formatting

Conversation

@tkaemming

@tkaemming tkaemming commented Aug 14, 2019

Copy link
Copy Markdown
Contributor

This code will need to be shared between the HTTP (JSON) reader (#406) and the native reader, so this extracts it from the NativeDriverReader.

Formatting the datetime is object is already handled as part of the JSON serializer in the API:

default=lambda obj: obj.isoformat() if isinstance(obj, datetime) else obj),

It makes more sense to pass around a datetime object than a string, since this avoids unnecessary type conversions in components such as the splitter.

@tkaemming
tkaemming requested a review from a team August 14, 2019 16:43
Comment thread snuba/reader.py
DATETIME_TYPE_RE = re.compile(r"(Nullable\()?DateTime\b")


def transform_date_columns(result: Result) -> Result:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How do you feel about having some tests for this function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There should be general coverage via the API tests, but a unit test wouldn't be a bad thing either. At the same time, this is all pretty likely going to go through another round of changes in the near future so I'm hesitant to invest too much time in it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fair 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There will probably be a change soon to change the type of Result, I promise to add them at that point (and fix the bug with non-scalar values noted here.)

I'm putting this in writing so you can hold me to it. 🙂

@tkaemming

tkaemming commented Aug 14, 2019

Copy link
Copy Markdown
Contributor Author

Out of scope for this change (not a regression), but there is also a bug here:

>>> import pprint
>>> from snuba import settings
>>> from snuba.api import clickhouse_ro
>>> from snuba.reader import NativeDriverReader
>>> 
>>> native = NativeDriverReader(clickhouse_ro)
>>> pprint.pprint(native.execute('SELECT now() FORMAT JSON'))
{'data': [{'now()': datetime.datetime(2019, 8, 14, 18, 22, 54, tzinfo=tzutc())}],
 'meta': [{'name': 'now()', 'type': 'DateTime'}]}
>>> pprint.pprint(native.execute('SELECT [now()] FORMAT JSON'))
{'data': [{'array(now())': (datetime.datetime(2019, 8, 14, 18, 22, 54),)}],
 'meta': [{'name': 'array(now())', 'type': 'Array(DateTime)'}]}

@tkaemming
tkaemming merged commit a4301c6 into master Aug 14, 2019
@tkaemming
tkaemming deleted the reader-date-formatting branch August 14, 2019 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants