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

Check for custom sql translation defined on C# special methods: op_LessThan, op_GreaterThan, etc. #32227

Closed
voroninp opened this issue Nov 3, 2023 · 11 comments
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported

Comments

@voroninp
Copy link

voroninp commented Nov 3, 2023

Currently EF Core does not provide an easy way to define custom SQL translations or AST transformations for operations defined on converted types.

For example, SQLite does not support DateTimeOffset type and it must be converted to int or string.

@roji proposed a workaround, but I think it would be nice, if I could define SQL translation for op_LessThan method of DateTimeOffset type:

modelBuilder.HasDbFunction(
    typeof(DateTimeOffset).GetMethod(
        nameof(op_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)));

So, query items.Where(i => i.TimeStamp < yesterday) is properly translated to SQL expression.

Right now, I must write it like this: items.Where(i => SqlTranslation.LessThan(i.TimeStamp, yesterday))

It means EF should make additional step when translating expression tree. When it sees one of comparison expressions it should check whether there's a translation defined for a corresponding method.

The major issue with this behavior would be that properties of the same type can be converted differently (or not converted at all) for various properties. But SQL translation applied to special methods is global. Whereas custom function provides flexibility. However, one must check whether type is converted and what function should be used in LINQ for correct translation.

Thus, it's still better to define translations within value converters.

@roji
Copy link
Member

roji commented Nov 3, 2023

@voroninp operators such as < are generally just translated as-is; that is, assuming both sides of the binary expression can be translated, RelationalSqlTranslatingExpressionVisitor.VisitBinary just creates the appropriate SqlBinaryExpression. In other words, I'm seeing why you'd need the above code at all, even if DateTimeOffset requires a value converter.

Can you please provide a bit more context, or post the code that doesn't work without what you're asking for?

@voroninp
Copy link
Author

voroninp commented Nov 3, 2023

I showed the query which won't work for SQLite database when Timestamp property is of DateTimeOffset type.
This is because there's a value converter applied to it.

@roji
Copy link
Member

roji commented Nov 3, 2023

Can you please post a repro for that not working?

@voroninp
Copy link
Author

voroninp commented Nov 3, 2023

@roji
Copy link
Member

roji commented Nov 4, 2023

  • First, your repro code fails because SQLite has no translation for DateTimeOffset.UtcNow - this isn't related to the comparison operator.
  • When removing that (i.e. doing .Where(f => f.Timestamp > f.Timestamp)), the query indeed fails because of the comparison. However, this isn't because it "doesn't find a method translator" - we very intentionally block comparisons between DateTime/TimeSpan/DateTimeOffset types in SQLite (code), since simply allowing a greater-than operator would perform a string (lexicographic) comparison, which would produce the wrong results.
  • You're bypassing this check above with SqlTranslation.LessThan, but that really is incorrect: comparing the text representation of two DateTimeOffsets is not the same as comparing the two DateTimeOffsets.

@bricelam do we have an issue for adding translations for DateTimeOffset? I can see #18844 for TimeSpan, but nothing for DTO.

@voroninp
Copy link
Author

voroninp commented Nov 4, 2023

@roji oops..I assumed string representation saves order of DTO, at least for UTC representation.

But ok, problem stays the same if DTO is converted to int.

I still cannot define custom SQL for comparison operator on DTO ;-)

@roji
Copy link
Member

roji commented Nov 4, 2023

EF indeed does not support overriding comparison operators - or any other kind of operator - with custom logic, only function calls. Allowing custom operator overriding logic would mean introducing hooks in many difference expression types (binary, but also unary, casting?), and a whole new API just to allow users to do this. Given that it's already possible to just add a custom function - as you've done for the workaround - I highly doubt that we'd implement something like this.

@voroninp
Copy link
Author

voroninp commented Nov 4, 2023

Fair enough. And there's already a pending issue for custom translations for value generators.

Btw, is it possible to assign custom translation to entity's instance method/property?

For example I have a Program entity with IsAdvertising calculated property.
It's just a switch over another property, and if property access is replaced with switch equivalent in linq expression it gets translated into SQL without problems.

So I either need a mechanism to substitute PropertyExpression/CallExpression in expression tree, so it gets translated by EF, or provide SQL translation.
But I couldn't find the way.

@voroninp
Copy link
Author

voroninp commented Nov 7, 2023

Here's the example:

public sealed record RadioProgram(
    ushort ChannelId,

    [StringLength(MaxLengthOf.ProgramHarmonizedTitle)]
    string HarmonizedTitle)
{
    public static readonly IReadOnlySet<ProgramType> AdvertisingProgramTypes = new HashSet<ProgramType>
    {
        ProgramType.Billboard,
        ProgramType.AnnouncementFromTheGovernment,
        ProgramType.Break,
        ProgramType.Audioboard,
    }.AsReadOnly();

    public int Id { get; private set; }

    public required ProgramType ProgramType { get; set; }
    public required DateIndex OnDate { get; set; }
    public required DateTimeOffset Timestamp { get; set; }

    public string? BreakCode() => ProgramType switch
    {
        _ when IsAdvertising() => HarmonizedTitle,
        _ => null,
    };

    public bool IsAdvertising() => AdvertisingProgramTypes.Contains(ProgramType);
}

And I obviously cannot call ctx.Set<RadioProgram>().Where(p => p.IsAdvertising())

So I declared in DbContext:

public static Expression<Func<RadioProgram, bool>> ProgramIsAnAdvertising { get; }
    =
    p => p.ProgramType == ProgramType.Billboard || p.ProgramType == ProgramType.AnnouncementFromTheGovernment
        || p.ProgramType == ProgramType.Break || p.ProgramType == ProgramType.Audioboard;

and I can make a call: ctx.Set<RadioProgram>().Where(AppDbContext.ProgramIsAnAdvertising)

It would be nice to have a mechanism for automatic (configured) replacement of IsAdvertising with AppDbContext.ProgramIsAnAdvertising even before the expression tree is translated to SQL.

@roji
Copy link
Member

roji commented Nov 12, 2023

Btw, is it possible to assign custom translation to entity's instance method/property?

Specifying a custom translation for CLR properties isn't currently possibly (barring messing around with visitors in preprocsesing). This is tracked by #10768.

I'll go ahead and close this the above questions were answered and I don't think anything actionable remains.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Nov 12, 2023
@roji roji added closed-no-further-action The issue is closed and no further action is planned. and removed type-enhancement labels Nov 12, 2023
@bricelam
Copy link
Contributor

@bricelam do we have an issue for adding translations for DateTimeOffset?

No.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-no-further-action The issue is closed and no further action is planned. customer-reported
Projects
None yet
Development

No branches or pull requests

3 participants