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

Temporal table support for owned and table splitting scenarios #26935

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Dec 8, 2021

Fix to #26858 - Query: improve TableExpressionBase extensibility by adding annotations
Fix to #26469 - Query: enable temporal tables for table splitting scenarios
Fix to #26451 - Temporal Table: Owned Entities support?

Added annotation infra for TableExpressionBase and annotations for temporal table information. Removed (now unnecessary) temporal specific table expressions.
Also added temporal table support for owned typed and table splitting in general using the annotations to store the temporal information (no need for provider specific logic in places where we didn't have good extensibility)
For table splitting, every entity must explicitly define period start/end columns. They all need to be the same, but if not explicitly provided we try to uniquefy them. We should fix this so that you only need to specify it in one place but it's currently hard to do (hopefully convention layering will make it easier)

Fixes #26858
Fixes #26469
Fixes #26451

@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch from c173a6b to 421ec8c Compare December 8, 2021 05:48
@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch from 421ec8c to 58f7223 Compare December 8, 2021 07:51
@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch 3 times, most recently from d861a8f to de6c2e2 Compare December 9, 2021 13:12
@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch from de6c2e2 to 67bcbe1 Compare December 10, 2021 11:37
@smitpatel
Copy link
Member

        ? new CrossApplyExpression(table)

Forgot to pass annotations.


Refers to: src/EFCore.Relational/Query/SqlExpressions/CrossApplyExpression.cs:43 in 67bcbe1. [](commit_id = 67bcbe1, deletion_comment = False)

@smitpatel
Copy link
Member

        ? new InnerJoinExpression(table, joinPredicate)

annotations


Refers to: src/EFCore.Relational/Query/SqlExpressions/InnerJoinExpression.cs:53 in 67bcbe1. [](commit_id = 67bcbe1, deletion_comment = False)

@smitpatel
Copy link
Member

        ? new LeftJoinExpression(table, joinPredicate)

annotations


Refers to: src/EFCore.Relational/Query/SqlExpressions/LeftJoinExpression.cs:53 in 67bcbe1. [](commit_id = 67bcbe1, deletion_comment = False)

@smitpatel
Copy link
Member

        ? new OuterApplyExpression(table)

annotations


Refers to: src/EFCore.Relational/Query/SqlExpressions/OuterApplyExpression.cs:43 in 67bcbe1. [](commit_id = 67bcbe1, deletion_comment = False)

@smitpatel
Copy link
Member

    expressionPrinter.AppendLine()

things other than table expression is not printing out annotations


Refers to: src/EFCore.Relational/Query/SqlExpressions/UnionExpression.cs:81 in 67bcbe1. [](commit_id = 67bcbe1, deletion_comment = False)

@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch 2 times, most recently from f5c0ad2 to 2dec3c8 Compare December 17, 2021 01:10
@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch from 2dec3c8 to c04b67c Compare January 13, 2022 03:21
@maumar
Copy link
Contributor Author

maumar commented Jan 13, 2022

new (hopefully final) version is up @AndriySvyryd

@AndriySvyryd
Copy link
Member

new (hopefully final) version is up @AndriySvyryd

It's best to keep the commits separate once the PR has been reviewed.

@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch 2 times, most recently from 12cc146 to c0b43a9 Compare January 13, 2022 17:21
Fix to #26858 - Query: improve TableExpressionBase extensibility by adding annotations
Fix to #26469 - Query: enable temporal tables for table splitting scenarios
Fix to #26451 - Temporal Table: Owned Entities support?

Added annotation infra for TableExpressionBase and annotations for temporal table information. Removed (now unnecessary) temporal specific table expressions.
Also added temporal table support for owned typed and table splitting in general using the annotations to store the temporal information (no need for provider specific logic in places where we didn't have good extensibility)
For table splitting, every entity must explicitly define period start/end columns. They all need to be the same, but if not explicitly provided we try to uniquefy them. We should fix this so that you only need to specify it in one place but it's currently hard to do (hopefully convention layering will make it easier)

Fixes #26858
Fixes #26469
Fixes #26451
@maumar maumar force-pushed the temporal_table_all_fixes_annotations branch from c0b43a9 to 1ca6380 Compare January 13, 2022 21:38
@maumar maumar merged commit 1ca6380 into main Jan 13, 2022
@maumar maumar deleted the temporal_table_all_fixes_annotations branch January 13, 2022 22:48
@spallister
Copy link

Hi @maumar - do you happen to know if this fix will be made available in a minor EF Core 6 update at some point, or will it not be available until EF Core 7 is released? Was excited to see support for temporal tables with EF Core 6, however, then realized we could not use the functionality since we are using Owned Entities in some cases.

Looking forward to your reply!

@AndriySvyryd
Copy link
Member

@spallister Follow #27380 for the answer to your question

@spallister
Copy link

Oh great - Thanks, @AndriySvyryd!

@mu-dawood
Copy link

when i have multiple owned types i get this error

When multiple temporal entities are mapped to the same table, their period start properties must map to the same column. Issue happens for entity type 'Location' with period property 'PeriodStart' which is mapped to column 'Location_PeriodStart'. Expected period column name is 'BankInfo_PeriodStart'.

Here is my code

        builder.Entity<Merchant>().ToTable((o) =>
                {
                    o.IsTemporal(t =>
                    {
                        t.HasPeriodEnd(PeriodEnd);
                        t.HasPeriodStart(PeriodStart);
                    });
                });
        builder.Entity<Merchant>().OwnsOne(o => o.CommerialRegistration, x => ConfigureTemporal(x));
        builder.Entity<Merchant>().OwnsOne(o => o.NationalId, x => ConfigureTemporal(x));
        builder.Entity<Merchant>().OwnsOne(o => o.BankInfo, x => ConfigureTemporal(x));
        builder.Entity<Merchant>().OwnsOne(o => o.Location, x => ConfigureTemporal(x));
    void ConfigureTemporal<TEntity, TRelatedEntity>(OwnedNavigationBuilder<TEntity, TRelatedEntity> builder) where TEntity : class where TRelatedEntity : class
    {
        builder.WithOwner();
        builder.ToTable(t => t.IsTemporal(t =>
                     {
                         t.HasPeriodEnd(PeriodEnd);
                         t.HasPeriodStart(PeriodStart);
                     }));
    }

@mu-dawood
Copy link

it was solved by .HasColumnName(PeriodStart); but it must be mapped automatic

@Badea741
Copy link

Badea741 commented Aug 24, 2023

@maumar
This can't be enabled in EF 6 with owned entity types even if configuring them with the same period
It works fine with EF 7
So I'm asking is this support for temporal tables with owned types available in EF 6 and if yes, how to fix this issue?

modelBuilder.Entity<Customer>(customer =>
        {
            customer.ToTable("Customers", t => t.IsTemporal(x =>
            {
                x.UseHistoryTable("CustomersHistory");
                x.HasPeriodStart("PeriodStart");
                x.HasPeriodEnd("PeriodEnd");
            }));
            customer.Property<DateTime>("PeriodStart").HasColumnName("PeriodStart");
            customer.Property<DateTime>("PeriodEnd").HasColumnName("PeriodEnd");
            customer.OwnsOne(x => x.CustomerDetail, customerDetail =>
            {
                customerDetail.ToTable("Customers", t => t.IsTemporal(x =>
                {
                    x.UseHistoryTable("CustomersHistory");
                    x.HasPeriodStart("PeriodStart");
                    x.HasPeriodEnd("PeriodEnd");
                }));
                customerDetail.Property<DateTime>("PeriodStart").HasColumnName("PeriodStart");
                customerDetail.Property<DateTime>("PeriodEnd").HasColumnName("PeriodEnd");
            });
        });

and that's the error message
image

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