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

Custom ValueConverter doesn't get translated to SQL #11156

Closed
mj1856 opened this issue Mar 5, 2018 · 7 comments
Closed

Custom ValueConverter doesn't get translated to SQL #11156

mj1856 opened this issue Mar 5, 2018 · 7 comments

Comments

@mj1856
Copy link

@mj1856 mj1856 commented Mar 5, 2018

I'm trying out the new ValueConverter type in the 2.1 preview. I plan to use this to author a library to support using NodaTime types in EF Core. I'm starting this experiment by trying to store a NodaTime.Instant type as a UTC-based DateTime into a SQL Server datetime2 field.

My Model:

public class Foo
{
    public int Id { get; set; }
    public Instant SomeInstant { get; set; }
}

public class MyContext : DbContext
{
    public DbSet<Foo> Foos { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer(@"Server=.\SQLEXPRESS;Database=Foo;Trusted_Connection=True;");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder
            .Entity<Foo>()
            .Property(x => x.SomeInstant)
            .HasConversion(NodaTimeConverters.InstantToDateTimeUtc);
    }
}

public static class NodaTimeConverters
{
    public static ValueConverter<Instant, DateTime> InstantToDateTimeUtc = new ValueConverter<Instant, DateTime>(
        instant => instant.ToDateTimeUtc(),
        dateTime => Instant.FromDateTimeUtc(DateTime.SpecifyKind(dateTime, DateTimeKind.Utc)));
}

I then create the migration script, init the database, add some values, etc. All works reasonably well until I go to query.

var instant = Instant.FromUtc(2018, 1, 1, 0, 0);
var foos = db.Foos.Where(x => x.SomeInstant > instant).ToArray();

This works and gives back the correct results, but if I use SQL Profiler to examine the query, I see:

SELECT [x].[Id], [x].[SomeInstant]
FROM [Foos] AS [x]

It did not include a WHERE clause in the translation, resulting in a full table select. I assume it then filters the results in memory. This would be bad on a large table.

My understanding is that it should be able to invoke the ValueConverter, translate the expression to one that is based on DateTime, allowing the SQL provider to write the WHERE clause - but this doesn't seem to be happening.

When I try the same thing using the built-in DateTimeToStringConverter, it does include a WHERE clause. Looking at the implementation of that class, I don't see anything special as compared to any other ValueConverter, so I'm not sure what's going on.

@mj1856

This comment has been minimized.

Copy link
Author

@mj1856 mj1856 commented Mar 5, 2018

Also, I see in the docs:

  • Use of value conversions may impact the ability of EF Core to translate expressions to SQL. A warning will be logged for such cases. Removal of these limitations is being considered for a future release.

However, I do not see any such warnings.

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Mar 5, 2018

@mj1856 Yes, this is a limitation of value converters that we are tracking with #10434. We are going to add the warning before 2.1 RTM, but it hasn't been done yet. We hope to add a mechanism to allow translation in a later release.

We had discussed here the idea of using this to support NodaTime. Very interested to see how it goes and hear any feedback you have. /cc @divega @roji

@roji

This comment has been minimized.

Copy link
Member

@roji roji commented Mar 6, 2018

Note that the next version of PostgreSQL/Npgsql will support Nodatime at the ADO.NET level (in fact, this should become the recommend way to do date/time), removing the need to do this at the EF Core level with value converters.

However, when it comes to translating operations, the same problem exists - translators have to be done for operations on Nodatime types, npgsql/efcore.pg#300 tracks that for Npgsql.

@StevenRasmussen

This comment has been minimized.

Copy link

@StevenRasmussen StevenRasmussen commented Mar 26, 2018

Just ran into this as well. Was hoping to be able to use NodaTime in our domain models but sadly it looks like we won't be able to yet at this point :(

@knumat

This comment has been minimized.

Copy link

@knumat knumat commented Dec 14, 2018

For anyone else that comes across this thread, it looks like Npgsql 4.0 (released May 31, 2018) adds support for NodaTime if you are using a PostgreSQL database. Rather than use custom ValueConverters (which loads the entire table to do client-side evaluation of WHERE clauses), queries are translated to SQL statements.
https://www.npgsql.org/doc/types/nodatime.html
Hopefully the other database providers will add this support soon, or else I'll be tempted to switch to PostgreSQL. Until then, I'll probably just keep using DateTime in my model objects and write a buddy property or an extension method to hold me over until there is NodaTime support for my database.

@apavelm

This comment has been minimized.

Copy link

@apavelm apavelm commented Jun 4, 2019

@ajcvickers This issue is still with the latest version of EF Core.

I see a warning, but cannot filter resultset by NodaTime's LocalDate/LocalDateTime/Instant

The LINQ expression 'where ([s].StartTime >= __dateStart_1)' could not be translated and will be evaluated locally.

Are any plans to implement this in further releases, or at least ability how to help SqlTranslator to do that?

I tried to add SqlColumnType for an EntityTypeBuilder
config.Property(t => t.StartTime).HasColumnType("datetime").HasConversion(ConverterHelpers.LocalDateTimeConverter);

public static ValueConverter LocalDateTimeConverter => new ValueConverter<LocalDateTime, DateTime>(v => v.ToDateTimeUnspecified(), v => LocalDateTime.FromDateTime(v));
But it doesn't work. Any ETA on this?

Thank you.

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Jun 4, 2019

@apavelm This is a duplicate of another issue, hence the closed-duplicate label. The issue tracking this work is #10434

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

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.