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.

Sign up for GitHub

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

Jump to bottom

Partitionkey add methods for comparison #1265

Merged
merged 9 commits into from
Mar 18, 2020
Merged

Partitionkey add methods for comparison #1265

merged 9 commits into from
Mar 18, 2020

Conversation

KristianJakubik
Copy link
Contributor

Description

This PR adds methods and operators for basic comparison to the PartitionKey struct.

Type of change

  • New feature (non-breaking change which adds functionality)

Closing issues

closes #1233

@KristianJakubik KristianJakubik changed the title Implements comparison operators, methods Partitionkey add methods for comparison Mar 8, 2020
j82w
j82w previously approved these changes Mar 9, 2020
View reviewed changes
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@j82w
Copy link
Contributor

j82w commented Mar 9, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

j82w
j82w reviewed Mar 9, 2020
View reviewed changes
changelog.md
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- [#1233](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1265) PartitionKey now supports operators ==, != for equality comparison.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [#1233](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1265) PartitionKey now supports operators ==, != for equality comparison.
- [#1233](https://github.com/Azure/azure-cosmos-dotnet-v3/pull/1265) PartitionKey now supports operators ==, != for equality comparison (Thanks to KristianJakubik).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't need to be mentioned in release notes 😄.

Copy link
Contributor

Choose a reason for hiding this comment

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

We really appreciate the contribution. There are other people mentioned in the changelog, but I'll leave it up to you.

@j82w
Copy link
Contributor

j82w commented Mar 13, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

j82w
j82w previously approved these changes Mar 16, 2020
View reviewed changes
ealsur
ealsur previously approved these changes Mar 16, 2020
View reviewed changes
@kirankumarkolli
Copy link
Member

What are the scenarios for IComparable?

@KristianJakubik
Copy link
Contributor Author

What are the scenarios for IComparable?

Well I was thinking about PartitionKey as a wrapper around PartitionKeyInternal, and PartitionKeyInternal implements IComparable. So, I think the scenario is the same as for PartitionKeyInternal. Anyway the implementation is just recalling the PartionKeyInternal implementation.

@j82w
Copy link
Contributor

j82w commented Mar 18, 2020

What are the scenarios for IComparable?

Well I was thinking about PartitionKey as a wrapper around PartitionKeyInternal, and PartitionKeyInternal implements IComparable. So, I think the scenario is the same as for PartitionKeyInternal. Anyway the implementation is just recalling the PartionKeyInternal implementation.

I think we should remove IComparable until there is a scenario for it. Adding the ability to sort partition keys based on some internal logic seems like bad design that will likely cause issues in the future. PartitionKeyInternal comparing logic might not work the way most users expect it to and it might change in the future which would cause a breaking change.

@KristianJakubik KristianJakubik dismissed stale reviews from ealsur and j82w via 4f0690c March 18, 2020 14:20
@KristianJakubik
Copy link
Contributor Author

What are the scenarios for IComparable?

Done

@j82w
Copy link
Contributor

j82w commented Mar 18, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@j82w
Copy link
Contributor

j82w commented Mar 18, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@j82w j82w merged commit f2e9d97 into Azure:master Mar 18, 2020
@j82w j82w mentioned this pull request Mar 26, 2020
Closed
ealsur pushed a commit that referenced this pull request Apr 23, 2020
@KristianJakubik @ealsur
Partitionkey add methods for comparison (#1265)
399c5af 
* Implements comparrision operators, methods

* Add test for partition key comparison

* changelog

* Refactor Equals Method

* Update SDKAPI

* Refactor tests

* Sort TypeTree by InvariantComparer

* Remove IComparable form PartitionKey

* Update DotNetSDKAPI.json
@ealsur ealsur mentioned this pull request Apr 23, 2020
Merged
1 task
kirankumarkolli pushed a commit that referenced this pull request Apr 27, 2020
@ealsur @KristianJakubik
[vNext] PartitionKey: Add IEquatable support (#1423)
167faf0 
* Partitionkey add methods for comparison (#1265)

* Implements comparrision operators, methods

* Add test for partition key comparison

* changelog

* Refactor Equals Method

* Update SDKAPI

* Refactor tests

* Sort TypeTree by InvariantComparer

* Remove IComparable form PartitionKey

* Update DotNetSDKAPI.json

* undo

* Compare case for default(PartitionKey) (#1312)

* default case

* changelog

* tests

* contract

Co-authored-by: Kristián Jakubík <jakubikkristian@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bchong95 bchong95 bchong95 left review comments

@kirankumarkolli kirankumarkolli kirankumarkolli approved these changes

@j82w j82w j82w approved these changes

@ealsur ealsur ealsur left review comments

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PartitionKey is missing methods for comparison
5 participants
@KristianJakubik @j82w @kirankumarkolli @ealsur @bchong95