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

SQL Server: translate dateOnly.ToDateTime(timeOnly) to DATETIMEFROMPARTS #35076

Open
IanKemp opened this issue Nov 11, 2024 · 7 comments · May be fixed by #35194
Open

SQL Server: translate dateOnly.ToDateTime(timeOnly) to DATETIMEFROMPARTS #35076

IanKemp opened this issue Nov 11, 2024 · 7 comments · May be fixed by #35194
Labels
Milestone

Comments

@IanKemp
Copy link

IanKemp commented Nov 11, 2024

As title.

Transform to

EF.Functions.DateTimeFromParts(
    dateOnly.Year, dateOnly.Month, dateOnly.Day,
    timeOnly.Hour, timeOnly.Minute, timeOnly.Second, timeOnly.Millisecond)

which would then translate (using the current mapping of EF.Functions.DateTimeFromParts) to the SQL

DATETIMEFROMPARTS(
    DATEPART(year, dateOnly), DATEPART(month, dateOnly), DATEPART(day, dateOnly),
    DATEPART(hour, timeOnly), DATEPART(minute, timeOnly), DATEPART(second, timeOnly), DATEPART(millisecond, timeOnly))
@roji
Copy link
Member

roji commented Nov 12, 2024

Putting in the backlog as a possible translation. One problem with this kind of translation is that it duplicates its argument (in this many times over); this is OK if the argument is a simple value (column), but if it's e.g. a complex scalar subquery, that would reevaluate the subquery many times over. At least at first, I'd not perform the translation in that case.

@roji roji added this to the Backlog milestone Nov 12, 2024
@roji roji changed the title dateOnly.ToDateTime(timeOnly) should be transformed to EF.Functions.DateTimeFromParts SQL Server: translate dateOnly.ToDateTime(timeOnly) to DATETIMEFROMPARTS Nov 12, 2024
@roji roji added the good first issue This issue should be relatively straightforward to fix. label Nov 12, 2024
@mseada94
Copy link

@roji
I would like to work on this method translation.
I am new to the repo, I will try to understand the translation and then start working on the code.
I think this PR would be a good reference to understand the method translation #31180

@roji
Copy link
Member

roji commented Nov 21, 2024

@mseada94 sure thing, go ahead.

@mseada94
Copy link

mseada94 commented Nov 24, 2024

@roji
Should I perform the translation for SQL Server Only and skip SQLite considering the issue #18844 and #25103?

@mseada94
Copy link

I will start by adding two tests should these test be enough?

    [ConditionalTheory]
    [MemberData(nameof(IsAsyncData))]
    public virtual Task Where_DateOnly_ToDateTime_With_Constant_TimeOnly(bool async)
        => AssertQuery(
            async,
            ss => ss.Set<Mission>()
                .Where(o => o.Date.ToDateTime(new TimeOnly(21, 5, 19, 94)) == new DateTime(1990, 11, 10, 21, 5, 19, 94))
                .AsTracking());


    [ConditionalTheory]
    [MemberData(nameof(IsAsyncData))]
    public virtual Task Where_DateOnly_ToDateTime_With_Property_TimeOnly(bool async)
        => AssertQuery(
            async,
            ss => ss.Set<Mission>()
                .Where(o => o.Date.ToDateTime(o.Time) == new DateTime(1990, 11, 10, 10, 15, 50, 500))
                .AsTracking());

@roji
Copy link
Member

roji commented Nov 24, 2024

@mseada94 please submit a PR rather than posting implementation questions here - I'll review the PR (though it may take some time).

mseada94 added a commit to mseada94/efcore that referenced this issue Nov 25, 2024
@mseada94 mseada94 linked a pull request Nov 25, 2024 that will close this issue
8 tasks
@mseada94
Copy link

I created the PR as draft

  • The translation is done as expected
  • I have an issue in comparison between datetime and string, I need to cast const value as datetime please guide where to look
  • TODO: add support for DateOnly as constant with TimeOnly Property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants