Skip to content

Cosmos: Modernize materializer & subdocument projectionbinding#38550

Open
JoasE wants to merge 281 commits into
dotnet:mainfrom
JoasE:feature/#34567-Materialization
Open

Cosmos: Modernize materializer & subdocument projectionbinding#38550
JoasE wants to merge 281 commits into
dotnet:mainfrom
JoasE:feature/#34567-Materialization

Conversation

@JoasE

@JoasE JoasE commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Part of: #34567
Implements: #34067
Fixes: #38138 , #31696

Copilot AI review requested due to automatic review settings July 8, 2026 09:35

Copilot AI left a comment

Copy link
Copy Markdown

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 101 out of 105 changed files in this pull request and generated 4 comments.

Files not reviewed (1)
  • src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Generated file

Copilot AI review requested due to automatic review settings July 8, 2026 10:05
@JoasE

JoasE commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Tests are failing for linux emulator because the projection order is not preserved there, and the materializer currently depends on this order. This is specifically usefull in NoTrackingWithIdentityResolution cases, where we materialize the id of the root document first before materializing sub documents. Production cosmos does preserve the projection order, but I'm wondering whether this is something we should depend and whether it's a guarantee in cosmos or might be subject to change. Regardless, I've created: Azure/azure-cosmos-db-emulator-docker#335
@AndriySvyryd Thoughts on this?
PS: PR still WIP, needs some cleanup but is close to final form

This appears to have been be planned by the linux emulator team

Copilot AI left a comment

Copy link
Copy Markdown

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 100 out of 104 changed files in this pull request and generated 4 comments.

Files not reviewed (1)
  • src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Generated file

Comment thread src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs Outdated
Comment thread src/EFCore.Cosmos/Properties/CosmosStrings.resx
Comment thread src/EFCore.Cosmos/Storage/Internal/CosmosNumberProjectionTypeMapping.cs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 8, 2026 11:15

Copilot AI left a comment

Copy link
Copy Markdown

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 100 out of 104 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Generated file

Comment thread test/EFCore.Tests/Storage/Json/JsonReaderDataTest.cs
Copilot AI review requested due to automatic review settings July 8, 2026 11:27

Copilot AI left a comment

Copy link
Copy Markdown

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 100 out of 104 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Generated file

Comment thread test/EFCore.Cosmos.FunctionalTests/Query/AdHocJsonQueryCosmosTest.cs Outdated
Copilot AI review requested due to automatic review settings July 8, 2026 13:01

Copilot AI left a comment

Copy link
Copy Markdown

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 100 out of 104 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • src/EFCore.Cosmos/Properties/CosmosStrings.Designer.cs: Generated file

Comment thread src/EFCore.Cosmos/Extensions/Internal/CosmosResponseStreamHelper.cs
Comment on lines +155 to +161
// @TODO: Ask if there is a better way for this..
// We need to update projection bindings in the subquery shaper to relate to a query expression that can actually provide the correct binding information.
// Then we are able to use the projection binding's query expression directly in ShaperProcessingVisitor, instead of storing the select expression separately there
// This is needed because cosmos can have projections which are subqueries, and those subqueries can have their own projections
// This appears to be needed because ShapedQueryExpression doesn't properly replace projection bindings their query expression when updating the shaper expression.
// But fixing that causes a lot of errors (in other providers?).
new ProjectionBindingQueryProjectionApplyingExpressionVisitor().Visit(subquery.ShaperExpression);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndriySvyryd I forgot to point this out
The current ShaperProcessingVisitor for cosmos will directly use the ProjectionBindingExpression.QueryExpression to find the ProjectionExpression for a ProjectionBindingExpression. This is to be able to generate shapers for subqueries in projections. This is different from relational where only the SelectExpression from the final ShapedQueryExpression is used, but relational doesn't have subqueries in projections AFAIK.
What I found odd is that I don't think this code would be needed if ShapedQueryExpression.UpdateQueryExpression actually replaced the ProjectionBindingExpression.QueryExpression of any ProjectionBindingExpressions it found in the ShaperExpression, which it does look like it intends to. However, fixing that did cause a lot of errors in other providers.

Do you find this current fix acceptable?

@JoasE JoasE requested a review from AndriySvyryd July 8, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cosmos: Materializer rounds when deserializing floating point numbers as fixed point numbers

3 participants