linq2db / linq2db.EntityFrameworkCore

Bring power of Linq To DB to Entity Framework Core projects
MIT License
462 stars 38 forks source link

CTE involving temporal table does not translate the temporal table SQL keywords #278

Closed Ephron-WL closed 1 year ago

Ephron-WL commented 1 year ago

this...

var cte = (
    from change in changeSet.TemporalAll()
    where change.Id == key
    select new
    {
        Change = change.Id,
        RowNumber = Sql.Ext.RowNumber()
            .Over()
            .PartitionBy(change.Id)
            .OrderByDesc(EF.Property<DateTime>(change, "PeriodStart"))
            .ToValue()
    })
    .AsCte("History")
    .Where(anon=>anon.RowNumber <=2)
    .ToLinqToDB();

...translates to this...

WITH [History] ([RowNumber], [Id])
AS
(
        SELECT
                ROW_NUMBER() OVER(PARTITION BY [change_1].[Id] ORDER BY [change_1].[PeriodStart] DESC),
                [change_1].[Id]
        FROM
                [Ephron.Metadata.DotNet].[Class] [change_1]
        WHERE
                [change_1].[Id] = @key_1
)
SELECT
        [anon].[Id],
        [anon].[RowNumber]
FROM
        [History] [anon]
WHERE
        [anon].[RowNumber] <= 2

... which is correct except for it is missing the temporal table keywords in the CTE...

FOR SYSTEM_TIME ALL

... it should be something like...

WITH [History] ([RowNumber], [Id])
AS
(
        SELECT
                ROW_NUMBER() OVER(PARTITION BY [change_1].[Id] ORDER BY [change_1].[PeriodStart] DESC),
                [change_1].[Id]
        FROM
                [Ephron.Metadata.DotNet].[Class] FOR SYSTEM_TIME ALL as [change_1]
        WHERE
                [change_1].[Id] = @key_1
)
SELECT
        [anon].[Id],
        [anon].[RowNumber]
FROM
        [History] [anon]
WHERE
        [anon].[RowNumber] <= 2

https://github.com/dotnet/efcore/blob/94be38cf68692d3e605175801add457936fa7331/src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs

image

I'll attempt to implement a fix in linq2db and see if the PR will be accepted.

warappa commented 1 year ago

Hi @Ephron-WL, I have the same problem - do you have any update on this?

Ephron-WL commented 1 year ago

Hi @Ephron-WL, I have the same problem - do you have any update on this?

I think I found a good solution. See this issue: https://github.com/zompinc/efcore-extensions/issues/5

The library is an extension on EF Core that uses the same expression tree evaluation technique that EF Core itself uses, so I think it is a more ideal approach to support CTEs. Now, it is important to note the issue I had with needing a subquery, which is addressed satisfactorily in the issue. The efcore-extensions author used as guidance for his work the work of the author of this library: https://github.com/PawelGerr/Thinktecture.EntityFrameworkCore . I really think we put together a good solution.

sdanyliv commented 1 year ago

Extension do not support temporal tables yet.

warappa commented 1 year ago

@Ephron-WL Thanks for your fast response. Unfortunately, I have to confess the issue thread is a little bit hard to follow as it involves yet another library. That is a route I would like to avoid for now.

@sdanyliv What would be needed to add this capability? Are there some PRs or commits which can act like a blueprint for the required changes?

warappa commented 1 year ago

Workaround (Hack)

I went the way of making a hack (don't judge me 😟): Using a ICommandInterceptor to replace the normal tables names with the tables names + temporal query expression (🤢). For my specific usecase it works.

Click here if you want to see it ```csharp public class TemporalQueryWorkaroundLinq2DbInterceptor : ICommandInterceptor { private readonly DateTime? _pointInTime; public TemporalQueryWorkaroundLinq2DbInterceptor(DateTime? pointInTime) { _pointInTime = pointInTime; } public DbCommand CommandInitialized(CommandEventData eventData, DbCommand command) { if (_pointInTime is not null) { var param = command.CreateParameter(); param.ParameterName = "p_point_in_time"; param.Value = _pointInTime.Value; command.Parameters.Add(param); var patchedCommandText = command.CommandText.Replace("[Table1]", $"[Table1] FOR SYSTEM_TIME AS OF @p_point_in_time", StringComparison.Ordinal); patchedCommandText = patchedCommandText.Replace("[Table2]", $"[Table2] FOR SYSTEM_TIME AS OF @p_point_in_time", StringComparison.Ordinal); patchedCommandText = patchedCommandText.Replace("[Table3]", $"[Table3] FOR SYSTEM_TIME AS OF @p_point_in_time", StringComparison.Ordinal); command.CommandText = patchedCommandText; } return command; } public void AfterExecuteReader(CommandEventData eventData, DbCommand command, System.Data.CommandBehavior commandBehavior, DbDataReader dataReader) { } public void BeforeReaderDispose(CommandEventData eventData, DbCommand? command, DbDataReader dataReader) { } public async Task BeforeReaderDisposeAsync(CommandEventData eventData, DbCommand? command, DbDataReader dataReader) { } public Option ExecuteNonQuery(CommandEventData eventData, DbCommand command, Option result) { return result; } public async Task> ExecuteNonQueryAsync(CommandEventData eventData, DbCommand command, Option result, CancellationToken cancellationToken) { return result; } public Option ExecuteReader(CommandEventData eventData, DbCommand command, System.Data.CommandBehavior commandBehavior, Option result) { return result; } public async Task> ExecuteReaderAsync(CommandEventData eventData, DbCommand command, System.Data.CommandBehavior commandBehavior, Option result, CancellationToken cancellationToken) { return result; } public Option ExecuteScalar(CommandEventData eventData, DbCommand command, Option result) { return result; } public async Task> ExecuteScalarAsync(CommandEventData eventData, DbCommand command, Option result, CancellationToken cancellationToken) { return result; } } ```

Proper Solution

But still, I'm interested where EF Core's TableExpression would need to be handled inside of Linq2Db to add the needed "text snippet" to the query.

sdanyliv commented 1 year ago

Will check what we can do here. Actually it needs support from linq2db side.

sdanyliv commented 1 year ago

Already released.