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

Properly implement support for Cosmos hierarchical partition keys #34557

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

roji
Copy link
Member

@roji roji commented Aug 28, 2024

This allow partial Cosmos hierarchical partition keys to be used properly, so that if only the 1st part of a partition key is specified by the user, it is properly lifted and passed to the Cosmos SDK (we were previously doing this only if the full partition key was specified).

Implementation notes:

  1. We first do a visitation pass over the predicate to identify all JSON ID and partition key comparisons there. We do not modify the predicate at this point (we did before), since we don't yet know which partition key values are provided and which aren't.
  2. We go over the partition key properties in order, and lift up comparisons found in the predicate from the above pass to the query compilation context (they flow to the Cosmos SDK from there). The moment there's a missing value, we stop.
  3. If all JSON ID and all partition key properties have been specified, we transform to ReadItem.
  4. Otherwise, we do a second pass to remove the partition key predicates that were lifted out in step 2.

Note that this also changes the WithPartitionKey to not use params object[]; that becomes an array parameter, which means that during compilation we can't know how many values were provided, and therefore whether all values are provided (necessary to know whether we can transform to ReadItem). Instead, we just have multiple overloads for 1, 2, and 3 partition key values.

Fixes #34553

"""
SELECT VALUE c
FROM root c
WHERE (c["$type"] IN ("HierarchicalPartitionKeyEntity", "DerivedHierarchicalPartitionKeyEntity") AND c["PartitionKey3"])
""");
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we need a way of asserting the partition key that was passed with the query. Should I file an issue for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking the same thing. I verified this by debugging in and checking what actual partition key gets constructed, but we need something better.

AssertSql(
"""
SELECT VALUE c
FROM root c
WHERE ((c["$type"] = "HierarchicalPartitionKeyEntity") AND ((c["PartitionKey1"] = "PK1") AND (c["PartitionKey2"] = 1)))
Copy link
Member

Choose a reason for hiding this comment

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

I really should have caught this when I was working on these tests and looking at the baselines!

Copy link
Member

Choose a reason for hiding this comment

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

At least we found it before shipping. :-D

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I should have caught this while writing all this :/

? source.Provider.CreateQuery<TEntity>(
Expression.Call(
instance: null,
method: WithPartitionKeyMethodInfo1.MakeGenericMethod(typeof(TEntity)),
Copy link
Member

@AndriySvyryd AndriySvyryd Aug 28, 2024

Choose a reason for hiding this comment

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

Consider removing the MakeGenericMethod by leveraging the compiler.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

You're referring to this pattern, right?

That's a good suggestion... Currently all of our operators work with MakeGenericMethod, so I propose to do this in a big pass for 10... Opened #34559.

@roji roji enabled auto-merge (squash) August 28, 2024 20:37
@roji roji merged commit a72420c into dotnet:main Aug 28, 2024
7 checks passed
@roji roji deleted the CosmosHierarchical branch August 28, 2024 22:03
roji added a commit to roji/efcore that referenced this pull request Aug 28, 2024
roji added a commit that referenced this pull request Aug 29, 2024
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 query does not extract and use partial hierarchical partition key
3 participants