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

Wrong translation for default(DateTime)/default(Guid)/default(TimeSpan) in linq projection #24075

Open
anranruye opened this issue Feb 6, 2021 · 16 comments
Assignees
Labels
area-query customer-reported needs-design poachable punted-for-6.0 punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-bug
Milestone

Comments

@anranruye
Copy link

anranruye commented Feb 6, 2021

How to reproduce:

    dbContext.Set<AnyEntityType>().Select(x => new
    { 
        DateTime = default(DateTime), 
        x.SomeProperty 
    }).ToList();

Expected result:
One of:

  1. Can not translate linq query to sql command
  2. Parameterize default(DateTime) like a variable
  3. Client evaluate default(DateTime)
  4. Convert returned string value to DateTime type

Actual result:

System.InvalidCastException: Unable to cast object of type 'System.String' to type 'System.DateTime'.
   at Microsoft.Data.SqlClient.SqlBuffer.get_DateTime()
   at Microsoft.Data.SqlClient.SqlDataReader.GetDateTime(Int32 i)
   at lambda_method5(Closure , QueryContext , DbDataReader , ResultContext , SingleQueryResultCoordinator )
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.Enumerator.MoveNext()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)

Generated sql:

      SELECT '0001-01-01T00:00:00.0000000' AS [DateTime], [x].[SomeProperty]
      FROM [TableName] AS [x]

EF Core version: 5.0.2
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 5.0
Operating system: Windows 10
IDE: Visual Studio 2019 16.8

@smitpatel
Copy link
Member

var connection = db.Database.GetDbConnection();
                connection.Open();
                using (var command = connection.CreateCommand())
                {
                    command.CommandText = "SELECT '0001-01-01T00:00:00.0000000'";
                    using (var reader = command.ExecuteReader())
                    {
                        reader.Read();
                        Console.WriteLine(reader.GetDataTypeName(0));
                        Console.WriteLine(reader.GetDateTime(0));
                    }
                }
                connection.Close();

The issue is in SqlClient which doesn't return correct type. The datatype is varchar

@smitpatel smitpatel removed this from the 6.0.0 milestone Aug 25, 2021
@roji
Copy link
Member

roji commented Aug 25, 2021

@smitpatel are you sure there's a SqlClient issue here? It seems pretty reasonable to me for this untyped literal to return a varchar (anything else would require sniffing and heuristically inferring the type based on contents). Shouldn't we be typing the literal in this case, i.e. CAST('...' AS timestamp2)?

@smitpatel
Copy link
Member

Other providers are working fine with the query. The untyped literal is how datetimes are printed in SQL for SqlClient so if a literal can be interpreted in multiple different types then there should be resiliency.
I removed from milestone if we want to add custom convert node to account for the mismatch.

@roji
Copy link
Member

roji commented Aug 25, 2021

FWIW the code above also fails on Npgsql (Can't cast database type text to DateTime) as well as on MySqlConnector (Couldn't interpret '0001-01-01T00:00:00.0000000' as a valid DateTime), anything else would mean the driver parses a string to a DateTime, with all the locale complexities etc...

So yeah, I think that if we want to make this work, we're going to need that convert node. It's also a bit odd to roundtrip an uncorrelated value like this, I wonder if there's an actual use case...

@anranruye
Copy link
Author

anranruye commented Aug 26, 2021

@roji

Consider we have an entity type like this:

public class Person
{
    public int Id { get; set; }

    public DateTime Birthday { get; set; }
   //Omit other properties 
}

Only the users who have permisson can access Birthday property and other sensitive information.

The query is like this:

    bool currentUserCanAccessBirthdayProperty = true; // or false
    var persons = db.People.Select(x => new Person
    {
        Id = x.Id,
        Birthday = currentUserCanAccessBirthdayProperty ? x.Birthday : default(DateTime)
    })

If currentUserCanAccessBirthdayProperty is false, then the query equals to

    var persons = db.People.Select(x => new Person
    {
        Id = x.Id,
        Birthday = default(DateTime)
    })

We use OData to shape the returned entities, so it's not possible to set Birthday to default(DateTime) after the query.

I know that changing the type of Birthday from DateTime to DateTime? may be better. But this is how I met this issue half year ago.

@anranruye
Copy link
Author

It's also a bit odd to roundtrip an uncorrelated value like this

I agree. It's better to handle 'default(DateTime)' at the client side. But making a convert node is also ok.

@roji
Copy link
Member

roji commented Aug 26, 2021

@anranruye thanks for the use-case scenario, that makes sense.

