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 DateOnly.ToString / TimeOnly.ToString query mapping for SQL Server #31289

Merged
merged 4 commits into from
Jul 24, 2023

Conversation

sulton-max
Copy link
Contributor

Summary of changes

  • Added Translator to the list of SqlServerObjectToStringTranslators

  • Added base test to GearsOfWarQueryTestBase

  • Added overloads of the base test to GearsOfWarQuerySqlServerTest, TPCGearsOfWarQuerySqlServerTest and TPTGearsOfWarQuerySqlServerTest

  • Fixes Translate DateOnly.ToString() / TimeOnly.ToString() ?? #31035

@sulton-max sulton-max changed the title Add DateOnly.ToString query mapping for SQL Server Add DateOnly.ToString / TimeOnly.ToString query mapping for SQL Server Jul 17, 2023
@sulton-max
Copy link
Contributor Author

@dotnet-policy-service agree

@roji
Copy link
Member

roji commented Jul 19, 2023

@sulton-max please take a look at the failing tests.

@sulton-max
Copy link
Contributor Author

sulton-max commented Jul 20, 2023

@sulton-max please take a look at the failing tests.

Need help on this

Issue - DateOnly standard format

Currently I'm having 2 issues

  1. SQL Lite can only work with "yyyy-mm-dd" format, which is ISO 8601
  2. Changing thread culture creates other issues - will address after first one

So

  • default value when converting DateOnly to string in c# - 7/20/2023 ( "m/d/yyyy" in my current culture )
  • SQL Lite can't work with "yyyy/mm/dd" too, only "yyyy-mm-dd" is accepted

I guess because data we are using inGearsOfWarData will be converted for DateOnly to "m/d/yyyy" when querying it right ?

How can I solve this ?

Tried multiple formats for SQL Lite in SqliteDateOnlyMethodTranslator with following logic using both strftime and date functions

        if (instance?.Type == typeof(DateOnly)
         && method.Name == nameof(DateOnly.ToString))
        {
            return _sqlExpressionFactory.Function(
              "strftime",
              new[] { _sqlExpressionFactory.Constant("%m/%d/%Y"), instance },
              nullable: true,
              argumentsPropagateNullability: new[] { false, true },
              returnType: typeof(string));
        }

@roji
Copy link
Member

roji commented Jul 20, 2023

@sulton-max please do the same thing for DateOnly/TimeOnly that we already do for DateTime - no more and no less; as @ErikEJ wrote above, there shouldn't be a DateOnly-specific translation with FORMAT, which uses a hard-coded United States date format. See the DateTime tests for ToString to understand how it should work.

@sulton-max
Copy link
Contributor Author

@sulton-max please do the same thing for DateOnly/TimeOnly that we already do for DateTime - no more and no less; as @ErikEJ wrote above, there shouldn't be a DateOnly-specific translation with FORMAT, which uses a hard-coded United States date format. See the DateTime tests for ToString to understand how it should work.

Ok, I removed method translators for DateOnly and TimeOnly, only added them in SqlServerObjectToStringTranslator just as DateTime

@roji I thought about that initially, but had a hard time finding where are DateTime ToString tests are, should I search references from GearsOfWarData entities ? Found DateTime on CogTag.IssueDate but it doesn't have any references to ToString tests

@roji
Copy link
Member

roji commented Jul 21, 2023

See Object_to_string_conversion.

Note that we cannot assert on the actual results from ToString(); since the translation is a SQL cast, every database has its own string representation when a DateTime (or DateOnly) is cast to a string. This is by design.

@sulton-max
Copy link
Contributor Author

sulton-max commented Jul 23, 2023

@roji

Oh, I see now. Should I add DateOnly / TimeOnly tests as part of Object_to_string_conversion and delete tests I wrote in GearsOfWarQueryTestBase and its overridden versions ?

So overall there must be 3 tests right ? ( in 3 overridden versions of Object_to_string_conversion for SQL Server, SQL Lite and Cosmo DB )

@roji
Copy link
Member

roji commented Jul 23, 2023

Yes

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.

@sulton-max I pushed a commit that removes the unrelated formatting changes and will merge as soon as the build is green - thanks for your contribution.

FYI it's important to review all your proposed changes before submitting a PR to make sure that nothing unintended slips in (like the above formatting stuff), a PR should ideally change exactly what it's supposed to, nothing more.

@sulton-max
Copy link
Contributor Author

@roji Understood

@roji, @ErikEJ thank you guys so much for helping throughout the process

@roji roji merged commit 8599122 into dotnet:main Jul 24, 2023
@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 24, 2023

@sulton-max You are welcome, congrats on your first contribution to EF Core

@sulton-max
Copy link
Contributor Author

@ErikEJ @roji thank you once again, I'm really happy, would you mind if I ask help when working on other issues too ?

@roji
Copy link
Member

roji commented Jul 27, 2023

@sulton-max we're always happy to receive contributions and to guide contributors where needed. But it's important for you to tackle tasks that you can overall reasonably accomplish by yourself, otherwise the guidance that we provide requires more time and effort than directly implementing the feature.

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

Successfully merging this pull request may close these issues.

Translate DateOnly.ToString() / TimeOnly.ToString() ??
4 participants