Subpartitioning: Adds SDK changes to support subpartitioning. #18503

SrinikhilReddy · 2021-01-07T21:24:16Z

SDK Changes to support Subpartitioning.

Adds the new PartitionKind 'MultiHash' which accepts multiple Partition keys.
Operations currently supported
1. Document/Container CRUD operations with MultiHash Partition Kind.

moderakh

a few comments:

we should consider using a builder pattern for validating that all the partition key components are primitive. Could you check what DotNet v3 SDK is doing?

We should add tests for the following scenarios:
Query:

Single Partition Query when you specify the partition-key value in the query text
Single Partition Query when you specify the partition-key value in the query option.
cross partition query when you specify the partial partition-key value prefix in the query text
cross partition query when you specify the partial partition-key value prefix in the query option
Batch
Bulk

You should run the full CIs against live endpoint.

For testing as we are bumping the API version we should run the full CTL run.
Could you sync with Naveen on running the CTL

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java

moderakh · 2021-01-13T03:31:25Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKey.java

+     * @param keys the value of partition keys.
+     */
+    @SuppressWarnings("serial")
+    public PartitionKey(final Object[] keys) {


Object can be any non primitive type, however for subpartitioning we only support primitive types,

we should consider using a builder pattern to construct and validate the content instead of passing raw object array.

Could you check what DotNet v3 is doing?

Added the pattern as suggested.

moderakh · 2021-01-13T03:33:15Z

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosContainerTest.java

-import com.azure.cosmos.models.IndexingPolicy;
-import com.azure.cosmos.models.SqlQuerySpec;
-import com.azure.cosmos.models.ThroughputProperties;
+import com.azure.cosmos.models.*;


code style: we don't do wildcard import.

You can change your intellj/eclipse default behaviour not not use wildcard import.

see this: https://stackoverflow.com/questions/3348816/intellij-never-use-wildcard-imports

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosMultiHashTest.java

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosContainerTest.java

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosMultiHashTest.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKind.java

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosMultiHashTest.java

moderakh · 2021-01-13T03:50:26Z

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosMultiHashTest.java

+ *
+ */
+
+package com.azure.cosmos;


please run the full CI run against the live endpoints.

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosMultiHashTest.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java

moderakh · 2021-01-13T03:54:03Z

/azp run java - cosmos - tests

azure-pipelines · 2021-01-13T03:54:13Z

Azure Pipelines successfully started running 1 pipeline(s).

Co-authored-by: Mohammad Derakhshani <moderakh@users.noreply.github.com>

moderakh · 2021-01-22T22:10:13Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java

+        this.partitionKeyValues = new ArrayList<Object>();
+    }
+
+    public PartitionKeyBuilder Add(String value)


java style lower camel case, same for the other methods here.

moderakh · 2021-01-22T22:10:42Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java

+     * Constructor. CREATE a new instance of the PartitionKeyBuilder object.
+     */
+    public PartitionKeyBuilder()
+    {


nit: java code style '{' on the same line as method, for other new methods too.

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/HttpConstants.java

kirankumarkolli · 2021-01-29T06:52:03Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java

+                break;
+                case MULTI_HASH:
+                    Object[] partitionKeyValues = new Object[partitionKeyDefinition.getPaths().size()];
+                    for(int path_iter = 0 ; path_iter < partitionKeyDefinition.getPaths().size(); path_iter++)


@moderakh any suggestions on elegant way of iteration.

it is possible to use java stream.

partitionKeyDefinition.getPaths().stream().map ....

but not much difference functional wise.

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKind.java

kirankumarkolli · 2021-01-29T06:59:12Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java

+        return this;
+    }
+
+    public PartitionKeyBuilder AddNoneValue()


Code doc please.

For None please cover it explicitly on targeted scenarios.

Null and None dis-ambiguration is very important in docs.

Added code comments..

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java

SrinikhilReddy · 2021-05-10T21:04:13Z

/azp run java - cosmos - tests

azure-pipelines · 2021-05-10T21:04:18Z

Commenter does not have sufficient privileges for PR 18503 in repo Azure/azure-sdk-for-java

kushagraThapar · 2021-05-10T21:06:04Z

/azp run java - cosmos - tests

azure-pipelines · 2021-05-10T21:06:17Z

Azure Pipelines successfully started running 1 pipeline(s).

kushagraThapar

Overall looks good to me, please get it reviewed by @moderakh before merging it in. Thanks @SrinikhilReddy for your contribution, appreciate it.

...cosmos/src/main/java/com/azure/cosmos/implementation/routing/PartitionKeyInternalHelper.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java

moderakh

please replace Document and InternalObjectNode with ObjectNode in the new test class.
Document and InternalObjectNode are internal types and as tests may be used later as sample by some customers this will be confusing to the customers.

please make sure full CI and CTL pass. after next version of SDK is released, please merge

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java

...cosmos/src/main/java/com/azure/cosmos/implementation/routing/PartitionKeyInternalHelper.java

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/util/Beta.java

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosMultiHashTest.java

SrinikhilReddy · 2021-05-12T06:07:06Z

