-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Cosmos: Complex properties binding #37400
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
Cosmos: Complex properties binding #37400
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements binding logic for complex properties in the Cosmos DB provider, enabling proper materialization of complex types from JSON documents. The implementation introduces a custom CosmosStructuralTypeMaterializerSource that defers complex property handling to allow nested materialization expressions to be generated during query compilation.
Key changes:
- Introduces
CosmosStructuralTypeMaterializerSourceto control when complex properties are materialized during shaping - Implements nested complex property and collection binding in
CosmosShapedQueryCompilingExpressionVisitor - Enhances null-handling logic in
CosmosProjectionBindingExpressionVisitorfor nullable value types
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/EFCore.Cosmos/Query/Internal/CosmosStructuralTypeMaterializerSource.cs |
New class that overrides complex type materialization behavior to enable deferred binding |
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs |
Adds logic to generate materialization expressions for complex properties and collections from JObject/JArray |
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs |
Exposes methods and fields needed for complex property binding; adds handling for ComplexPropertyBindingExpression |
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs |
Improves null-safety checks for nullable value types and refactors member expression updates |
src/EFCore.Cosmos/Extensions/CosmosServiceCollectionExtensions.cs |
Registers the new CosmosStructuralTypeMaterializerSource service |
test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs |
Uncomments assertions that now pass with complex property binding implemented |
test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs |
New test class with Cosmos-specific overrides for complex property projection tests |
test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesCosmosFixture.cs |
Test fixture configuration for complex properties tests in Cosmos |
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Outdated
Show resolved
Hide resolved
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Show resolved
Hide resolved
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Outdated
Show resolved
Hide resolved
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosStructuralTypeMaterializerSource.cs
Outdated
Show resolved
Hide resolved
...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
roji
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @JoasE, thanks for submitting this and sorry I haven't gotten around to reviewing it earlier.
I admit that this part of the Cosmos query pipeline is generally unfamiliar to me, and also represents an older/legacy part of the EF codebase in general. I'll do my best to fully understand everything and provide meaningful feedback. As we add more test coverage I'm sure issues will be flushed out.
For now, here's a first round of review comments - most are nits but see some more important things as well.
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs
Show resolved
Hide resolved
...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs
Outdated
Show resolved
Hide resolved
…cosmos-complex-properties-binding-3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
|
@JoasE just a note - when you've handled a review comment, please resolve it to keep track of comments that still need handling (or discussion) vs. those which are completely done. (I'll re-review very soon) |
249ae47 to
6b86657
Compare
|
@roji friendly reminder in this! No worries if priorities changed :) |
|
Yeah, sorry about that - I'll really try to get to it in the next few days (and in general be more reactive to your Cosmos PRs). |
roji
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got around to this, sorry for the delay @JoasE. This looks ready to merge - see the comments below (nothing major). I'll prioritize this (and your other PRs) so we can iterate quickly and built out the support.
...osmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesCosmosFixture.cs
Outdated
Show resolved
Hide resolved
...osmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesCosmosFixture.cs
Outdated
Show resolved
Hide resolved
...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
roji
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JoasE, looking forward to the follow-up PRs!
Adds a CosmosStructuralTypeMaterializerSource and overrides AddStructuralTypeInitialization in CosmosShapedQueryCompilingExpressionVisitor to generate materialization expressions for complex properties
Leverages Nullable<>.HasValue for nullable value types in to support value types without equals operator
Part of: #31253