Skip to content

Work on Cosmos primitive collections, subquery and general query infra#33895

Merged
roji merged 1 commit intodotnet:mainfrom
roji:CosmosPrimitiveCollections
Jun 5, 2024
Merged

Work on Cosmos primitive collections, subquery and general query infra#33895
roji merged 1 commit intodotnet:mainfrom
roji:CosmosPrimitiveCollections

Conversation

@roji
Copy link
Copy Markdown
Member

@roji roji commented Jun 4, 2024

This is the 1st PR doing work on the Cosmos pipeline; the main goal here is to support primitive collections, but this also brings in lots of other necessary query infra, modernizes the pipeline and aligns it with latest best practices and relational. Subsequent PRs should be smaller/more incremental.

This PR leaves certain things unimplemented which I intend to work on very soon (to avoid one huge PR), not everything is tracked via issues (but I have my own list etc.).

Implements #25701 for primitive collections
Implements #25700 for primitive collections
Largely implements #25765
Fixes #33858

Implements dotnet#25701 for primitive collections
Implements dotnet#25700 for primitive collections
Largely implements dotnet#25765
Fixes dotnet#33858
@roji roji requested a review from a team June 4, 2024 10:50
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
protected override bool ShouldConvertToInlineQueryRoot(Expression expression)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At some point we should consider switching the default for primitive collections. This would be a breaking change for existing non-relational providers, which would have to opt out.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't seem like it would provide enough value to justify the braking change


// TODO: Should the type be an array, or enumerable/queryable?
var arrayClrType = projection.Type.MakeArrayType();
// TODO: Temporary hack - need to perform proper derivation of the array type mapping from the element (e.g. for
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ajcvickers we need to implement getting the array type mapping from its element, including value converters etc. (like in relational)

{
// TODO: the below immediately wraps the JSON array property in a subquery (SELECT VALUE i FROM i IN c.Array).
// TODO: This isn't strictly necessary, as c.Array can be referenced directly; however, that would mean producing a
// TODO: ShapedQueryExpression that doesn't wrap a SelectExpression, but rather a KeyAccessExpression directly; this isn't currently
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ajcvickers note yet another case where SelectExpression gets in the way

&& (convertedType == null
|| convertedType.IsAssignableFrom(ese.Type)))
{
// TODO: Subquery projecting out an entity/structural type
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Scalar subqueries are implemented for primitives, but not yet for structural types - that's coming.

private const string RootAlias = "c";

private IDictionary<ProjectionMember, Expression> _projectionMapping = new Dictionary<ProjectionMember, Expression>();
private readonly List<SourceExpression> _sources = [];
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This starts to add infra for multiple sources in a Cosmos query (e.g. joins), but at this point we only actually support a single one (enforced in CosmosQuerySqlGenerator)

private CoreTypeMapping? FindCollectionMapping(in TypeMappingInfo mappingInfo)
{
var clrType = mappingInfo.ClrType!;
var elementMapping = mappingInfo.ElementTypeMapping;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ajcvickers this is the type mapping side of the primitive collection support - first the new EF 8.0 primitive collection thing, then the Cosmos-specific dictionary mapping support.

AssertSql(
"""
SELECT VALUE {"Value" : ((c["Discriminator"] = "Kiwi") ? c["FoundOn"] : 0)}
SELECT VALUE
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Did some SQL style changes: when we project using JSON object style, we break lines. When there's only one property being projected, we prefer "x AS y" and not object style, but in some cases we still must use object style (AS style isn't supported with "Value").

Copy link
Copy Markdown
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

Add issue #s to TODO's

@roji
Copy link
Copy Markdown
Member Author

roji commented Jun 5, 2024

Thanks for the review @AndriySvyryd (it's a good way to also shame me into reviewing #33747 ;)).

@ajcvickers @cincuranet @maumar I'll go ahead and merge to not block progress, but definitely take a look if you have a moment - I can correct problems in further PRs.

Add issue #s to TODO's

I'm going to be continuing work on all this part in the coming days - I've got my personal list to track the work, would rather not span the repo with micro-issues etc...

@roji roji merged commit 59bf9dd into dotnet:main Jun 5, 2024
@roji roji deleted the CosmosPrimitiveCollections branch June 5, 2024 08:04
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: bad SQL generated when projecting out "Value"

2 participants