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

[Internal] Subpartitioning: Adds SDK changes to support subpartitioning. #1658

Merged
merged 49 commits into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
e3c29f0
Test to check for document CRUD with MultiHash Kind.
SrinikhilReddy Jun 23, 2020
4096360
Test update.
SrinikhilReddy Jun 23, 2020
ae6c6f9
Document CRUD With MultiHash
SrinikhilReddy Jun 24, 2020
a74bc39
Merge branch 'master' into users/nanarava/multihash
SrinikhilReddy Jun 24, 2020
6df7605
Revisions based on code review.
SrinikhilReddy Jun 24, 2020
9e56030
Merge branch 'users/nanarava/multihash' of https://github.com/Srinikh…
SrinikhilReddy Jun 24, 2020
98be829
Resolving comments, adding a negative test.
SrinikhilReddy Jun 25, 2020
1c87f5c
Fixed suggested in code review.
SrinikhilReddy Jun 26, 2020
64af5a6
Tidying up the comments and Error messages.
SrinikhilReddy Jun 26, 2020
80c0d59
Implementing suggested changes in PR.
SrinikhilReddy Jul 2, 2020
726eb06
PartitionKey Null/None
SrinikhilReddy Jul 7, 2020
8b70871
Correction to undefined
SrinikhilReddy Jul 7, 2020
2dc6158
Changes suggested in reviews.
SrinikhilReddy Jul 10, 2020
a106adf
Tidying up comments.
SrinikhilReddy Jul 10, 2020
186518f
Correcting the test case. And implementing review suggestions.
SrinikhilReddy Jul 15, 2020
eaefee3
Merge branch 'master' into users/nanarava/multihash
SrinikhilReddy Jul 15, 2020
c81b047
Implementing suggestions from PR review.
SrinikhilReddy Jul 16, 2020
bb157a0
Merge branch 'users/nanarava/multihash' of https://github.com/Srinikh…
SrinikhilReddy Jul 16, 2020
5574bed
Misc cleanup
SrinikhilReddy Jul 16, 2020
3a679c9
Suggestions based on the internal review meeting.
SrinikhilReddy Jul 17, 2020
cd07652
Undo one change.
SrinikhilReddy Jul 17, 2020
fac9372
Moving changes to inside a internal directive
SrinikhilReddy Jul 20, 2020
3cd5172
Saving changes.
SrinikhilReddy Jul 22, 2020
ae54275
Merge branch 'master' of https://github.com/Azure/azure-cosmos-dotnet…
SrinikhilReddy Jul 22, 2020
6718e09
Merge branch 'master' of https://github.com/Azure/azure-cosmos-dotnet…
SrinikhilReddy Jul 22, 2020
69ef4a9
Merge branch 'master' of https://github.com/Azure/azure-cosmos-dotnet…
SrinikhilReddy Jul 23, 2020
07456d9
Changes based on review.
SrinikhilReddy Jul 24, 2020
4fa3f64
Change to IReadOnlyList.
SrinikhilReddy Jul 24, 2020
d1b0299
Resolving comments.
SrinikhilReddy Jul 24, 2020
abb76e8
Merge branch 'master' of https://github.com/Azure/azure-cosmos-dotnet…
SrinikhilReddy Jul 24, 2020
32d75a2
Cleanup
SrinikhilReddy Jul 24, 2020
53214a3
Code review changes.
SrinikhilReddy Jul 24, 2020
3dc5306
update to container properties constructor.
SrinikhilReddy Jul 28, 2020
f6ba9e9
Apply suggestions from code review
SrinikhilReddy Jul 29, 2020
a874ebb
Changes suggested in PR.
SrinikhilReddy Jul 29, 2020
772e35a
Better error message.
SrinikhilReddy Jul 29, 2020
58aa136
Cleaner code comments.
SrinikhilReddy Jul 29, 2020
9c22862
throw error for no value specified case.
SrinikhilReddy Jul 29, 2020
30da3e1
Clean code.
SrinikhilReddy Jul 29, 2020
1151d8f
Emulator startup comfig add for subpartitioning.
SrinikhilReddy Jul 29, 2020
fa03413
Merge branch 'master' into users/nanarava/multihash
j82w Jul 30, 2020
740b203
Update direct version to 3.11.4
SrinikhilReddy Jul 30, 2020
e1d2a85
Merge branch 'users/nanarava/multihash' of https://github.com/Srinikh…
SrinikhilReddy Jul 30, 2020
8c8248e
Fix for the pipeline errors.
SrinikhilReddy Jul 31, 2020
a674d0c
Merge branch 'master' of https://github.com/Azure/azure-cosmos-dotnet…
SrinikhilReddy Jul 31, 2020
cd2d448
Test fix.
SrinikhilReddy Jul 31, 2020
3bebf3c
Merge branch 'master' into users/nanarava/multihash
j82w Jul 31, 2020
407eea7
Override httpversion during test.
SrinikhilReddy Jul 31, 2020
76475ed
Merge branch 'users/nanarava/multihash' of https://github.com/Srinikh…
SrinikhilReddy Jul 31, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions Microsoft.Azure.Cosmos/src/PartitionKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
namespace Microsoft.Azure.Cosmos
{
using System;
using System.Linq;
using Microsoft.Azure.Documents;
using Microsoft.Azure.Documents.Routing;

/// <summary>
Expand Down Expand Up @@ -95,6 +97,50 @@ internal PartitionKey(object value)
this.IsNone = false;
}

