Conversation
…erator for cosmos
…complex-properties-query
…complex-properties-query
c4dea47 to
3b21620
Compare
There was a problem hiding this comment.
Pull request overview
Adds Cosmos DB query pipeline support for complex/structural types (complex properties), expanding translation capabilities and adding Cosmos-specific test coverage to validate expected SQL and failure behaviors.
Changes:
- Generalizes Cosmos projection/binding from entity-only to structural types via
StructuralTypeProjectionExpression, adding complex-property binding support. - Introduces structural equality translation for complex types (including parameter extraction support) and adds a Distinct translation guard when client projection is involved.
- Adds/updates Cosmos functional tests for complex type queries and complex-property association scenarios (projection, equality, set operations, collections, primitive collections), plus a few ad-hoc helpers/fixtures.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.Specification.Tests/Query/AdHocComplexTypeQueryTestBase.cs | Adjusts ad-hoc complex-type test seeding and exposes context for provider-specific derivations. |
| test/EFCore.Cosmos.FunctionalTests/Query/NorthwindAggregateOperatorsQueryCosmosTest.cs | Updates Distinct-related baselines and replaces CI skip attribute. |
| test/EFCore.Cosmos.FunctionalTests/Query/ComplexTypeQueryCosmosTest.cs | New Cosmos-specific complex type query test baselines and translation-failure expectations. |
| test/EFCore.Cosmos.FunctionalTests/Query/Associations/OwnedNavigations/OwnedNavigationsProjectionCosmosTest.cs | Adds a Distinct-over-owned-associate translation-failure test. |
| test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesStructuralEqualityCosmosTest.cs | New Cosmos tests validating complex/associate structural equality translation behavior. |
| test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesSetOperationsCosmosTest.cs | New Cosmos tests for set operations over complex-property association scenarios. |
| test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs | Enables previously-skipped projection tests and updates baselines accordingly. |
| test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesPrimitiveCollectionCosmosTest.cs | New Cosmos tests for primitive-collection operations under complex properties. |
| test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesMiscellaneousCosmosTest.cs | New Cosmos tests for miscellaneous complex-property predicate/value-type translations. |
| test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesCollectionCosmosTest.cs | New Cosmos tests for complex-property collection operators (Count/Where/Distinct/Index/etc.). |
| test/EFCore.Cosmos.FunctionalTests/Query/AdHocCosmosTestHelpers.cs | Adds a helper to configure deterministic int key generation for Cosmos ad-hoc tests. |
| test/EFCore.Cosmos.FunctionalTests/Query/AdHocComplexTypeQueryCosmosTest.cs | New Cosmos ad-hoc complex-type test with Cosmos SQL baselines and Cosmos-specific initialization. |
| test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs | Changes one tracking test to skip sync rather than silently completing, and runs async path. |
| src/EFCore.Cosmos/Query/Internal/SqlExpressionVisitor.cs | Updates visitor dispatch to the new structural projection expression type. |
| src/EFCore.Cosmos/Query/Internal/Expressions/StructuralTypeProjectionExpression.cs | Renames/generalizes entity projection to structural projection and adds complex-property binding. |
| src/EFCore.Cosmos/Query/Internal/Expressions/SelectExpression.cs | Tracks whether client projection is used (for later translation decisions like Distinct). |
| src/EFCore.Cosmos/Query/Internal/Expressions/ObjectReferenceExpression.cs | Generalizes object references from IEntityType to ITypeBase. |
| src/EFCore.Cosmos/Query/Internal/Expressions/ObjectArrayAccessExpression.cs | Extends array-access expressions to support complex-property collections. |
| src/EFCore.Cosmos/Query/Internal/Expressions/ObjectAccessExpression.cs | Extends object-access expressions to support complex properties and structural types. |
| src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs | Refactors to partial class and routes equality rewriting to structural-type-aware logic. |
| src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.StructuralEquality.cs | New structural equality implementation for entity/complex comparisons and parameter extraction. |
| src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs | Updates projection binding removal to work with structural projections and new access expressions. |
| src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs | Enables complex-property collection translation paths and prevents Distinct in client-projection scenarios. |
| src/EFCore.Cosmos/Query/Internal/CosmosQuerySqlGenerator.cs | Updates SQL generator to use the new structural projection expression. |
| src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs | Marks selects that fall back to client projection, impacting later translation decisions. |
| src/EFCore.Cosmos/Query/Internal/CosmosAliasManager.cs | Updates alias rewriting for structural object references. |
| src/EFCore.Cosmos/Extensions/Internal/CosmosShapedQueryExpressionExtensions.cs | Updates projection extraction logic for the renamed structural projection expression. |
Comments suppressed due to low confidence (3)
src/EFCore.Cosmos/Query/Internal/Expressions/StructuralTypeProjectionExpression.cs:186
BindComplexPropertythrowsUnableToBindMemberToEntityProjectionwith memberType set to "navigation". This looks like a copy/paste error and will produce a misleading error message for complex properties; use a distinct memberType (e.g. "complexProperty") like the relational structural projection does.
src/EFCore.Cosmos/Query/Internal/Expressions/StructuralTypeProjectionExpression.cs:287UpdateEntityTypeshould only be callable whenStructuralTypeis anIEntityType. As written, it usesderivedType.GetAllBaseTypes().Contains(StructuralType)which mixesITypeBasewith the entity-type hierarchy and can misbehave (or fail to compile, depending on theGetAllBaseTypes()return type). Align this with the relational implementation by first asserting/castingStructuralTypetoIEntityTypeand then performing the base-type check against that entity type.
src/EFCore.Cosmos/Query/Internal/Expressions/StructuralTypeProjectionExpression.cs:337ToString()still returns the prefix "EntityProjectionExpression" even though the type was renamed toStructuralTypeProjectionExpression, which makes debugging/logging confusing. Update the string to reflect the new type name.
test/EFCore.Cosmos.FunctionalTests/Query/AdHocCosmosTestHelpers.cs
Outdated
Show resolved
Hide resolved
| var accessExpression = objectArrayAccess.InnerProjection.Object; | ||
| _projectionBindings[accessExpression] = jObjectParameter; | ||
| _ownerMappings[accessExpression] = | ||
| (objectArrayAccess.Navigation.DeclaringEntityType, objectArrayAccess.Object); | ||
| ((IEntityType)objectArrayAccess.PropertyBase.DeclaringType, objectArrayAccess.Object); | ||
| _ordinalParameterBindings[accessExpression] = Add( |
There was a problem hiding this comment.
objectArrayAccess.PropertyBase can now be an IComplexProperty (complex collection). The cast to (IEntityType)objectArrayAccess.PropertyBase.DeclaringType will throw if the declaring type is a complex type. If _ownerMappings is only needed for owned navigations, guard this assignment to the navigation case (or change the mapping to store ITypeBase).
There was a problem hiding this comment.
It can't be as complex types are always handled by CosmosShapedQueryCompilingExpressionVisitor.AddStructuralTypeInitialization Should I add a comment here to clarify?
test/EFCore.Cosmos.FunctionalTests/Query/AdHocCosmosTestHelpers.cs
Outdated
Show resolved
Hide resolved
| accessExpression = objectAccessExpression.Object; | ||
| storeNames.Add(objectAccessExpression.PropertyName); | ||
| _ownerMappings[objectAccessExpression] | ||
| = (objectAccessExpression.Navigation.DeclaringEntityType, accessExpression); | ||
| = ((IEntityType)objectAccessExpression.PropertyBase.DeclaringType, accessExpression); | ||
| } |
There was a problem hiding this comment.
_ownerMappings assumes the access chain represents owned navigations and casts objectAccessExpression.PropertyBase.DeclaringType to IEntityType. With the new PropertyBase support for complex properties, ObjectAccessExpression can now represent an IComplexProperty, whose DeclaringType may be an IComplexType—this cast will throw at runtime. Consider only populating _ownerMappings when PropertyBase is an INavigation (owned entity navigation), and skip/handle complex-property accesses separately.
There was a problem hiding this comment.
It can't be as complex types are always handled by CosmosShapedQueryCompilingExpressionVisitor.AddStructuralTypeInitialization Should I add a comment here to clarify?
src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.StructuralEquality.cs
Outdated
Show resolved
Hide resolved
|
@JoasE thanks for this. I'll hold off reviewing until you resolve the Copilot comments above, just to save on the effort (we can do this in future PRs as well). Feel free to ping me once everything's ready for a human review. |
|
@roji btw, this should be ready now, incase you missed it! |
…complex-properties-query
roji
left a comment
There was a problem hiding this comment.
@JoasE finally got around to looking at this, thanks for your patience. I reviewed up to a point, but noticed that there are some significant, unexplained discrepancies between the Cosmos implementation and the relational one (see comments). We can of course do things differently in Cosmos where there's a real difference/motivation, but we should otherwise strive to have the implementations more or less aligned, at least in principle - this would both make it easier to work across the providers, and possibly to extract out some common functionality in the future.
So I'll hold off completing the review for now. I'm happy to discuss point-by-point, but overall can you please go over and compare your implementation to the corresponding relational one, and either (a) align it where it makes sense, or (b) provide a justification for the discrepancy where it doesn't?
src/EFCore.Cosmos/Extensions/Internal/CosmosShapedQueryExpressionExtensions.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/Expressions/ObjectAccessExpression.cs
Outdated
Show resolved
Hide resolved
| /// any release. You should only use it directly in your code with extreme caution and knowing that | ||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public virtual ShapedQueryExpression? Subquery { get; } |
There was a problem hiding this comment.
This all looks quite different from the relational CollectionResultExpression, which does not have Subquery vs. Parameter (those exist on StructuralTypeReferenceExpression, which is a very different thing). I'm not completely sure what design you're going for here, but it's important to try to align across the providers, at least unless there's a good reason not to. In relational, CollectionResultExpression is the shaper-side expression for a collection of structural types, so it should be oblivious to whether the source of the collection is a subquery or not.
src/EFCore.Cosmos/Query/Internal/Expressions/ObjectArrayAccessExpression.cs
Outdated
Show resolved
Hide resolved
| /// any release. You should only use it directly in your code with extreme caution and knowing that | ||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| public bool UsesClientProjection { get; private set; } |
There was a problem hiding this comment.
This is a completely new concept, and I'm not sure how it relates to implementing complex types... Once again, we don't have anything like this in relational - you'll have to explain this and why it's necessary on Cosmos.
src/EFCore.Cosmos/Query/Internal/Expressions/StructuralTypeProjectionExpression.cs
Outdated
Show resolved
Hide resolved
| if (!_complexPropertyExpressionsMap.TryGetValue(complexProperty, out var expression)) | ||
| { | ||
| // TODO: Unify ObjectAccessExpression and ObjectArrayAccessExpression | ||
| expression = complexProperty.IsCollection |
There was a problem hiding this comment.
This differs significantly from the relational implementation - the latter assumes that _complexPropertyExpressionsMap contains the complexProperty (i.e. is pre-populated), and throws otherwise, whereas here you lazily create and populate it if it's not in the dictionary.
|
@roji Thanks for looking at this! This might be a bit premature but felt like explaining If you need anything else from me, let me know! I will also try to do a better job explaining / leaving comments where I make certain choices in the future. If they are too bulky I can always remove them later. |
…complex-properties-query
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.StructuralEquality.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
| /// <summary> | ||
| /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to | ||
| /// the same compatibility standards as public APIs. It may be changed or removed without notice in | ||
| /// any release. You should only use it directly in your code with extreme caution and knowing that | ||
| /// doing so can result in application failures when updating to a new Entity Framework Core release. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This property indicates whether the query uses client-side projection. We have to keep track of this | ||
| /// because of #34067. We can't apply distinct to queries with client-side projection. | ||
| /// </remarks> | ||
| public bool UsesClientProjection { get; private set; } | ||
|
|
There was a problem hiding this comment.
UsesClientProjection is tracked as mutable state on SelectExpression, but it isn't copied over when the select is cloned/updated (e.g. via VisitChildren, Update, WithReadItemInfo, WithSingleValueProjection). That can reset the flag back to false after it was set, allowing TranslateDistinct to apply DISTINCT even though the query has client-side projection (the scenario this flag is meant to guard). Consider propagating UsesClientProjection to any newly-constructed SelectExpression instances created from an existing one.
Adds Cosmos Db query pipeline support for complex properties
Part of: #31253
Replace IEntityType and IProperty with ITypeBase and IPropertyBase in appropriate places. Along with a couple of class and variable renames
Adds ComplexProperty binding for StructuralTypeProjectionExpression
Do not translate distinct in TranslateDistinct with projections of child documents, related: #34067
Translate Nullable<>.HasValue to != null and Nullable<>.Value to direct property access
Move structural equality to partial class
Translate complex equality by direct comparison
Use CollectionResultExpression for complex property collections
Add assosiations, query and adhoc tests
list.Contains(complexType) not supported as per: 36468
TypeIs not supported, as per: 31250