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

Cosmos FromSql #25525

Merged
1 commit merged into from
Aug 17, 2021
Merged

Cosmos FromSql #25525

1 commit merged into from
Aug 17, 2021

Conversation

roji
Copy link
Member

@roji roji commented Aug 15, 2021

Notes:

  • Did not implement FromSqlInterpolated for now as we're not sure we want to. Current naming is FromSqlRaw (as opposed to FromSql).
  • Cosmos has no DbParameter - parameters are added via QueryDefinition.WithParameter where the name/value are given directly. The main use of accepting DbParameter in relational is to allow configuring parameter facets (size, precision, scale), but as these concepts don't exist in Cosmos we simply don't support it. So Cosmos FromSql supports only direct values.
  • The current implementation always integrates the raw SQL as a subquery, even when it isn't composed over. This is because we currently always send SELECT c FROM root c for entity projection, which adds an additional level of nesting (with c as key), compared to SELECT * (see Cosmos: Stop nesting results in an extra JSON object #25527); this means we don't know how to materialize results from SELECT *, or projections such as SELECT c.Region, c.PostalCode (even when all the entity's properties are properly projected). Always wrapping in a subquery allows us to produce the correct nested structure; we should probably plan for producing SELECT * at some point.
    • Note also that since by default we map all DbSets to the same container with a discriminator, and we compose to filter by the discriminator, we'll (almost?) always be composing anyway (compared to relational where we only compose for discriminator for TPH sub-types).

Subqueries in Cosmos

Docs say only correlated subqueries are supported (https://docs.microsoft.com/en-us/azure/cosmos-db/sql/sql-query-subquery). However, this seems to (implicitly) refer to subqueries within JOIN:

  • Works (correlated subquery in join): SELECT Count(1) FROM c JOIN (SELECT VALUE t FROM t IN c.Tags)
  • Does not work (non-correlated): SELECT Count(1) FROM c JOIN (SELECT * FROM root)
  • In other words, JOIN subqueries (like JOIN in general) are just a mechanism for projecting out from the same document (which means they must be correlated)
  • So it will never be possible to use FromSql in a JOIN (unlike in relational) - only in FROM.

Composition over subquery in FROM:

Operator Sample Northwind SQL
WHERE SELECT * FROM (SELECT * FROM root c WHERE c.Discriminator = 'Customer') c WHERE c.City = 'Berlin'
ORDER BY SELECT * FROM (SELECT * FROM root c WHERE c.Discriminator = 'Customer') c ORDER BY c.PostalCode
LIMIT / OFFSET SELECT * FROM (SELECT * FROM root c WHERE c.Discriminator = 'Customer') c OFFSET 1 LIMIT 3

Closes #17311

Thanks to @RaymondHuy for previous work in #25457

@roji roji requested review from smitpatel and maumar August 15, 2021 17:58
@roji roji force-pushed the roji/CosmosRawSql branch 2 times, most recently from 71854bf to 4343601 Compare August 16, 2021 15:42
Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Joins/SelectMany to generate a query with FromSqlRaw inside lambda is not possible perhaps write a test for EF.CompiledQuery which generates same. If parameterExtractingEV says constant values in the tree, we inline the values and don't send server parameters.

@roji
Copy link
Member Author

roji commented Aug 16, 2021

Joins/SelectMany to generate a query with FromSqlRaw inside lambda is not possible perhaps write a test for EF.CompiledQuery which generates same. If parameterExtractingEV says constant values in the tree, we inline the values and don't send server parameters.

Relevant test is FromSqlRaw_queryable_composed_compiled_with_parameter

@roji roji requested a review from smitpatel August 16, 2021 17:47
}

default:
throw new ArgumentOutOfRangeException();
Copy link
Member

Choose a reason for hiding this comment

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

InvalidOperationException with the argument tree printed

Copy link
Member Author

Choose a reason for hiding this comment

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

Was trying to avoid a new string :trollface:

Copy link
Member

Choose a reason for hiding this comment

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

Just print out the expression tree. No need of exception message or resources.

@AndriySvyryd
Copy link
Member

Cosmos has no DbParameter - parameters are added via QueryDefinition.WithParameter where the name/value are given directly. The main use of accepting DbParameter in relational is to allow configuring parameter facets (size, precision, scale), but as these concepts don't exist in Cosmos we simply don't support it. So Cosmos FromSql supports only direct values.

We should create an issue for adding support for parameters. While they doesn't offer new functionality they are more convenient than manually seriliazing values to json, especially with #17306

@ghost
Copy link

ghost commented Aug 16, 2021

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@roji
Copy link
Member Author

roji commented Aug 16, 2021

We should create an issue for adding support for parameters. While they doesn't offer new functionality they are more convenient than manually seriliazing values to json, especially with #17306

@AndriySvyryd can you open that issue? I'm not sure exactly what you have in mind...

@AndriySvyryd
Copy link
Member

@AndriySvyryd can you open that issue? I'm not sure exactly what you have in mind...

Perhaps it just works, added a note to #17306

@ghost
Copy link

ghost commented Aug 16, 2021

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

  1. Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this
From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

@ghost ghost merged commit c3e2340 into main Aug 17, 2021
@ghost ghost deleted the roji/CosmosRawSql branch August 17, 2021 00:17
This pull request was closed.
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: FromSql support
3 participants