/// <summary>
/// Creates a new partition key value.
/// </summary>
/// <param name="value">An object of Type PartitionKeyValueList which supports multiple partition key paths.</param>
internal PartitionKey(PartitionKeyValueList value)
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved
{
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use the cstyle comment blocks. use // instead, since VS allows for easy comment and uncomment blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

* Why these checks?
* These changes are being added in the PR for SDK to support multiple paths in a partition key.
*
* Currently, when a resource does not specify a value for the PartitionKey,
* we assign a temporary value `PartitionKey.None` and later discern whether
* it is a PartitionKey.Undefined or PartitionKey.Empty based on the Collection Type.
*
* For collections with multiple path keys, absence of a partition key values is
* always treated as a PartitionKey.Undefined.
*/
if (value == null
|| value.partitionKeyValues.Count == 0
|| (value.partitionKeyValues.Count == 1 && object.ReferenceEquals(value.partitionKeyValues[0], PartitionKeyValueList.NoneType)))
{
this.InternalKey = PartitionKey.None.InternalKey;
this.IsNone = true;
}
else
{
object[] valueArray = new object[value.partitionKeyValues.Count];
int i = 0;
foreach (object val in value.partitionKeyValues)
{
if (object.ReferenceEquals(PartitionKeyValueList.NoneType, val))
{
valueArray[i++] = Undefined.Value;
}
else
{
valueArray[i++] = val;
}
}
this.InternalKey = new Documents.PartitionKey(valueArray).InternalKey;
this.IsNone = false;
}
}

