qdrant icon indicating copy to clipboard operation
qdrant copied to clipboard

Lookup: expose feature to REST and gRPC interfaces

Open coszio opened this issue 1 year ago • 3 comments

Exposes the lookup feature in the Groups API, in both REST and gRPC interfaces

Dependencies

Builds on top of #1996

TO DOs:

  • [x] Expose feature
  • [x] Write integration tests

All Submissions:

  • [x] Have you followed the guidelines in our Contributing document?
  • [x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. [x] Does your submission pass tests?
  2. [x] Have you formatted your code locally using cargo fmt command prior to submission?
  3. [x] Have you checked your code using cargo clippy command?

Changes to Core Features:

  • [x] Have you written new tests for your core changes, as applicable?
  • [x] Have you successfully ran tests with your changes locally?

coszio avatar Jun 01 '23 14:06 coszio

Could you temporarily point this PR to lookup-in-group-by instead while it is in draft, until https://github.com/qdrant/qdrant/pull/1996 is merged? That way the diff would only show new changes. Once the dependency is merged you can point to dev again and rebase (or cherry-pick if needed). I want to prevent investing time in the same code multiple times.

Not sure how you feel about that. I've done it a few times. Though the rebase process is a bit tricky I think it helps with the stacked reviews. :smile:

Too bad you can't really comment on diffs :(

timvisee avatar Jun 02 '23 07:06 timvisee

I hadn't thought of that, but it makes sense! The rebase process is something I am actively doing anyway 👍

coszio avatar Jun 02 '23 14:06 coszio

Much better now, thanks for the suggestion @timvisee

coszio avatar Jun 02 '23 14:06 coszio

@generall I agree that vectors are not needed for the default scenario. Do you approve keeping with_payload=true and with_vector=false as defaults? ~Might be a little exotic with respect of our default false on with_*'s but i~ It makes sense here

coszio avatar Jun 05 '23 21:06 coszio

After some consideration, I decided to rollback changes with multiple lookups in grpc. Sorry for taking it back and forward too much.

Functionality-wise I think it works alright!

generall avatar Jun 07 '23 08:06 generall