please replace Document and InternalObjectNode with ObjectNode in the new test class.
Document and InternalObjectNode are internal types and as tests may be used later as sample by some customers this will be confusing to the customers.

please make sure full CI and CTL pass. after next version of SDK is released, please merge

Fixed as you suggested Mo, synced with naveen for CTL runs. Will ping the folks on this thread once i get a successful CTL run. Thank you!

simplynaveen20

CTL results look good

kushagraThapar · 2021-05-26T17:40:19Z

/azp run java - cosmos - tests

azure-pipelines · 2021-05-26T17:40:33Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java

kushagraThapar · 2021-06-10T15:53:13Z

/azp run java - cosmos - tests

azure-pipelines · 2021-06-10T15:53:26Z

Azure Pipelines successfully started running 1 pipeline(s).

Add dapr AppInsights connection string (Azure#18503) * Add dapr AppInsights connection string * Remove the deprecation note for now

SrinikhilReddy added 2 commits December 14, 2020 01:21

Changes to run tests for multihash collections

8ab6ed8

Changes.

d5d3b47

ghost added the Cosmos label Jan 7, 2021

SrinikhilReddy mentioned this pull request Jan 7, 2021

Subpartitioning: Refactors code to replace internal flags with preview flags for public preview Azure/azure-cosmos-dotnet-v3#1952

Merged

kushagraThapar changed the title ~~[Internal] Subpartitioning: Adds SDK changes to support subpartitioning.~~ [DO NOT MERGE] Subpartitioning: Adds SDK changes to support subpartitioning. Jan 7, 2021

SrinikhilReddy marked this pull request as ready for review January 12, 2021 17:41

SrinikhilReddy requested review from FabianMeiswinkel, kirankumarkolli, kushagraThapar, mbhaskar, milismsft, moderakh, simplynaveen20 and xinlian12 as code owners January 12, 2021 17:41

kushagraThapar changed the title ~~[DO NOT MERGE] Subpartitioning: Adds SDK changes to support subpartitioning.~~ Subpartitioning: Adds SDK changes to support subpartitioning. Jan 12, 2021

moderakh suggested changes Jan 13, 2021

View reviewed changes

moderakh reviewed Jan 13, 2021

View reviewed changes

sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/CosmosMultiHashTest.java Outdated Show resolved Hide resolved

moderakh reviewed Jan 13, 2021

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java Show resolved Hide resolved

SrinikhilReddy and others added 2 commits January 15, 2021 14:10

Parititionkey builder model as suggested by Mo.

5459104

Apply suggestions from code review

e1c3001

Co-authored-by: Mohammad Derakhshani <moderakh@users.noreply.github.com>

moderakh reviewed Jan 22, 2021

View reviewed changes

kirankumarkolli reviewed Jan 29, 2021

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/HttpConstants.java Show resolved Hide resolved

kirankumarkolli reviewed Jan 29, 2021

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKind.java Show resolved Hide resolved

kirankumarkolli reviewed Jan 29, 2021

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master'

22fe137

SrinikhilReddy requested review from kirankumarkolli and moderakh May 10, 2021 22:26

kushagraThapar approved these changes May 10, 2021

View reviewed changes

SrinikhilReddy added 2 commits May 10, 2021 20:23

Fixing code suggestions

5bf9d26

Merge remote-tracking branch 'upstream/master'

0c67ba6

moderakh approved these changes May 11, 2021

View reviewed changes

SrinikhilReddy added 2 commits May 11, 2021 23:03

Resolve code comments

00d5f8e

Merge remote-tracking branch 'upstream/master'

f008411

SrinikhilReddy closed this May 12, 2021

SrinikhilReddy reopened this May 12, 2021

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java

28926cf

simplynaveen20 approved these changes May 26, 2021

View reviewed changes

kirankumarkolli approved these changes May 26, 2021

View reviewed changes

kushagraThapar requested changes Jun 8, 2021

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/PartitionKeyBuilder.java Outdated Show resolved Hide resolved

SrinikhilReddy added 2 commits June 10, 2021 01:07

Merge remote-tracking branch 'upstream/master'

ed325f9

Fixing a test

b6e452e

kushagraThapar approved these changes Jun 10, 2021

View reviewed changes

kushagraThapar merged commit 5499f82 into Azure:master Jun 11, 2021

shubham1172 mentioned this pull request Jun 26, 2023

Azure Cosmos DB: support for hierarchical partition keys Azure/azure-sdk-for-go#21063

Closed

Subpartitioning: Adds SDK changes to support subpartitioning. #18503

Subpartitioning: Adds SDK changes to support subpartitioning. #18503

Uh oh!

Conversation

SrinikhilReddy commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SDK Changes to support Subpartitioning.

Uh oh!

moderakh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

moderakh commented Jan 13, 2021

Uh oh!

azure-pipelines bot commented Jan 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SrinikhilReddy commented May 10, 2021

Uh oh!

azure-pipelines bot commented May 10, 2021

Uh oh!

kushagraThapar commented May 10, 2021

Uh oh!

azure-pipelines bot commented May 10, 2021

Uh oh!

kushagraThapar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moderakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SrinikhilReddy commented Jan 7, 2021 •

edited

Loading

moderakh left a comment •

edited

Loading