/// <summary>
/// Creates a new partition key value.
/// </summary>
Expand Down
81 changes: 81 additions & 0 deletions Microsoft.Azure.Cosmos/src/PartitionKeyValueList.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
//------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
//------------------------------------------------------------
namespace Microsoft.Azure.Cosmos
{
using System;
using System.Collections;
using System.Collections.Generic;
using Microsoft.Azure.Documents;
using Microsoft.Azure.Documents.Routing;

/// <summary>
/// Represents a partition key value list in the Azure Cosmos DB service.
/// </summary>
public sealed class PartitionKeyValueList
Copy link
Member

Choose a reason for hiding this comment

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

Do List convey the intent of ordering?

Choose a reason for hiding this comment

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

Ordering is implicit and user is expected to provide the values in the order of the PartitionKeys from the definition. For eg. If the definition is [/address/state , /address/city] , the list provided ["WA", "Seattle"] here will assign the values of Pkeys in that order.

{
internal readonly IList<object> partitionKeyValues;

internal static readonly object NoneType = new object();
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Creates a new partition key value list object.
/// </summary>
public PartitionKeyValueList()
{
this.partitionKeyValues = new List<object>();
}

/// <summary>
/// Adds a partition key value of type string to the list.
/// </summary>
/// <param name="val">The value of type string to be used as partitionKey.</param>
public void Add(string val)
{
if (val == null)
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved
{
this.AddNullValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this case. If a user wants to add null, then they can call AddNullValue()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

}
else
{
this.partitionKeyValues.Add(val);
}
}

/// <summary>
/// Adds a partition key value of type double to the list.
/// </summary>
/// <param name="val">The value of type double to be used as partitionKey.</param>
public void Add(double val)
{
this.partitionKeyValues.Add(val);
}

/// <summary>
/// Adds a partition key value of type bool to the list.
/// </summary>
/// <param name="val">The value of type bool to be used as partitionKey.</param>
public void Add(bool val)
{
this.partitionKeyValues.Add(val);
}

/// <summary>
/// Adds a partition key value which is null
/// </summary>
public void AddNullValue()
{
this.partitionKeyValues.Add(null);
}

internal void AddUndefined()
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved
{
this.partitionKeyValues.Add(Undefined.Value);
}

internal void AddNoneType()
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved
{
this.partitionKeyValues.Add(NoneType);
}
}
}
100 changes: 70 additions & 30 deletions Microsoft.Azure.Cosmos/src/Resource/Container/ContainerCore.Items.cs
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,12 @@ private async Task<ResponseMessage> ExtractPartitionKeyAndProcessItemStreamAsync
// User specified PK value, no need to extract it
if (partitionKey.HasValue)
{
PartitionKeyDefinition pKeyDefinition = await this.GetPartitionKeyDefinitionAsync();
if (((PartitionKey)partitionKey).InternalKey.Components.Count != pKeyDefinition.Paths.Count)
Copy link
Contributor

Choose a reason for hiding this comment

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

This down cast is dangerous. Just use partitionKey.Value instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

and check with partitionKey.HasValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

{
throw new ArgumentException(RMResources.MissingPartitionKeyValue);
}
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved

return await this.ProcessItemStreamAsync(
partitionKey,
itemId,
Expand Down Expand Up @@ -708,7 +714,7 @@ private async Task<ResponseMessage> ProcessItemStreamAsync(

return responseMessage;
}

public override async Task<PartitionKey> GetPartitionKeyValueFromStreamAsync(
Stream stream,
CancellationToken cancellation = default(CancellationToken))
Expand All @@ -733,21 +739,17 @@ public override async Task<PartitionKey> GetPartitionKeyValueFromStreamAsync(
IJsonNavigatorNode jsonNavigatorNode = jsonNavigator.GetRootNode();
CosmosObject pathTraversal = CosmosObject.Create(jsonNavigator, jsonNavigatorNode);

string[] tokens = await this.GetPartitionKeyPathTokensAsync(cancellation);
for (int i = 0; i < tokens.Length - 1; i++)
{
if (!pathTraversal.TryGetValue(tokens[i], out pathTraversal))
{
return PartitionKey.None;
}
}
IReadOnlyList<string[]> tokenslist = await this.GetPartitionKeyPathTokensAsync(cancellation);
CosmosElement[] cosmosElementArray = new CosmosElement[tokenslist.Count];

if (!pathTraversal.TryGetValue(tokens[tokens.Length - 1], out CosmosElement partitionKeyValue))
int index = 0;
foreach (string[] tokens in tokenslist)
{
return PartitionKey.None;
CosmosElement partitionKeyValue = this.ParseTokenListForElement(pathTraversal, tokens);
cosmosElementArray[index++] = partitionKeyValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

again just use a regular for loop here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

}

return this.CosmosElementToPartitionKeyObject(partitionKeyValue);
return this.CosmosElementToPartitionKeyObject(cosmosElementArray);
}
finally
{
Expand All @@ -756,31 +758,69 @@ public override async Task<PartitionKey> GetPartitionKeyValueFromStreamAsync(
}
}

private PartitionKey CosmosElementToPartitionKeyObject(CosmosElement cosmosElement)
private CosmosElement ParseTokenListForElement(CosmosObject pathTraversal, string[] tokens)
Copy link
Contributor

Choose a reason for hiding this comment

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

this method could probably be made static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

{
// TODO: Leverage original serialization and avoid re-serialization (bug)
switch (cosmosElement.Type)
CosmosElement partitionKeyValue;

for (int i = 0; i < tokens.Length - 1; i++)
{
case CosmosElementType.String:
CosmosString cosmosString = cosmosElement as CosmosString;
return new PartitionKey(cosmosString.Value);
if (!pathTraversal.TryGetValue(tokens[i], out pathTraversal))
{
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning null is a soft contract. Convert this method to a TryGet instead, so the user knows to check for null.

}
}

case CosmosElementType.Number:
CosmosNumber cosmosNumber = cosmosElement as CosmosNumber;
double value = Number64.ToDouble(cosmosNumber.Value);
return new PartitionKey(value);
if (!pathTraversal.TryGetValue(tokens[tokens.Length - 1], out partitionKeyValue))
{
return null;
}

case CosmosElementType.Boolean:
CosmosBoolean cosmosBool = cosmosElement as CosmosBoolean;
return new PartitionKey(cosmosBool.Value);
return partitionKeyValue;
}

case CosmosElementType.Null:
return PartitionKey.Null;
private PartitionKey CosmosElementToPartitionKeyObject(CosmosElement[] cosmosElementArray)
Copy link
Contributor

Choose a reason for hiding this comment

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

make the method static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

{
PartitionKeyValueList partitionKeyValueList = new PartitionKeyValueList();

default:
throw new ArgumentException(
string.Format(CultureInfo.InvariantCulture, RMResources.UnsupportedPartitionKeyComponentValue, cosmosElement));
foreach (CosmosElement cosmosElement in cosmosElementArray)
{
if (cosmosElement == null)
{
partitionKeyValueList.AddNoneType();
}
else
{
// TODO: Leverage original serialization and avoid re-serialization (bug)
switch (cosmosElement.Type)
Copy link
Contributor

Choose a reason for hiding this comment

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

do not switch on the type. Use an ICosmosElementVisitor instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

{
case CosmosElementType.String:
CosmosString cosmosString = cosmosElement as CosmosString;
partitionKeyValueList.Add(cosmosString.Value);
break;

case CosmosElementType.Number:
CosmosNumber cosmosNumber = cosmosElement as CosmosNumber;
double value = Number64.ToDouble(cosmosNumber.Value);
partitionKeyValueList.Add(value);
break;

case CosmosElementType.Boolean:
CosmosBoolean cosmosBool = cosmosElement as CosmosBoolean;
partitionKeyValueList.Add(cosmosBool.Value);
break;

case CosmosElementType.Null:
partitionKeyValueList.AddNullValue();
break;

default:
throw new ArgumentException(
string.Format(CultureInfo.InvariantCulture, RMResources.UnsupportedPartitionKeyComponentValue, cosmosElement));
}
}
}

return new PartitionKey(partitionKeyValueList);
}

private Uri GetResourceUri(RequestOptions requestOptions, OperationType operationType, string itemId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ public override async Task<string> GetRIDAsync(CancellationToken cancellationTok
/// </summary>
/// <param name="cancellationToken"></param>
/// <returns>Returns the partition key path</returns>
public override async Task<string[]> GetPartitionKeyPathTokensAsync(CancellationToken cancellationToken = default(CancellationToken))
public override async Task<IReadOnlyList<string[]>> GetPartitionKeyPathTokensAsync(CancellationToken cancellationToken = default(CancellationToken))
{
ContainerProperties containerProperties = await this.GetCachedContainerPropertiesAsync(cancellationToken);
if (containerProperties == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ public abstract Task<ThroughputResponse> ReplaceThroughputIfExistsAsync(
public abstract Task<ContainerProperties> GetCachedContainerPropertiesAsync(
CancellationToken cancellationToken = default(CancellationToken));

public abstract Task<string[]> GetPartitionKeyPathTokensAsync(CancellationToken cancellationToken = default(CancellationToken));
public abstract Task<IReadOnlyList<string[]>> GetPartitionKeyPathTokensAsync(
CancellationToken cancellationToken = default(CancellationToken));

public abstract Task<Documents.Routing.PartitionKeyInternal> GetNonePartitionKeyValueAsync(
CancellationToken cancellationToken);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Microsoft.Azure.Cosmos
{
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using Microsoft.Azure.Documents;
using Microsoft.Azure.Documents.Routing;
Expand Down Expand Up @@ -70,7 +71,7 @@ public class ContainerProperties
[JsonProperty(PropertyName = Constants.Properties.ConflictResolutionPolicy, NullValueHandling = NullValueHandling.Ignore)]
private ConflictResolutionPolicy conflictResolutionInternal;

private string[] partitionKeyPathTokens;
private IReadOnlyList<string[]> partitionKeyPathTokens;
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use string[] use IReadOnlyList<string>, so people don't mutate your list.

private string id;

/// <summary>
Expand Down Expand Up @@ -510,7 +511,7 @@ internal ContainerProperties(string id, PartitionKeyDefinition partitionKeyDefin

internal bool HasPartitionKey => this.PartitionKey != null;

internal string[] PartitionKeyPathTokens
internal IReadOnlyList<string[]> PartitionKeyPathTokens
{
get
{
Expand All @@ -519,7 +520,7 @@ internal string[] PartitionKeyPathTokens
return this.partitionKeyPathTokens;
}

if (this.PartitionKey.Paths.Count > 1)
if (this.PartitionKey.Paths.Count > 1 && this.PartitionKey.Kind != PartitionKind.MultiHash)
SrinikhilReddy marked this conversation as resolved.
Show resolved Hide resolved
{
throw new NotImplementedException("PartitionKey extraction with composite partition keys not supported.");
}
Expand All @@ -529,7 +530,13 @@ internal string[] PartitionKeyPathTokens
throw new ArgumentOutOfRangeException($"Container {this.Id} is not partitioned");
}

this.partitionKeyPathTokens = this.PartitionKeyPath.Split(ContainerProperties.partitionKeyTokenDelimeter, StringSplitOptions.RemoveEmptyEntries);
Collection<string[]> partitionKeyPathTokensList = new Collection<string[]>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Use List

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

foreach (string s in this.PartitionKey?.Paths)
Copy link
Contributor

Choose a reason for hiding this comment

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

s can be null if this.PartitionKey is null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

Copy link
Contributor

Choose a reason for hiding this comment

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

avoid 1 letter variable names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this.

{
partitionKeyPathTokensList.Add(s.Split(ContainerProperties.partitionKeyTokenDelimeter, StringSplitOptions.RemoveEmptyEntries));
}

this.partitionKeyPathTokens = partitionKeyPathTokensList;
return this.partitionKeyPathTokens;
}
}
Expand Down Expand Up @@ -559,5 +566,6 @@ internal void ValidateRequiredProperties()
this.indexingPolicyInternal.IncludedPaths.Add(new IncludedPath() { Path = IndexingPolicy.DefaultPath });
}
}

j82w marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1115,7 +1115,7 @@ private void ValidateCreateContainerResponseContract(ContainerResponse container

Assert.IsNotNull(containerSettings.PartitionKeyPath);
Assert.IsNotNull(containerSettings.PartitionKeyPathTokens);
Assert.AreEqual(1, containerSettings.PartitionKeyPathTokens.Length);
Assert.AreEqual(1, containerSettings.PartitionKeyPathTokens[0].Length);
Assert.AreEqual("id", containerSettings.PartitionKeyPathTokens[0]);

ContainerInternal containerCore = containerResponse.Container as ContainerInlineCore;
Expand Down
Loading