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

[FEATURE] Add knn builder and query search support #547

Closed
adrian-arapiles opened this issue Nov 16, 2022 · 23 comments
Closed

[FEATURE] Add knn builder and query search support #547

adrian-arapiles opened this issue Nov 16, 2022 · 23 comments
Assignees
Labels
enhancement New feature or request

Comments

@adrian-arapiles
Copy link

Is your feature request related to a problem?

It's not possible to build a search query using knn. With old java rest high client we can copy and reuse code like that thread cover https://forum.opensearch.org/t/k-nn-query-from-es-high-level-client/3155

What solution would you like?

Get classes for create and use query builders for search.

What alternatives have you considered?

At the moment I will tried to parse a json for raw search request.

Do you have any additional context?

@adrian-arapiles adrian-arapiles added enhancement New feature or request untriaged labels Nov 16, 2022
@wbeckler
Copy link

wbeckler commented Nov 16, 2022 via email

@adrian-arapiles
Copy link
Author

Hello, I could make a PR if you tech me how to do.
I already fork the repo from main branch, but I face issues with missmatch httpclient version from 4 to 5.
In version 2.4.0 of opensearch-rest-client uses major 4 of httpclient but in main branch already using classes from major 5 of httpclient.
I have ready class and changes for knn search support. The build and tests passed.

Thanks.

@reta
Copy link
Collaborator

reta commented Nov 17, 2022

@adrian-arapiles that's correct, 3.0 has moved to Apache HttpClient 5, whereas versions below stay on Apache HttpClient 4

@reta
Copy link
Collaborator

reta commented Nov 17, 2022

