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

Adding filter to the toString() method of KnnFloatVectorQuery #13990

Merged
merged 6 commits into from
Nov 18, 2024
Merged

Adding filter to the toString() method of KnnFloatVectorQuery #13990

merged 6 commits into from
Nov 18, 2024

Conversation

viswanathk
Copy link
Contributor

@viswanathk viswanathk commented Nov 12, 2024 •
edited
Loading

Addresses the issue #13983

@viswanathk viswanathk changed the title Addresses the issue #13983 by adding filter to the toString() method of KnnFloatVectorQuery Adding filter to the toString() method of KnnFloatVectorQuery Nov 12, 2024
benwtrent
benwtrent requested changes Nov 12, 2024
View reviewed changes
Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

Could you update the byte knn query & DiversifyingChildern* knn queries as well?

lucene/core/src/test/org/apache/lucene/search/TestKnnFloatVectorQuery.java Outdated Show resolved Hide resolved
lucene/core/src/java/org/apache/lucene/search/KnnFloatVectorQuery.java Outdated Show resolved Hide resolved
@viswanathk
Copy link
Contributor Author

Could you update the byte knn query & DiversifyingChildern* knn queries as well?

I made the changes fo DiversifyingChildren*, but by byteknn do you mean ByteVectorSimilarityQuery?

@jpountz
Copy link
Contributor

jpountz commented Nov 12, 2024

I believe that @benwtrent meant KnnByteVectorQuery.

@viswanathk
Copy link
Contributor Author

I hope I got all of them now.

@viswanathk viswanathk requested a review from benwtrent November 12, 2024 18:17
@jpountz
Copy link
Contributor

jpountz commented Nov 13, 2024

Could you update the byte knn query & DiversifyingChildern* knn queries as well?

Unrelated: I noticed that the DiversifyingChildren* queries don't use the filter to evaluate the query, is this a bug?

@benwtrent
Copy link
Member

Unrelated: I noticed that the DiversifyingChildren* queries don't use the filter to evaluate the query, is this a bug?

I am unsure what you mean by this? The filter is utilized when building the accepted docs set in the super() classes.

benwtrent
benwtrent approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@benwtrent benwtrent left a comment

Choose a reason for hiding this comment

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

I think a CHANGES entry is in order. This seems like a nice little bug fix to aid folks in debugging issues.

@viswanathk once you add the changes entry, I can merge and backport to 10.x

@viswanathk
Copy link
Contributor Author

I think a CHANGES entry is in order. This seems like a nice little bug fix to aid folks in debugging issues.

@viswanathk once you add the changes entry, I can merge and backport to 10.x

@benwtrent Added in CHANGES for 10.1. Also corrected some git history mess up.

@benwtrent benwtrent merged commit a991a9a into apache:main Nov 18, 2024
3 checks passed
benwtrent pushed a commit that referenced this pull request Nov 18, 2024
@viswanathk @benwtrent
Adding filter to the toString() method of KnnFloatVectorQuery (#13990)
c96ec0b 
* Adding filter to toString() of KnnFloatVectorQuery when it's present (addresses #13983)

* addressing review comments

* adding knnbytevectorquery

* unit test improvements

* tidy

* adding changes entry for the bug fix
@benwtrent benwtrent mentioned this pull request Nov 18, 2024
Closed
benchaplin pushed a commit to benchaplin/lucene that referenced this pull request Dec 31, 2024
@viswanathk @benchaplin
Adding filter to the toString() method of KnnFloatVectorQuery (apache…
f0694a0 
…#13990)

* Adding filter to toString() of KnnFloatVectorQuery when it's present (addresses apache#13983)

* addressing review comments

* adding knnbytevectorquery

* unit test improvements

* tidy

* adding changes entry for the bug fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benwtrent benwtrent benwtrent approved these changes

@vigyasharma vigyasharma vigyasharma approved these changes

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@viswanathk @jpountz @benwtrent @vigyasharma