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

Add API for creating hierarchical PartitionKeys #23577

Merged
merged 26 commits into from
Nov 1, 2024
Merged

Add API for creating hierarchical PartitionKeys #23577

merged 26 commits into from
Nov 1, 2024

Conversation

Pietrrrek
Copy link
Contributor

@Pietrrrek Pietrrrek commented Oct 14, 2024 •
edited
Loading

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to module CHANGELOG.md are included.
  • MIT license headers are included in each file.

This PR adds a builder for creating PartitionKey structs with multiple keys in order to support hierarchical partitioning keys.

Related issue: #21063

@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 Oct 14, 2024
@github-actions GitHub Actions
Copy link

Thank you for your contribution @Pietrrrek! We will review the pull request and get back to you soon.

@Pietrrrek
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Boyum IT"

@Pietrrrek Pietrrrek mentioned this pull request Oct 14, 2024
Open
@Pilchie
Copy link
Member

Pilchie commented Oct 14, 2024

@simorenoh and/or @analogrelay - can you take a look at this when you get a chance?

analogrelay
analogrelay reviewed Oct 14, 2024
View reviewed changes
Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Haven't done a full review yet, but I do have an API comment that I think is worth looking at first. Would be interested what other folks who have a bit more experience with the Go SDK have to say though.

sdk/data/azcosmos/partition_key_builder.go Outdated Show resolved Hide resolved
@Pietrrrek
Copy link
Contributor Author

Pietrrrek commented Oct 17, 2024 •
edited
Loading

The coverage tests are failing, but the error message doesn't seem quite right

FAIL
coverage: 84.4% of statements
FAIL	github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos	49.481s
FAIL
Searching for coverage files in D:\a\_work\1\s
Reading config data from D:\a\_work\1\s\/eng/config.json
(data/azcosmos) Failing if the coverage is below 0.40
D:\a\_work\1\s\coverage.xml
Status: Succeeded	Coverage file: D:\a\_work\1\s\coverage.xml	 Coverage Amount: 0.8439

I'm not sure how to proceed from here, anyone that can help?

analogrelay
analogrelay requested changes Oct 17, 2024
View reviewed changes
Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

The Append methods are looking great, now we need to get the tests up and running :)

sdk/data/azcosmos/emulator_cosmos_item_test.go Outdated Show resolved Hide resolved
analogrelay
analogrelay requested changes Oct 21, 2024
View reviewed changes
Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Closing in on it! A few minor adjustments and test fixes, but I think you're close!

sdk/data/azcosmos/partition_key_definition.go Outdated Show resolved Hide resolved
sdk/data/azcosmos/partition_key_definition.go Outdated Show resolved Hide resolved
sdk/data/azcosmos/emulator_cosmos_item_test.go Outdated Show resolved Hide resolved
sdk/data/azcosmos/emulator_cosmos_item_test.go Outdated Show resolved Hide resolved
sdk/data/azcosmos/emulator_cosmos_item_test.go Outdated Show resolved Hide resolved
@Pietrrrek @analogrelay
Prefix enum options with type name
2adac98 
Co-authored-by: Ashley Stanton-Nurse <[email protected]>
analogrelay
analogrelay reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Looks like just a query syntax error left 🤞🏻 . If you're having some trouble running the tests (I believe they require the emulator which can be difficult to get running on some platforms), let me know. I'd be happy to help, or jump in and see if I can fix these tests up so we can land this!

sdk/data/azcosmos/emulator_cosmos_item_test.go Outdated Show resolved Hide resolved
sdk/data/azcosmos/emulator_cosmos_item_test.go Outdated Show resolved Hide resolved
analogrelay
analogrelay approved these changes Nov 1, 2024
View reviewed changes
Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

I went ahead and made the change to remove the dangling WHERE clauses. This seemed to fix the tests on my machine. Assuming the tests do indeed pass, this is good to go!

@analogrelay analogrelay merged commit 058b4f1 into Azure:main Nov 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhendrixMSFT jhendrixMSFT jhendrixMSFT left review comments

@analogrelay analogrelay analogrelay approved these changes

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

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

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

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

@JeffreyRichter JeffreyRichter Awaiting requested review from JeffreyRichter

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Pietrrrek @Pilchie @analogrelay @jhendrixMSFT