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

Update v1.7.0 - implementation #81

Merged
merged 11 commits into from
Dec 7, 2023
Merged

Update v1.7.0 - implementation #81

merged 11 commits into from
Dec 7, 2023

Conversation

timvisee
Copy link
Member

@timvisee timvisee commented Dec 4, 2023

Tasks

  • create_shard_key
  • delete_shard_key
  • discover
  • discover_batch
  • Sparse vectors
  • Manhattan distance
  • shard_key usage in API
  • Improve conversions: From<Vec<(_, _)>> ➡️ From<&[(_, _)]> because we construct new Vecs anyway

Tasks after merging

  • Update Rust examples in documentation, add shard key selection

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
  3. Have you checked your code using cargo clippy --all --all-features command?

@@ -663,6 +687,58 @@ impl QdrantClient {
.await?)
}

pub async fn create_shard_key(
&self,
collection_name: impl AsRef<str>,
Copy link
Member Author

Choose a reason for hiding this comment

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

We use impl ToString in a lot of other places, but I think impl AsRef<str> is a lot better in this case. Since this is a new function, I choose to use this better one here.

We could also remove all function parameters and imply provide a CreateShardKeyRequest. Not sure which is better.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you think AsRef is better in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I already answered this, but somehow my answer is missing 😄

Anyway, in this case: AsRef allows 1 string clone, ToString requires 2 string clones.

Comment on lines 816 to 822
pub async fn upsert_points(
&self,
collection_name: impl ToString,
shard_key_selector: Option<Vec<shard_key::Key>>,
points: Vec<PointStruct>,
ordering: Option<WriteOrdering>,
) -> Result<PointsOperationResponse> {
Copy link
Member Author

Choose a reason for hiding this comment

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

I put the shard_key_selector right after collection_name. Seems to make sense to me. But we could also add it as last parameter, any opinions?

Copy link
Member

Choose a reason for hiding this comment

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

Make another function with this parameter, to not overload default functions with arguments?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do that, but that would duplicate a whole lot of them (and we already have duplication for some blocking/non-blocking calls).

Maybe we can use some form of builder pattern in the future.

@timvisee timvisee requested review from agourlay and generall December 4, 2023 15:56
@agourlay
Copy link
Member

agourlay commented Dec 4, 2023

Looks good 👍

I am only missing the user perspective on the shard selector:

  • how does one build a shard key at the call site?
  • are there conversions from string?

@timvisee
Copy link
Member Author

timvisee commented Dec 5, 2023

  • how does one build a shard key at the call site?

Construct the Key enum, which holds a Number or Keyword variant. These are functional as soon as the user has created the shard key in the collection with that very same enum.

  • are there conversions from string?

No. Good idea.

@timvisee
Copy link
Member Author

timvisee commented Dec 5, 2023

@agourlay

These now work:

ShardKeySelector::from(1);
ShardKeySelector::from("a");
ShardKeySelector::from(vec![1, 2, 3]);
ShardKeySelector::from(vec!["a".to_string(), "b".to_string()]);
ShardKeySelector::from(vec!["a", "b"]);

@generall generall marked this pull request as ready for review December 6, 2023 20:07
@timvisee timvisee mentioned this pull request Dec 7, 2023
19 tasks
@agourlay
Copy link
Member

agourlay commented Dec 7, 2023

FYI I updated one my project to use this branch.
Intellij got very confused by the new shard key parameter and did not report syntax error.
Had to rely on cargo for the migration.

@timvisee timvisee merged commit 762703f into dev Dec 7, 2023
2 checks passed
timvisee added a commit that referenced this pull request Dec 7, 2023
* Add create and delete shard key functions

* Use more direct shard key types

* Add discovery functions

* Conversions from borrowed types into sparse vectors

* Add shard key selector parameter to all relevant API functions

* Move shard key selector parameter adjacent collection name parameter

* Update documentation example

* Add conversion helpers for shard key selector

* sync proto

* proto changes

* fix readme

---------

Co-authored-by: generall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants