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

Add SqlServer DATEPART support for Microsecond and Nanosecond properties. #34135

Closed
wants to merge 3 commits into from

Conversation

CRFricke
Copy link
Contributor

@CRFricke CRFricke commented Jul 1, 2024

Support added for Datetime, DateTimeOffset, TimeOnly, and TimeSpan.
Fixes #13481

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

@CRFricke
Copy link
Contributor Author

CRFricke commented Jul 1, 2024

@dotnet-policy-service agree

@CRFricke
Copy link
Contributor Author

CRFricke commented Aug 5, 2024

PR awaiting review. Also, the Cosmos test runs are awaiting approval.

@maumar maumar self-assigned this Sep 25, 2024
@cincuranet cincuranet assigned cincuranet and unassigned maumar Sep 25, 2024
@cincuranet
Copy link
Contributor

This looks good @CRFricke, I just pushed few, basically, formatting tweaks. I'll merge after the CI finishes.

@cincuranet cincuranet enabled auto-merge (squash) October 8, 2024 08:43

namespace Microsoft.EntityFrameworkCore.Query;

public abstract class DatepartQueryTestBase<TFixture> : QueryTestBase<TFixture>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a full test suite only to test Datepart? We already have lots of very similar tests in both in NorthwindWhereQueryTestBase (e.g. Where_datetime_second_component), though the Northwind data indeed doesn't contain data with microsecond etc. We also have GearsOfWarQueryTestBase (e.g. Where_datetimeoffset_now), where it should be easy to add the data we need, rather than adding a whole new test suite.

I'm also noting that Datepart is the name of a SQL Server function, and in no way universal (e.g. PostgreSQL has no DATEPART function), so it doesn't seem right to use that name in core specification tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I tried adding valid non-zero Microsecond and Nanosecond values to existing table rows in both the Northwind & GearsOfWar databases. This failed miserably (the existing tests failed). After spending good week trying to make the existing tests as well as new ones work, I decided to regroup and go the full test suite route.

I took another look tonight and see that the existing Northwind & GearsOfWar Millisecond tests use a value of "0". I will do the same for the Microsecond and Nanosecond tests and remove the Datepart test suite.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you... I'm not asking for this in order to be annoying, but adding test suites for each translation/feature can very quickly get out of hand, and we're already in a place where we have lots of suites around the place, without a clear system/reason and lots of duplication etc.

Yeah, changing test data can indeed lead to existing tests failing - that can be problematic. Adding new rows with the new data should hopefully be less invasive (though there are no guarantees).

If you get stuck on this or find yourself doing a huge change, please let us know - we may be able to help with some ideas.

public override Task Select_by_timespan_nanosecond_component(bool async)
=> AssertTranslationFailed(() => base.Select_by_timespan_nanosecond_component(async));

public override Task OrderBy_timespan_nanosecond_component(bool async)
Copy link
Member

Choose a reason for hiding this comment

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

What's the goal of having tests for OrderBy and OrderByDescending? We usually test translations via simple Where(), and in some cases, we also have Select() to make sure that the type mapping is correct in the projected translation result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually write a test for every use case I can think of (they say there's no such thing as too many tests).

If you're saying that OrderBy and OrderByDescending will always end up in the same code path that Where() does when generating the T-SQL clause, then there no need for the extra tests. (I haven't been in the EFCore code anywhere near long enough to know.)

I'm happy to remove the extra tests if that's the case.

Copy link
Member

Choose a reason for hiding this comment

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

they say there's no such thing as too many tests

Unfortunately that's not quite true, tests do have a maintenance cost (and sometimes a runtime cost as well), and with a project of EF's size this can get significant.

So yes, we do make certain assumptions for testing translations: there's no need to test each translation with each and every LINQ operator (OrderBy, OrderByDescending, Where, Join...). Where() is usually the easiest operator to use (e.g. as it's easy to test that it gets the single row you want), and that's really sufficient.

For full coverage, we also have a test projecting the result of the translation out of the database (so Select()), to ensure that the resulting type and type mapping is correct (these usually don't matter for Where(), which is why just testing that isn't sufficient - but we're far from systematic on this).

@CRFricke
Copy link
Contributor Author

CRFricke commented Oct 9, 2024

Apparently, at some point after my last commit, the SQLite DateTime.Millisecond conversion began rounding up rather than truncating when the Microsecond value is GE 500.

I will be reworking the tests as noted above. This should eliminate the test failures.

@cincuranet
Copy link
Contributor

cincuranet commented Oct 9, 2024

As it turns out this implementation is not correct, because it does not take into account the differences between C# behavior (the value is 0-999) and the SQL/T-SQL.

Because I'm already here, I'll rework it and create new PR (#34861).

@cincuranet cincuranet closed this Oct 9, 2024
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.

Implement DatePart nanosecond
4 participants