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

Implement truncate Date_Time for database backend #8235

Merged
merged 8 commits into from
Nov 8, 2023

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Nov 6, 2023

Pull Request Description

Also adds some checks for column names generated for floor, ceil, truncate, round.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@jdunkerley jdunkerley linked an issue Nov 7, 2023 that may be closed by this pull request
2 tasks
@jdunkerley jdunkerley marked this pull request as ready for review November 7, 2023 14:15
True ->
self.make_unary_op op new_name
False ->
Error.throw (Unsupported_Database_Operation.Error ("`Column.truncate` for `Date_Time` is not supported by this connection."))
Copy link
Member

Choose a reason for hiding this comment

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

Technically, this is dead code currently, because Postgres has this operation and in SQLite we cannot reach here, because first we do a check precise_value_type == Value_Type.Date_Time - which will never be true in SQLite (currently) as it does not have date-time columns.

But I think it's good to have it - it will make for clearer errors once we add date-time to SQLite or add another backend and forget to implement truncate for it.

👍

Comment on lines 793 to 794
Test.group prefix+"Date truncation" <|
Test.specify "should be able to truncate a column of Date_Times" pending=pending_datetime <|
Copy link
Member

Choose a reason for hiding this comment

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

nit: since the whole group is related to date-time, the pending status can be applied to the whole group

Suggested change
Test.group prefix+"Date truncation" <|
Test.specify "should be able to truncate a column of Date_Times" pending=pending_datetime <|
Test.group prefix+"Date truncation" pending=pending_datetime <|
Test.specify "should be able to truncate a column of Date_Times" <|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 796 to 803
dates_without_zone = [Date_Time.new 2020 10 24 1 2 3, Date_Time.new 2020 10 24 1 2 3]
dates_with_zone = dates_without_zone . map (d-> d.at_zone tz)
[dates_without_zone, dates_with_zone].map dates->
table = table_builder [["foo", dates]]
truncated = table.at "foo" . truncate
truncated . to_vector . should_equal [Date.new 2020 10 24, Date.new 2020 10 24]
truncated . value_type . should_equal Value_Type.Date
truncated.name . should_equal "truncate([foo])"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this test works as you intend related to the timezone checking.

Please inspect the table.at "foo" . value_type.

I suspect that in Postgres, it will always be Date_Time with_timezone=True - that is because the way the tables are created using table_builder is we first create an in-memory table and upload it - but in-memory tables only support Date_Time with_timezone=True and not Date_Time with_timezone=False. If you want specific tests for Date_Time with_timezone=True you would need to use connection.create_table to be able to specify the types you want.

IMHO it is not necessary and testing just one approach will suffice, but ofc it is welcome to have better test coverage. But let's ensure the tests actually test what we think they do - because (unless I'm very mistaken), currently we have code paths that pretend to test the Date_Time with_timezone=False code path, but they actually don't.

Copy link
Member

Choose a reason for hiding this comment

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

Also note that in Enso code, there is no such thing as date-time without timezone.

Your dates_without_zone actually do have a timezone. It is just the system default timezone.

This test is risky, because if my system default timezone is to the east from Warsaw, e.g. Tokyo, it will actually move one day backwards and this test will be failing. Our unit tests should work regardless of the location they are run in.

> d = Date_Time.new 2020 10 24 1 2 3 zone=(Time_Zone.parse "Asia/Tokyo")
>>> Nothing
> d.date
>>> 2020-10-24
> d.at_zone (Time_Zone.parse "Europe/Warsaw")
>>> 2020-10-23T18:02:03+02:00[Europe/Warsaw]
> d.at_zone (Time_Zone.parse "Europe/Warsaw") . date
>>> 2020-10-23

Copy link
Member

Choose a reason for hiding this comment

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

Please inspect the table.at "foo" . value_type.

I think the value_type should be checked in these tests to guarantee they are checking what we think they are checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good overall, I just have some comments about the tests - I'm worried they may not check what they look like checking.

If we can get the tests fixed, all looks OK.

@GregoryTravis GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Nov 8, 2023
@mergify mergify bot merged commit 6be94a8 into develop Nov 8, 2023
33 of 34 checks passed
@mergify mergify bot deleted the wip/gmt/8047-trunc branch November 8, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement truncate Date_Time for database backend
3 participants