Skip to content

[Cosmos] fix partition_key type in create container method - #34795

Closed
Hussein Awala (hussein-awala) wants to merge 1 commit into
Azure:mainfrom
hussein-awala:cosmostyping
Closed

Hussein Awala (hussein-awala) wants to merge 1 commit into
Azure:mainfrom
hussein-awala:cosmostyping

Conversation

@hussein-awala

@hussein-awala Hussein Awala (hussein-awala) commented Mar 16, 2024 •

Copy link
Copy Markdown

Description

This PR updates the type hint of partition_key param in the methods create_container and replace_container by making it optional, where None is an accepted value to create nonpartitioned containers.

related: apache/airflow#38196
related: #33341

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@github-actions github-actions Bot added Community Contribution Community members are working on the issue Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Mar 16, 2024
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution Hussein Awala (@hussein-awala)! We will review the pull request and get back to you soon.

@hussein-awala

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@annatisch

Copy link
Copy Markdown
Member

Thanks so much for this Hussein Awala (@hussein-awala) - changes look good to me, but adding Simon Moreno (@simorenoh) and bambriz to confirm from the service team.

@simorenoh

Copy link
Copy Markdown
Member

hi Hussein Awala (@hussein-awala) - I'm not entirely positive on the mypy issue that is present for you folks at apache airflow, but these are not changes that we can do on our end. We have not allowed for the creation of non-partitioned containers for years now, and the current improvements we have made to our typing within the Cosmos SDK are relevant and up to date with the behavior we have for the client and for the underlying service. Since the beginning of the async client, this has been marked as a non-optional field for the relevant methods:
image
PR in case you're curious: https://github.com/Azure/azure-sdk-for-python/pull/21404/files#diff-a274a8107879de7dace0a296bd1f57274e12e22a764ed77c0c0579c275980e1c

And that is for good reason - like I mentioned, this is not behavior we have allowed for several years now and users should be seeing an error when attempting to use None for a partition key definition:
image

As such, I'll be closing this PR.

@hussein-awala

Copy link
Copy Markdown
Author

We have not allowed for the creation of non-partitioned containers for years now

This seems incorrect, the method still accepts None which is handled by the method without raising an exception:

if partition_key is not None:
definition["partitionKey"] = partition_key

if partition_key is not None:
definition["partitionKey"] = partition_key

And from Cosmos documentation:

Azure Cosmos DB supports creating containers without a partition key. Currently you can create nonpartitioned containers by using Azure CLI and Azure Cosmos DB SDKs (.NET, Java, NodeJs) that have a version less than or equal to 2.x. You can't create nonpartitioned containers using the Azure portal. However, such nonpartitioned containers aren’t elastic and have fixed storage capacity of 20 GB and throughput limit of 10K RU/s.

@simorenoh

Simon Moreno (simorenoh) commented Mar 18, 2024 •

Copy link
Copy Markdown
Member

Hussein Awala (@hussein-awala) I see what you mean - while that may look like redundant code, we actually have it there on purpose to ensure we don't fill out the container definition with a None partition key. Reason for that being that the error message returned by the service is a lot more informative that way. Compare the one I sent before vs. this one:
image

You also only included part of that page on documentation for migrating from non-partitioned collections (which are even marked as legacy in that same document) to newer partitioned ones. It is also mentioned there that the behavior is only available for versions <= 2.x.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community Contribution Community members are working on the issue Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants