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

Support different SQL translation when store representation has changed due to value conversion #10434

Open
Tracked by #240
ajcvickers opened this issue Nov 29, 2017 · 46 comments

Comments

@ajcvickers
Copy link
Member

Consider cases like #10427 which involves SQL translations for DateTime calculations. If the property has been mapped to some non-datetime type, then this kind of translation will not work anymore. Instead, query should either:

  • Do a different translation
  • Revert to client evaluation

Probably client eval is what we can realistically do in the short term, but in the future it would be good to have the type mapping/value conversion participate in generating the translations.

@ajcvickers
Copy link
Member Author

Triage: client eval for now.

@ajcvickers ajcvickers self-assigned this Dec 1, 2017
@ajcvickers ajcvickers added this to the 2.1.0 milestone Dec 1, 2017
@ajcvickers ajcvickers modified the milestones: 2.1.0-preview1, 2.1.0 Jan 17, 2018
@ajcvickers ajcvickers removed this from the 2.1.0 milestone Feb 11, 2018
@ajcvickers ajcvickers assigned anpete and unassigned ajcvickers Feb 14, 2018
@ajcvickers ajcvickers added this to the 2.1.0 milestone Feb 14, 2018
@ajcvickers
Copy link
Member Author

Triage: For 2.1, we will:

  • Detect places where query is going to use a converted type in a place "that matters"--for example, Where clause--and output a warning.
  • The warning will be for every operation other than equality/inequality
  • The warning will direct people to consider client eval

For 2.1, we will not:

  • Force client eval in these cases--for example, forcing client eval on all ordering would be bad since often only a stable ordering is needed and forcing client eval would kill perf
  • Try to add API around this area since there are many considerations and there is not enough time to bake it for 2.1. See Support client evaluation when store evaluation is not appropriate #10265

@ajcvickers
Copy link
Member Author

See also #11156

anpete added a commit that referenced this issue Mar 27, 2018
anpete added a commit that referenced this issue Mar 27, 2018
anpete added a commit that referenced this issue Mar 28, 2018
anpete added a commit that referenced this issue Mar 28, 2018
anpete added a commit that referenced this issue Mar 28, 2018
anpete added a commit that referenced this issue Mar 28, 2018
anpete added a commit that referenced this issue Mar 28, 2018
@anpete anpete removed this from the 2.1.0 milestone Mar 28, 2018
@anpete
Copy link
Contributor

anpete commented Mar 28, 2018

Warning is done. Clearing milestone to track further improvements.

@michaldudak
Copy link

What about equality/inequality in Where clause?
It seems that when there is a converted type in a Where clause (something like .Where(b => b.BlogId == new BlogId("8807e48e-1236-4ef0-a8c9-3de730e52e1b") in my case, where BlogId is a struct with a type conversion to/from a Guid) the client-side evaluation kicks in. So in this case, all Blogs would get fetched from the database, which is far from acceptable.

@OpenSpacesAndPlaces
Copy link

Ran into this running contains on a Int32[] that is value converting from a string field.
Translation of method 'System.Linq.Enumerable.Contains' failed.

@AndriySvyryd
Copy link
Member

A design proposal is to have two store-side value converters on the type mapping that wouldn't impact the store type used for migrations:

  • One for serialization/deserialization, it can affect the provider type
  • One for query when using type inference. Composition can be used to construct the appropriate converter chain depending on the context (e.g. when used as an argument of a function that expects a different store type)

@voroninp
Copy link

Will it be planned for EF 9?

@ajcvickers
Copy link
Member Author

@voroninp Too early to say yet, but I think it's probably quite unlikely that we'll fully implement this in EF Core 9.

@voroninp
Copy link

@ajcvickers , if I wanted to extend that part, where should I look at?

In most cases I need translation of comparison operators (>, >=, <, <=) and in most cases it's just the same operation applied to the converted value.

@roji
Copy link
Member

roji commented Aug 18, 2023

@voroninp you'd have to provide more context on exactly which (converted) types you want to add comparison operators to.

@voroninp
Copy link

For example we have ints which are time HHmmss, we convert them to our own type RadioTime (don't ask why not TimeOnly) which is effectively just a wrapper. And we need queries where we compare times.

@roji
Copy link
Member

roji commented Aug 18, 2023

You'll have to define functions representing operations over RadioTime (e.g. GreaterThan), and then use user-defined function mapping to map them to the SQL operators.

@voroninp
Copy link

voroninp commented Aug 18, 2023

@roji

System.InvalidOperationException Message=The parameter 'left' for the DbFunction DateIndex.op_GreaterThan(DateIndex, DateIndex)' has an invalid type 'DateIndex'. Ensure the parameter type can be mapped by the current provider.

And in ConfigureConventions I have this:

configurationBuilder.Properties<DateIndex>(b =>
{
    b.HaveConversion<DateIndexToIntConverter>();
});

@voroninp
Copy link

voroninp commented Nov 3, 2023

@roji, I need some help:

UPD: I got it. It's an expression tree, so it will be Expression.LessThan, so there won't be any call to op_LessThan. ='(
There should be additional check on EF Core side.

modelBuilder.HasDbFunction(
    typeof(SqlTranslation).GetMethod(
        nameof(SqlTranslation.LessThan), 
        BindingFlags.Public | BindingFlags.Static, 
        new[] { typeof(DateTimeOffset), typeof(DateTimeOffset) })!)
    .HasTranslation(
        args => new SqlBinaryExpression(
            ExpressionType.LessThan,
            args[0],
            args[1],
            typeof(bool),
            new BoolTypeMapping("boolean", DbType.Boolean)));

This works, but if I replace with typeof(DateTimeOffset).GetMethod("op_LessThan", BindingFlags.Public | BindingFlags.Static)! it fails. Are special methods not checked for custom mapping?

And what type of SQL expression should I use, if I want to define it as an instance function on spot?

bool IsOlderThan(Classification classification) => this.Timestamp < classification.Timestamp;

@roji
Copy link
Member

roji commented Nov 3, 2023

@voroninp can you please open a separate issue with the precise repro code sample that shows what additional check you're proposing?

@soroshsabz
Copy link

ITNOA

Hi,

How many votes needed to start implementation of this feature?

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

No branches or pull requests