-
Notifications
You must be signed in to change notification settings - Fork 494
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
[Internal] Subpartitioning: Adds SDK changes to support subpartitioning. #1658
Conversation
/// Creates a new partition key value. | ||
/// </summary> | ||
/// <param name="value">The value to use as partition key.</param> | ||
internal PartitionKey(object[] value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object has a lot of negative feedback considering it only supports a very few primitive types. One option is to add a PartitionKeyValue type that has implicit conversion for the supported primitive types.
@kirankumarkolli, @FabianMeiswinkel, and @ealsur do you have any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me consistency trumps everything else here - we used object for a single-value PK - so object[] is good enough for multiple.
I would also assume that this rarely results in issues with customers - at least I have ever heard of a customer trying to use a property that isn't string or numeric as the PK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with object is the chance of runtime exceptions if the actual type is not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea (but it can cause inception issues) is to have PartitionKey(PartitionKey[] value)
. It can technically allow a PK that is an array to be passed as parameter to another PK constructor (array in array), but it's something the user would be actively doing. I think it boils down to reducing runtime exceptions, any check we add would generate runtime exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FabianMeiswinkel we don't use object for single-value PK. We have explicit primitives on the PartitionKey constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi guys, fixed as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jake for your suggestion. It looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PartitionKeyBuilder pkBuilder = new PartitionKeyBuilder()
.Add(15232)
.Add("Pittsburgh");
await container.ReadItemAsync<Document>("Id1", pkBuilder.Build())
List<PartitionKey> pKValueList = new List<PartitionKey>(){
new PartitionKey(15232),
new PartitionKey("Pittsburgh"),
};
Cosmos.PartitionKey pKey = new Cosmos.PartitionKey(pKValueList);
await container.ReadItemAsync<Document>("Id1", pKey)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we agreed not to provide the List overload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we agreed to not provide the list overload. I posted that during the meeting to show the difference. I don't see a list overload in the code.
Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Settings/ContainerProperties.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Container/ContainerInternal.cs
Outdated
Show resolved
Hide resolved
/// Creates a new partition key value. | ||
/// </summary> | ||
/// <param name="value">The value to use as partition key.</param> | ||
internal PartitionKey(object[] value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with object is the chance of runtime exceptions if the actual type is not supported.
/// Creates a new partition key value. | ||
/// </summary> | ||
/// <param name="value">The value to use as partition key.</param> | ||
internal PartitionKey(object[] value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea (but it can cause inception issues) is to have PartitionKey(PartitionKey[] value)
. It can technically allow a PK that is an array to be passed as parameter to another PK constructor (array in array), but it's something the user would be actively doing. I think it boils down to reducing runtime exceptions, any check we add would generate runtime exceptions.
Microsoft.Azure.Cosmos/src/Resource/Settings/ContainerProperties.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Settings/ContainerProperties.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Settings/ContainerProperties.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Settings/ContainerProperties.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
e1d2a85
/azp run |
Commenter does not have sufficient privileges for PR 1658 in repo Azure/azure-cosmos-dotnet-v3 |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Closing due to in-activity, pease feel free to re-open. |
SDK Changes to support Subpartitioning.
PartitionKeyBuilder
.ContainerProperties
constructor that accepts anIReadOnlyList<string>
object as multiple partition key pathsSample usage of Collection create with MultiHash Partition kind and multiple partition key paths.
Sample Document Create, where the PartitionKey is extracted from the resource stream.
Reading back the document with the PartitionKey set during the read call.
Type of change