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

Range type should have the ability to hold both f64 and i64 fields #59

Open
inejc opened this issue Jul 12, 2023 · 1 comment
Open

Range type should have the ability to hold both f64 and i64 fields #59

inejc opened this issue Jul 12, 2023 · 1 comment

Comments

@inejc
Copy link

inejc commented Jul 12, 2023

Hi, according to the docs, it is possible to use range filtering on both float and integer payloads but the Range client type can only hold f64 optional fields, i.e.:

rust-client/src/qdrant.rs

Lines 2931 to 2940 in cd7ee0f

pub struct Range {
#[prost(double, optional, tag = "1")]
pub lt: ::core::option::Option<f64>,
#[prost(double, optional, tag = "2")]
pub gt: ::core::option::Option<f64>,
#[prost(double, optional, tag = "3")]
pub gte: ::core::option::Option<f64>,
#[prost(double, optional, tag = "4")]
pub lte: ::core::option::Option<f64>,
}

@geo-ant
Copy link

geo-ant commented Jul 26, 2023

Hey, I've taken a look at this and I'd be willing to work on this if the maintainers deem it worthwhile. From what I can see it will require changes in at least the qdrant engine as well as the rust client.

So here's what I understand (at least I think I do, but I'll gladly take hints):

What you showed is the protobuf generated Rust code which comes from the protobuf definitions. The Range structure is used in the Condition structure, which is used in the SearchPoints structure that is passed to the search points member function of the QdrantClient. We cannot unilaterally change the layout of the Range struct because its part of the data that gets passed as a remote procedure call to the qdrant engine. From what I saw in the qdrant engine, there is a conversion layer that converts this range type to segment::types::Range. This is the type that is actually used by the engine but it also only contains Option<FloatPayloadType> fields (with FloatPayloadType = f64). So one would have to extend the Fields of this type to be able to hold either i64 or f64 which is definitely possible but will touch a lot of other code.

Hence, I would only want to do this if a maintainer is fine with that. Plus I'd like to know if I missed something and everything is much simpler after all 😁

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

No branches or pull requests

2 participants