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 translation for DateOnly.ToDateTime(timeOnly) #35194

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mseada94
Copy link

@mseada94 mseada94 commented Nov 25, 2024

Fixes #35076

  • 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

I created the PR as draft

  • The translation is done as expected
  • TODO: 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

@mseada94 mseada94 requested a review from a team as a code owner November 25, 2024 00:02
@mseada94
Copy link
Author

The translation is done as expected but I have issue in the where comparison which compare date with datetime

Error Message on test: 

Microsoft.Data.SqlClient.SqlException : Conversion failed when converting date and/or time from character string.

SQL sent to the database:

SELECT [m].[Id], [m].[BriefingDocument], [m].[BriefingDocumentFileExtension], [m].[CodeName], [m].[Date], [m].[Difficulty], [m].[Duration], [m].[Rating], [m].[Time], [m].[Timeline]
FROM [Missions] AS [m]
WHERE DATETIMEFROMPARTS(DATEPART(year, [m].[Date]), DATEPART(month, [m].[Date]), DATEPART(day, [m].[Date]), DATEPART(hour, [m].[Time]), DATEPART(minute, [m].[Time]), DATEPART(second, [m].[Time]), DATEPART(millisecond, [m].[Time])) = '1990-11-10T10:15:50.5000000'

@mseada94
Copy link
Author

@dotnet-policy-service agree

@mseada94 mseada94 marked this pull request as draft November 25, 2024 00:06
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

I think the translation should be to DATETIME2FROMPARTS (which returns SQL Server datetime2), rather than DATETIMEFROMPARTS (which returns the discouraged datetime.

We also need to refrain from doing the translation when either the DateOnly or the TimeOnly are a complex SQL expression (anything other than a column/constant/parameter), for performance reasons; see #35076 (comment).

@roji roji self-assigned this Nov 25, 2024
    - use array literal
    - use DATETIME2FROMPARTS
    - use time 7 fractions instead of milisecond
    - add support for constant DateOnly

Fixes dotnet#35194
@mseada94 mseada94 requested a review from roji November 25, 2024 08:58
@mseada94 mseada94 marked this pull request as ready for review November 25, 2024 09: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.

SQL Server: translate dateOnly.ToDateTime(timeOnly) to DATETIMEFROMPARTS
2 participants