On an unrelated note, for your example, it could be argued that the type mapping of x.Birthday should be used to generate the literal for default(DateTime) (though that's probably not going to be easy as the whole conditional expression is elided early on). But in any case, the resulting literal in the SQL likely needs to be explicitly typed with a convert node.

@ajcvickers ajcvickers assigned bricelam and unassigned smitpatel Aug 28, 2021
@ajcvickers ajcvickers added this to the 6.0.0 milestone Aug 28, 2021
@ajcvickers
Copy link
Member

Note from triage: we may be able to do something similar to what we do for spatial types. Then again, we may not. Brice to take a look.

@anranruye
Copy link
Author

Here is a workaround:

    bool currentUserCanAccessBirthdayProperty = true; // or false
    DateTime defaultDateTime = default(DateTime);
    var persons = db.People.Select(x => new Person
    {
        Id = x.Id,
        Birthday = currentUserCanAccessBirthdayProperty ? x.Birthday : defaultDateTime 
    })

In fact, I already mentioned this in the original post:

Parameterize default(DateTime) like a variable

@roji roji removed this from the 7.0.0 milestone Jan 11, 2022
@roji roji changed the title Sql server: Wrong translation for default(DateTime)/default(Guid)/default(TimeSpan) in linq projection Wrong translation for default(DateTime)/default(Guid)/default(TimeSpan) in linq projection Jan 11, 2022
@roji
Copy link
Member

roji commented Jan 11, 2022

This has come up again for the SQL Server bool/bit mapping - the literal representation CAST(1 AS BIT) can cause an inferior query plan in some scenarios (#27150).

Stepping back...

  • This isn't a SQL Server-specific problem; it can happen anywhere where the bare SQL literal representation of a type, when evaluated, yields a different type by default - I've run into this several times in PostgreSQL.
  • Our approach for dealing with this up to now has been to add explicit CAST in the SQL literal generation (e.g. in numeric types, bool). We could continue handling the problem in a similar way for DateTime/Guid/TimeSpan/bool, but there are some drawbacks:
    • This produces needlessly verbose SQL: the CAST is necessary for literal projections, but isn't necessary in other contexts (e.g. when compared to a column).
    • #27150 shows that the extra cast can cause performance issues.
  • We could add a flag on RelationalTypeMapping, which allows provider writers to specify whether the literal representation roundtrips back to the same type (e.g. for int), or whether it doesn't (for bit).
  • In the query pipeline, we'd check that flag for literal projections and add the CAST for that case, but not for other cases. This may even be usable in other cases where a literal's type mapping cannot be inferred (e.g. a parameter to a database function which accepts any parameter type, and therefore doesn't assign a specific type mapping?).

@smitpatel
Copy link
Member

This produces needlessly verbose SQL: the CAST is necessary for literal projections, but isn't necessary in other contexts (e.g. when compared to a column).

Cast is not unnecessary for all non-projection cases. When types are not implicitly convertible without loss, explicit cast is needed. So #15586 is unavoidable.

@roji
Copy link
Member

roji commented Jan 11, 2022

@smitpatel which scenario do you have in mind (can you give an example)?

@roji
Copy link
Member

roji commented Jan 12, 2022

Note this "interesting" case on SQL Server:

-- The following works fine:
SELECT CAST('2020-01-01 12:00' AS datetime2) AT TIME ZONE 'W. Europe Standard Time';

-- The following errors: Argument data type varchar is invalid for argument 1 of AT TIME ZONE function
SELECT '2020-01-01 12:00' AT TIME ZONE 'W. Europe Standard Time';

So this is another (odd) scenario where explicit SQL typing needs to be added. At this point I'm just doing it in the method translator in #27168.

@AndriySvyryd AndriySvyryd added this to the 7.0.0 milestone Feb 15, 2022
@AndriySvyryd AndriySvyryd assigned roji and unassigned bricelam and maumar Feb 15, 2022
@ajcvickers ajcvickers added punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. and removed propose-punt labels Jul 7, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, Backlog Jul 7, 2022
@Suchiman
Copy link
Contributor

I've also just run into this. For anyone else looking for a workaround, adding (DateTime)(object) yields the desired SQL, e.g.:

    dbContext.Set<AnyEntityType>().Select(x => new
    { 
        DateTime = (DateTime)(object)DateTime.MinValue, 
        x.SomeProperty 
    }).ToList();

SQL

      SELECT CAST('0001-01-01T00:00:00.0000000' AS datetime2) AS [DateTime], [x].[SomeProperty]
      FROM [TableName] AS [x]

@Elanis
Copy link

Elanis commented Jun 1, 2023

Same things happens when you configure a DateTime class field to be mapped to a column on SQL Server with type datetime .
It uses DateTime.MinValue as a default value, which is invalid for SQL datetime type (min value is 1783-01-01).

When applying a migration that would create that column and set default values, you would have an error message about invalid casting from varchar to datetime.

@Tragetaschen
Copy link
Contributor

Tragetaschen commented Jun 26, 2023

I saw this issue too late and created another reproduction, this time with Guids being serialized as string into the query and then not being read back in correctly.

https://github.com/Tragetaschen/efcore-nullable-guid-string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query customer-reported needs-design poachable punted-for-6.0 punted-for-7.0 Originally planned for the EF Core 7.0 (EF7) release, but moved out due to resource constraints. type-bug
Projects
None yet
Development

No branches or pull requests

10 participants