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

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

Merged
merged 2 commits into from
Jul 19, 2022

Conversation

amotl
Copy link
Member

@amotl amotl commented Jul 18, 2022

Hi,

this patch will be good to have before #437 in order to better outline specific changes to date vs. datetime return values.

All the values in the datetime column have been filled with the same value as in the corresponding date column. No other values have been changed.

With kind regards,
Andreas.

It is filled with the same value as in the corresponding `date` column.
@amotl amotl marked this pull request as ready for review July 18, 2022 16:38
@@ -213,7 +213,7 @@ supported, all other fields are 'None'::
>>> pprint(result)
['Aldebaran',
1373932800000,
None,
1373932800000,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can add also time info, down to millis?

Copy link
Member Author

@amotl amotl Jul 18, 2022

Choose a reason for hiding this comment

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

Sure. a7fa1ee brings in a corresponding improvement, which updates all the previous 1367366400000 timestamps.

On the other hand, I didn't change the timestamp 308534400000, it might have intentionally been set to 12 October 1979 00:00:00 UTC.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed, after that we need to add a column of DATE to test table locations, and we need also to add functionality to the client to truncate to start of day when using python date type, even if the table column (result) is a complete timestamp with time info.

Copy link
Member Author

@amotl amotl Jul 18, 2022

Choose a reason for hiding this comment

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

Thank you for the suggestion. I just looked up the definition of DATE again, and it also says within the Note admonition down the line

You cannot create table columns of type DATE. 1

So, I think TIMESTAMP is really the only type which can be actually stored. Maybe it makes sense to exercise both variants TIMESTAMP WITH TIME ZONE and/vs. TIMESTAMP WITHOUT TIME ZONE instead?

Footnotes

  1. https://crate.io/docs/crate/reference/en/5.0/general/ddl/data-types.html#date

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added #439 a followup. Let me know if that makes sense in this context, in order to further improve this spot.

All the timestamps included only time information to the hour-level.
This patch makes it use the full capabilities of milliseconds.
@amotl amotl requested a review from matriv July 18, 2022 18:25
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM, added a followup task/improvement as discussed.

@amotl amotl merged commit 979e82a into master Jul 19, 2022
@amotl amotl deleted the amo/datetime-doctests branch July 19, 2022 10:43
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.

3 participants