Always convert to parameter query roots in relational #30966
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roji
changed the title
roji
force-pushed
the
AlwaysConvertToQueryRoot
branch
2 times, most recently
from
59a5170 to
7b17a5f
Compare
roji
force-pushed
the
AlwaysConvertToQueryRoot
branch
from
7b17a5f to
b54e60a
Compare
|
@maumar FYI rebased on latest main |
|
@maumar I think this may intersect with some work @ajcvickers has planned (on metadata/type mapping of primitive collections), so it may be good to get this merged quickly before conflicts arise. |
maumar
reviewed
Jul 11, 2023
| @@ -126,6 +126,9 @@ | |||
| <data name="CannotProduceUnterminatedSQLWithComments" xml:space="preserve"> | |||
| <value>Can't produce unterminated SQL with comments when generating migrations SQL for {operation}.</value> | |||
| </data> | |||
| <data name="CompatibilityLevelTooLowForScalarCollections" xml:space="preserve"> | |||
| <value>EF Core's SQL Server compatibility level is set to {compatibilityLevel}; compatibility level 130 (SQL Server 2016) is the minimum for most forms of querying of JSON arrays.</value> | |||
There was a problem hiding this comment.
nit: did we decide to mention compatibility level explicitly in exception messages? Either way, we should maybe unify with JsonValuePathExpressionsNotSupported
There was a problem hiding this comment.
Yeah, I think it's useful to mention the required and current compatibility level in the exceptions we throw... This way users know right away what's going on.
Changed JsonValuePathExpressionsNotSupported to also include the compatibility level, check out the added commit.
maumar
approved these changes
Jul 11, 2023
roji
force-pushed
the
AlwaysConvertToQueryRoot
branch
from
b54e60a to
62bf50a
Compare
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation of primitive collections requires that providers opt into parameter query roots in preprocessing (inline query roots are opted into at the relational level). This was done in order to support legacy modes where e.g. OPENJSON isn't supported, and so we get just a normal parameter with Enumerable Contains instead of ParameterQueryRootExpression with Queryable Contains, and the legacy translation kicks in (IN with constants).
With this PR, we now always get a ParameterQueryRootExpression. If the provider doesn't override RelationalQueryableMethodTranslatingExpressionVisitor.TranslateCollection (e.g. to implement OPENJSON), there's now a fallback path in VisitMethodCall that identifies Contains and generates the legacy InExpression. Note that this requires changing error handling in QueryableMethodTranslatingEV: we can no longer throw immediately if the source can't be translated, to allow this fallback translation to work.
This also allows us to always set ElementTypeMapping on the type mapping returned by the type mapping source; we previously didn't set it for old SQL Server, since that controlled whether nav expansion interpreted a property as queryable or not.
After this PR, the only place which cares about whether the database supports OPENJSON is RelationalQueryableMethodTranslatingExpressionVisitor.TranslateCollection, and providers no longer need to mess around in preprocessing.
Note that at the non-relational level, inline and parameter query roots still aren't generated by default, since that would require fixing up Cosmos and InMemory. We can do that later.
Note: this PR is based on top of #30956, review the 2nd commit only.