On the general note, the k-NN functionality is provided by the plugin (https://github.com/opensearch-project/k-NN), so we probably should not bake it into core client (since the plugin may not be always available) but make it part of the k-NN plugin, similarly to percolator (https://github.com/opensearch-project/OpenSearch/tree/main/modules/percolator) and others (in fact, it is there already https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java)

@adrian-arapiles
Copy link
Author

@reta

On the general note, the k-NN functionality is provided by the plugin (https://github.com/opensearch-project/k-NN), so we probably should not bake it into core client (since the plugin may not be always available) but make it part of the k-NN plugin, similarly to percolator (https://github.com/opensearch-project/OpenSearch/tree/main/modules/percolator) and others (in fact, it is there already https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java)

That's true, the knn is a plugin functionality. The code that you share about KnnQueryBuilder is the class that I use until now with old High Rest Client but wont work with new builders java client pattern.
I didn't find any way to import knn libraries in mvn repositories.
By other hand there isn't a way to create a query from json (for example) to workaround this situation.
If you know any idea, please tell me.
Right now the problem is I can't make a knn query with this new client using builder pattern.
In the case that do you want to I share the code for knn builders, tell me I have ready.

Is there any possibility to add support to that for version 2.x until any definitively solution?

Thanks.

@reta
Copy link
Collaborator

reta commented Nov 17, 2022

Thanks @adrian-arapiles

@reta

On the general note, the k-NN functionality is provided by the plugin (https://github.com/opensearch-project/k-NN), so we probably should not bake it into core client (since the plugin may not be always available) but make it part of the k-NN plugin, similarly to percolator (https://github.com/opensearch-project/OpenSearch/tree/main/modules/percolator) and others (in fact, it is there already https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java)

That's true, the knn is a plugin functionality. The code that you share about KnnQueryBuilder is the class that I use until now with old High Rest Client but wont work with new builders java client pattern.

Could you please elaborate more on that (may be some examples)? Because the original discussion thread has only JSON

I didn't find any way to import knn libraries in mvn repositories.

That has to be fixed by publishing client libraries (for k-NN plugin)

By other hand there isn't a way to create a query from json (for example) to workaround this situation. If you know any idea, please tell me. Right now the problem is I can't make a knn query with this new client using builder pattern. In the case that do you want to I share the code for knn builders, tell me I have ready.

Is my understanding correct that if KnnQueryBuilder (from k-NN plugin) is available, it could be used with new client using builder pattern? Or I missing something

Is there any possibility to add support to that for version 2.x until any definitively solution?

The publishing k-NN plugin client libraries for 2.x should be straightforward. I am not the one making the decision, but adding any temporary API is not a good way to solve this particular problem (in my opinion), once public it could not be easily removed.

Thanks.

@adrian-arapiles
Copy link
Author

Thanks @reta

Could you please elaborate more on that (may be some examples)? Because the original discussion thread has only JSON

Yes, sorry. Right now, I am using KnnQueryBuilder copied from knn repo.
So my code looks like that:

...
            // Multimedias is a object with a list of vectors. I'm doing a multisearch with individual search of knn vectors.
            for (Multimedia multimedia : property.multimedias()) {
                KnnQueryBuilder knnQueryBuilder = new KnnQueryBuilder("my-field", multimedia.getPhashVector(), 2);
                BoolQueryBuilder query = QueryBuilders.boolQuery()
                    .must(knnQueryBuilder));

                SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
                searchSourceBuilder.query(query);
                searchSourceBuilder.minScore(0.96f);
                searchSourceBuilder.size(1000);
                SearchRequest searchRequest = new SearchRequest("my-index-name");
                searchRequest.source(searchSourceBuilder);
                multiSearchRequest.add(searchRequest);
            }

            MultiSearchResponse searchResults = openSearchClient.multiSearch(multiSearchRequest);
...

So, if I'm not wrong, this code works fine with java high rest client but in new java client that uses different classes, builder pattern, etc it won't work.
Maybe I'm missing something but I didn't find any method to use raw JSON as forum mentioned or how to reuse code from KnnQueryBuilder in new java client.

That has to be fixed by publishing client libraries (for k-NN plugin)

Okay, do you have any issue following that or idea about when this gonna be released? Can I help with that?

Is my understanding correct that if KnnQueryBuilder (from k-NN plugin) is available, it could be used with new client using builder pattern? Or I missing something

I'm not sure about that. I didn't found any way to do that. If you have any clue I gonna investigate and test it.
Anyways I have class KnnQueryBuilder using new builder pattern available to share with you.
b8016c1

The publishing k-NN plugin client libraries for 2.x should be straightforward. I am not the one making the decision, but adding any temporary API is not a good way to solve this particular problem (in my opinion), once public it could not be easily removed.

I agree with that. Maybe could add support to build search from raw json? I think its a good feature and it works as a workaround until knn libraries are released.

Thanks.

@reta
Copy link
Collaborator

reta commented Nov 18, 2022

thanks @adrian-arapiles, I think we could transfer that to https://github.com/opensearch-project/k-NN plugin, the client APIs should be published and should be usable with all clients. @dblock could you help please to transfer this issue? (it seems like I cannot, but I could copy / paste), thanks

@dblock dblock transferred this issue from opensearch-project/opensearch-java Nov 28, 2022
@navneet1v
Copy link

Adding @vamshin for prioritization.

@jmazanec15
Copy link
Member

Seems it would be best to look into adding plugins to opensearch-api-specification: opensearch-project/opensearch-clients#19.

There appears to be an issue already tracking this: opensearch-project/opensearch-api-specification#23.

@SeyedAlirezaFatemi
Copy link

Right now, I am using KnnQueryBuilder copied from knn repo.

@adrian-arapiles , How exactly are you doing this? I'm asking since I'm not able to add org.opensearch.plugin:opensearch-knn as a dependent and import from org.opensearch.knn. I'm also using the Java high-level REST client.

@adrian-arapiles
Copy link
Author

Right now, I am using KnnQueryBuilder copied from knn repo.

@adrian-arapiles , How exactly are you doing this? I'm asking since I'm not able to add org.opensearch.plugin:opensearch-knn as a dependent and import from org.opensearch.knn. I'm also using the Java high-level REST client.

I just created a KnnQueryBuilder class in my project and copied code from the class. https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java

Then I use as another QueryBuilder. For example:

float [] value = ...;
int k = 2;
KnnQueryBuilder knnQueryBuilder = new KnnQueryBuilder("field", value, k);
BoolQueryBuilder query = QueryBuilders.boolQuery().must(knnQueryBuilder);

I hope that I explained well. The problems is that you can't get as dependency this class, so I replicated the class in my repo.

@SeyedAlirezaFatemi
Copy link

I just created a KnnQueryBuilder class in my project and copied code from the class. https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java

My problem is that that class has a lot of dependencies on other classes such as org.opensearch.knn.index.KNNMethodContext, org.opensearch.knn.index.mapper.KNNVectorFieldMapper, org.opensearch.knn.index.util.KNNEngine. So do you also copy all those classes to your repo?

@adrian-arapiles
Copy link
Author

I just created a KnnQueryBuilder class in my project and copied code from the class. https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/index/query/KNNQueryBuilder.java

My problem is that that class has a lot of dependencies on other classes such as org.opensearch.knn.index.KNNMethodContext, org.opensearch.knn.index.mapper.KNNVectorFieldMapper, org.opensearch.knn.index.util.KNNEngine. So do you also copy all those classes to your repo?

Hi,
Sorry about my late.
I only copied 2 classes, the rest come from opensearch dependencies from maven (in my case).
KnnQueryBuilder.java
KnnQuery.java

@wbeckler
Copy link

wbeckler commented Jun 8, 2023

Just a heads up that the opensearch-java client has an open PR to add knn support:
#524

@navneet1v
Copy link

@wbeckler thanks.. @SeyedAlirezaFatemi @adrian-arapiles

@dblock
Copy link
Member

dblock commented Jun 26, 2023

Is this the same as #539 and belongs in the opensearch-java project? If so, let's move this issue there and close the above as dup?

@navneet1v
Copy link

@dblock the issues seems to be same. @wbeckler can provide more details. But the idea was k-NN query builder was not supported by any opensearch client, and in this issue customer are specifically for k-NN queryBuilder in java client.

@wbeckler
Copy link

I am moving this issue back to the opensearch-java client, as it's still a need and that's where the work would go.

@wbeckler wbeckler transferred this issue from opensearch-project/k-NN Jun 30, 2023
@VachaShah
Copy link
Collaborator

kNN support was added in opensearch-java 2.6.0.

@Dharin-shah
Copy link

Dharin-shah commented Nov 22, 2023

Why is this closed, the specific usecase to build a knn query along with other functions is missing? can someone please help here, i tried copying the code, but i guess there are too many depenedencies that needs to be copied too
@SeyedAlirezaFatemi @adrian-arapiles, did the copy help?

My use case is similar

private QueryBuilder getFunctionScoreWithSemanticSearchAndCustomScoreScript(QueryVectorParams queryVectorParams) {
        FullTextQuery currentFullTextQuery = fullTextQuery;
        AbstractQueryBuilder fulltextQuery = currentFullTextQuery.buildQuery().orElseGet(QueryBuilders::matchAllQuery);
        KnnFloatVectorQuery knnQuery = new KnnFloatVectorQuery(queryVectorParams.getModelId(),
            queryVectorParams.getParams(), 500);
        BoolQueryBuilder combinedQuery = QueryBuilders.boolQuery();
        combinedQuery.should(fulltextQuery);

        //combinedQuery.should(knnQuery); //doesnt work
        return FunctionScore.buildWithCustomRankingScript(fulltextQuery, queryVectorParams.getModelId(),
            queryVectorParams.getQueryVector(), queryVectorParams.getParams());
   }

@dblock
Copy link
Member

dblock commented Nov 22, 2023

@Dharin-shah Looks like this wasn't complete support. Open a new issue with details of all the things you know are missing?

@Dharin-shah
Copy link

Dharin-shah commented Nov 23, 2023

import java.io.IOException;
import org.apache.lucene.search.KnnFloatVectorQuery;
import org.apache.lucene.search.Query;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.index.query.AbstractQueryBuilder;
import org.opensearch.index.query.QueryShardContext;

public class CustomKnnQueryBuilder extends AbstractQueryBuilder<CustomKnnQueryBuilder> {
    private final String field;
    private final float[] vector;
    private final int kval;

    public CustomKnnQueryBuilder(String field, float[] vector, int k) {
        this.field = field;
        this.vector = vector;
        this.kval = k;
    }

    @Override
    protected void doWriteTo(StreamOutput out) throws IOException {
        out.writeString(field);
        out.writeFloatArray(vector);
        out.writeInt(kval);
    }

    public CustomKnnQueryBuilder(StreamInput in) throws IOException {
        field = in.readString();
        vector = in.readFloatArray();
        kval = in.readInt();
    }

    @Override
    protected void doXContent(XContentBuilder builder, Params params) throws IOException {
        builder.startObject("knn");
        builder.startObject(field);
        builder.startArray("vector");
        for (float v : vector) {
            builder.value(v);
        }
        builder.endArray(); 
        builder.field("k", kval);
        builder.endObject();
        builder.endObject();
    }

    @Override
    protected Query doToQuery(QueryShardContext context) {
        return new KnnFloatVectorQuery(field, vector, kval);
    }


    @Override
    protected boolean doEquals(CustomKnnQueryBuilder other) {
        return field.equals(other.field) && kval == other.kval && java.util.Arrays.equals(vector, other.vector);
    }

    @Override
    protected int doHashCode() {
        return java.util.Objects.hash(field, java.util.Arrays.hashCode(vector), kval);
    }

    @Override
    public String getWriteableName() {
        return "custom_knn_query";
    }
}

i have this custom builder that requires no dependencies, but this is just for my use case, where i need to build the query along with other parts of the query

@jmazanec15 jmazanec15 removed their assignment Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

10 participants