Work in progress on composite support #708

roji · 2018-11-17T13:27:57Z

This is work in progress on composite support for EFCore.PG 2.2, allowing you to map a CLR type to a PostgreSQL column defined as a composite type. This leverages composite support at the Npgsql level which is already there.

It's definitely not complete, but I wanted to get some feedback before going any deeper. It's probably a bit ambitious to push this for 2.2, but we may have enough value to deliver something.

What works:

ForNpgsqlHasComposite() can be used to generate composite types in the database (much like ForNpgsqlHasEnum(). Currently there's no overload the infers the definition from the CLR type (like for enums).
NpgsqlMigrationsSqlGenerator picks up the annotations and generates the types in the database (CREATE TYPE ... AS (...)).
NpgsqlTypeMappingSource pulls global composite mappings from the ADO layer (like enums) and sets up new NpgsqlCompositeTypeMapping, including the configured NameTranslator. This requires exposing a new interface at the ADO level, so this depends on the unreleased 4.0.4.
NpgsqlSqlTranslatingExpressionVisitor identifies MemberExpressions which traverse a composite type by finding a composite mapping for the CLR type. It then creates a new NpgsqlCompositeMemberExpression, including the composite's translated member name.
NpgsqlCompositeMemberExpression is picked up by NpgsqlQuerySqlGenerator, which generates the appropriate member access SQL.
All this allows you to query on composite fields (e.g. WHERE my_entity.my_composite.some_field > 100).
Nested composites are (somewhat) supported. You can create them - the composite annotation contains an ordering vaue, allowing the nested composite to be created before the outer composite. Querying doesn't yet work though (see below).

What doesn't work:

NpgsqlCompositeTypeMapping does not contain information about the composite's types. Unlike for enums, this cannot be inferred only from the CLR type, since the order of the composite's field is important and it only exists in the database. Unfortunately at the ADO level the actual database definition is only loaded when we actually connect to the database, but the EF Core layer looks at global composite mappings, which are set up without any specific databases. We'll have to think about what to do with this.
This means that SQL and possibly code literals are unsupported at this time, because composite field knowledge is required at the mapping level. I think there's enough value without these for us to release this, but let me know what you think.
Queries on fields of nested composites aren't translated yet, because MemberAccessBindingExpressionVisitor.GetPropertyPath does not traverse the outer composite. I think it's OK to move forward while documenting this limitation.
Composite types currently cannot be altered in any way after creation (although this is supported by PostgreSQL). Again, not a blocker IMO.
Scaffolding isn't yet implemented, although it shouldn't be difficult. This would mean scaffolding ForNpgsqlHasComposite(), but it will be up to the user to actually create the relevant CLR type and entity properties. This partial support is currently what we do for enums, maybe in the future we can actually generate the CLR types as well.
No tests yet (tested ad-hoc)

@ajcvickers @divega @smitpatel you may be interested in this work which seems to push the limits a little.

austindrenski

Only had time for a partial review. I'll add another one with more coverage later today/tomorrow.

src/EFCore.PG/Design/Internal/NpgsqlAnnotationCodeGenerator.cs

src/EFCore.PG/Extensions/NpgsqlModelBuilderExtensions.cs

src/EFCore.PG/Metadata/PostgresComposite.cs

roji · 2018-11-18T11:37:58Z

@austindrenski have pushed a commit addressing your comments thanks. Although this seems like it's too early for nits and other minor comments - I'm looking more for general comments on the approach, possible suggestions on how to handle the remaining issues, and opinions on whether this is viable with the limitations listed above (and for 2.2).

austindrenski

Another couple of questions. I'm still looking over the changes to NpgsqlTypeMappingSource, but generally, I think this looks viable with the limitations.

Whether it's viable for 2.2 depends on how long we have until the release... I'm not expecting anything until after the Thanksgiving holiday, which means we could have at least another week to work on this.

In the meantime, some additional tests would be helpful to get a better picture of what is and is not working (e.g. a couple of Assert.Throws would be illustrative).

austindrenski · 2018-11-20T02:06:21Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

@@ -659,6 +659,7 @@ public virtual void Generate(NpgsqlDropDatabaseOperation operation, [CanBeNull]
            Check.NotNull(builder, nameof(builder));

            GenerateEnumStatements(operation, model, builder);
+            GenerateCompositeStatements(operation, model, builder);
            GenerateRangeStatements(operation, model, builder);


Should we create user-defined ranges first, so that they exist for use by composites?

austindrenski · 2018-11-20T02:27:39Z

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

+                // TODO: Schema support for the field type
+                .Append(string.Join(", ", compositeType.Fields.Select(
+                    f => $"{Dependencies.SqlGenerationHelper.DelimitIdentifier(f.Name)} {Dependencies.SqlGenerationHelper.DelimitIdentifier(f.StoreType)}")))
+                .AppendLine(");");


Is there a blocking issue for schema support here, or was it just simpler to skip it for now?

austindrenski · 2018-11-20T02:35:02Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

@@ -62,7 +77,10 @@ public class NpgsqlSqlTranslatingExpressionVisitor : SqlTranslatingExpressionVis
            [CanBeNull] Expression topLevelPredicate = null,
            bool inProjection = false)
            : base(dependencies, queryModelVisitor, targetSelectExpression, topLevelPredicate, inProjection)
-            => _queryModelVisitor = queryModelVisitor;
+        {
+            _typeMappingSource = (NpgsqlTypeMappingSource)dependencies.TypeMappingSource;


Does this need to be NpgsqlTypeMappingSource rather than TypeMappingSource?

austindrenski · 2018-11-20T02:40:36Z

src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs

+                memberExpression.Expression, _queryModelVisitor.QueryCompilationContext, out _);
+            if (properties.Count == 0)
+                return null;
+            var lastPropertyType = properties[properties.Count - 1].ClrType;


I know I've seen this elsewhere in the provider/upstream, but it always looks fragile... Could we move it into a helper and clarify what exactly this access pattern is doing?

austindrenski · 2018-11-20T03:29:13Z

src/EFCore.PG/Storage/Internal/Mapping/NpgsqlCompositeTypeMapping.cs

+{
+    public class NpgsqlCompositeTypeMapping : RelationalTypeMapping
+    {
+        [NotNull] static readonly NpgsqlSqlGenerationHelper SqlGenerationHelper =


I think this class needs to be updated along the lines of #626.

austindrenski · 2018-11-20T03:35:16Z

src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs

+            if (mappingInfo.ClrType?.IsEnum != true)
+                return null;   // ClrType was given and is not an enum
+
+            return null;  // NotImplemented


I'm not sure I follow what's happening here... Shouldn't this have the previous enum logic?

roji · 2018-11-22T10:40:58Z

FYI there's a good chance we'll end up punting this for 3.0... @divega suggested an idea for working around the current lack of information in NpgsqlCompositeMapping, but there's a lot going on at the moment and 3.0 may change the query pipeline internals quite a bit in ways that may impact this. I'll still go over your review though, @austindrenski.

YohDeadfall · 2018-11-22T12:21:12Z

It's a really large change, so I can't review it right now because of the presentation I mentioned. But definitely will do it in the nearest future.

Could you share @divega's suggestion with us?

roji · 2018-11-22T15:47:22Z

Could you share @divega's suggestion with us?

Sure, sorry about that, was in a bit of a hurry...

Basically the EF Core way of doing things is to manage as much as possible without a connection to the database. This is true when generating migrations, type mappings etc.

Now, when first doing the support for enums, my idea was to pull all enum mappings from the ADO layer, since in any case the user has to set up enum mappings there (NpgsqlConnector.GlobalTypeMapper.MapEnum()). Crucially, though, the global ADO mapping is enough to set up everything at the EF Core level as well: we get the CLR type, the database type and the name translator, and don't need anything else.

Composite types are a bit different - the global mapping set up at the ADO level isn't enough for us; although we can get the fields from the CLR type (just like the enum) and even use the translator to generate their database counterparts, we're missing their order, since the CLR property order will not necessarily correspond to the PostgreSQL composite type's field order. And the literal representation of a composite in PostgreSQL is positional - you need to specify values in the order of the composite fields (without names) - so you must know the order. At the ADO layer, the order is only known when the composite type is actually loaded from the database, when the first connection is instantiated and the types are loaded from pg_type and related tables - but the EF Core type mapper has no access to any specific connection.

So one way to solve this would be to somehow lazily communicate the database composite definition (with the order) to the EF Core type mapper/mapping on the first physical connection. That's complicated, and in theory you could have two databases with two different definitions being accessed with the same type mapper.

@divega's suggestion was simply to make sure the user communicates all the necessary information at the EF Core level (i.e. context options) rather than attempting to pull in information from the ADO layer. This is more in line with who EF Core works and should work fine, but has its own difficulties - the user would need to duplicate the information already mapped at the ADO layer (store type, CLR type, name translator). But it's definitely doable.

Then, for generating code literals (for data seeding) there are other challenges... To generate an instantiation of an arbitrary user CLR type we'd probably need to look for an appropriate constructor on the type (matching parameter names and types) and use that - not trivial but doable. As an alternative we could initialize the individual properties after the constructor (new UserType() { X=1, Y=2}), but I'm pretty sure the EF Core C# generator (CSharpHelper.cs) doesn't support the kind of expression yet (we could add it for 3.0 though).

smitpatel · 2018-11-27T20:27:50Z

Looks good from query perspective. Nested would be difficult due to MemberAccessBinding GetPropertyPath method. That method has deviated too far for initial use case and we certainly need better way of binding. I have that on my radar for 3.0

Work in progress on composite support

47d976e

roji requested review from YohDeadfall and austindrenski November 17, 2018 13:27

austindrenski added the enhancement New feature or request label Nov 17, 2018

austindrenski suggested changes Nov 17, 2018

View reviewed changes

Address partial review by @austindrenski

ec1e3db

austindrenski reviewed Nov 20, 2018

View reviewed changes

roji force-pushed the dev branch from 00bbc9b to d5fc5e3 Compare December 13, 2018 09:47

roji marked this pull request as draft May 25, 2020 22:20

roji closed this Sep 8, 2020

roji deleted the composite branch September 8, 2020 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work in progress on composite support #708

Work in progress on composite support #708

roji commented Nov 17, 2018 •

edited

Loading

austindrenski left a comment

roji commented Nov 18, 2018

austindrenski left a comment

austindrenski Nov 20, 2018

austindrenski Nov 20, 2018

austindrenski Nov 20, 2018

austindrenski Nov 20, 2018

austindrenski Nov 20, 2018

austindrenski Nov 20, 2018

roji commented Nov 22, 2018

YohDeadfall commented Nov 22, 2018

roji commented Nov 22, 2018

smitpatel commented Nov 27, 2018

Work in progress on composite support #708

Work in progress on composite support #708

Conversation

roji commented Nov 17, 2018 • edited Loading

austindrenski left a comment

Choose a reason for hiding this comment

roji commented Nov 18, 2018

austindrenski left a comment

Choose a reason for hiding this comment

austindrenski Nov 20, 2018

Choose a reason for hiding this comment

austindrenski Nov 20, 2018

Choose a reason for hiding this comment

austindrenski Nov 20, 2018

Choose a reason for hiding this comment

austindrenski Nov 20, 2018

Choose a reason for hiding this comment

austindrenski Nov 20, 2018

Choose a reason for hiding this comment

austindrenski Nov 20, 2018

Choose a reason for hiding this comment

roji commented Nov 22, 2018

YohDeadfall commented Nov 22, 2018

roji commented Nov 22, 2018

smitpatel commented Nov 27, 2018

roji commented Nov 17, 2018 •

edited

Loading