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

Better cast/convert support for Cosmos #35000

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ChrisJollyAU
Copy link
Contributor

Improve the support and handling of the cast/converts in Cosmos

Originally started for #34963 but after #34982 was created extended that work to more cases

VisitUnary now leaves the Convert operation in place instead of stripping it. This has 2 potential benefits:

  • Similarity between Cosmos and relational may help any efforts to create a common shared base rather than a lot of duplication of things (e.g. SqlExpressionFactory etc) in Cosmos
  • Leaving it in place allows binary expressions to work out what type mapping the Convert needs (if it doesn't have one already)
  • This is especially the case with string concatenation to an int or something like that. the int value will have a Convert to type of Object - aka we know we need a convert but we don't yet know what to convert it to. The type mapping handling of the binary expression will work it out and apply to all sides

In the QuerySqlGenerator, we effectively no-op most casts except for if we have a convert to string.

This does light up a number of math function tests that were failing - all of them due to #17246 (client evaluation exception)
Also some tests for #20677 are now passing (not all of them related to that issue)
The Cosmos tests for #34729 are now passing as well

This fixes #34982

@ChrisJollyAU ChrisJollyAU requested a review from a team as a code owner October 28, 2024 16:43
@AndriySvyryd AndriySvyryd requested a review from Copilot December 4, 2024 20:56

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 12 changed files in this pull request and generated no suggestions.

Files not reviewed (7)
  • src/EFCore.Cosmos/Query/Internal/CosmosQuerySqlGenerator.cs: Evaluated as low risk
  • src/EFCore.Cosmos/Query/Internal/Expressions/SqlUnaryExpression.cs: Evaluated as low risk
  • src/EFCore.Cosmos/Query/Internal/CosmosSqlTranslatingExpressionVisitor.cs: Evaluated as low risk
  • src/EFCore.Cosmos/Query/Internal/SqlExpressionFactory.cs: Evaluated as low risk
  • test/EFCore.Cosmos.FunctionalTests/Query/PrimitiveCollectionsQueryCosmosTest.cs: Evaluated as low risk
  • test/EFCore.Cosmos.FunctionalTests/Query/NorthwindSelectQueryCosmosTest.cs: Evaluated as low risk
  • src/EFCore.Cosmos/Query/Internal/CosmosQueryableMethodTranslatingExpressionVisitor.cs: Evaluated as low risk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cosmos: review cast/convert handling
2 participants