You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/12/21 15:31:32 UTC
[GitHub] [lucene] alessandrobenedetti opened a new pull request, #12029: KnnVectorQuery introduce getters/setters
alessandrobenedetti opened a new pull request, #12029:
URL: https://github.com/apache/lucene/pull/12029
### Description
Knn Queries are locked currently, it would be beneficial for applications using them to have access to getters and setters.
An example is how filter queries are managed in Apache Solr:
the processing of pre-filters and post-filters could benefit from opening up the access to such variables.
Especially the pre-filter support introduced in Solr 9.1 could get great benefits from being able to set the filter, after the query has been parsed.
See:
https://github.com/apache/solr/pull/1245
If there are no objections I would simply remove the final and add the getters/setters.
I may consider alternative if there's some valid concern in doing that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
msokolov commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1366914837
Hi, you had asked for my review, but I think you got lots of helpful feedback already. Anyway this looks fine to me. I added one comment about a test, we could tighten it up a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361552026
All queries need to be immutable for the query cache to work correctly and consistently.
You are right, the docs need help here. Unfortunately docs on immutability were attached to deprecated methods and disappeared! https://github.com/apache/lucene-solr/blob/releases/lucene-solr/5.5.5/lucene/core/src/java/org/apache/lucene/search/Query.java#L95-L107
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361726280
BytesRef.clone won't do what we want here. it is a shallow clone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti merged pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by "alessandrobenedetti (via GitHub)" <gi...@apache.org>.
alessandrobenedetti merged PR #12029:
URL: https://github.com/apache/lucene/pull/12029
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361577899
the `getTarget()` getters are unsafe as they return mutable things (`float[]`, `BytesRef`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on a diff in pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
msokolov commented on code in PR #12029:
URL: https://github.com/apache/lucene/pull/12029#discussion_r1059769467
##########
lucene/core/src/test/org/apache/lucene/search/TestKnnVectorQuery.java:
##########
@@ -33,6 +33,7 @@
import org.apache.lucene.store.Directory;
import org.apache.lucene.util.TestVectorUtil;
import org.apache.lucene.util.VectorUtil;
+import org.junit.Assert;
Review Comment:
OK, tiny nit here - but LuceneTestCase inherits from Assert so we don't need to import and can just use the assertions directly without qualification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361523810
Thanks @rmuir for the prompt answer, I took a look at Query.java, and couldn't find any particular reason for the Knn query to be immutable(aside from historical reasons?).
If you can elaborate I am happy to consider alternatives (I could bring back final and just add getters if any better).
Knn query has a sub-query that uses as an internal filter, and having access to that can make the Solr side of filter/post-filters processing much easier and performant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on a diff in pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on code in PR #12029:
URL: https://github.com/apache/lucene/pull/12029#discussion_r1059446376
##########
lucene/core/src/test/org/apache/lucene/search/TestKnnByteVectorQuery.java:
##########
@@ -73,6 +73,14 @@ public void testToString() {
assertEquals("KnnByteVectorQuery:f1[0,...][10]", q1.toString("ignored"));
}
+ public void testGetTarget() {
+ byte[] queryVectorBytes = floatToBytes(new float[] {0, 1});
+ BytesRef targetQueryVector = new BytesRef(queryVectorBytes);
+ KnnByteVectorQuery q1 = new KnnByteVectorQuery("f1", queryVectorBytes, 10);
+
+ assertEquals(targetQueryVector, q1.getTargetCopy());
Review Comment:
done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361503436
queries should be immutable, see Query.java documentation. Hence I don't think we should add getter/setters or remove final keywords.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] risdenk commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1398549982
Re: immutable Query in Solr - See https://issues.apache.org/jira/browse/SOLR-16509 and https://github.com/apache/solr/pull/1146
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361606803
@rmuir you are right again, I gave it another try, using copies, this should be safe.
If there are still concerns I may move to some Builder/Constructors approaches, to be able to build a KnnVectorQuery starting from an old one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1362512997
Thanks again @rmuir, moved to a deep copy!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] dsmiley commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361753981
Completely agree with Robert -- Query subclasses ought to be immutable and the javadocs ought to be updated to scream this. Nasty/hard bugs happen when a Query is mutable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on a diff in pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
msokolov commented on code in PR #12029:
URL: https://github.com/apache/lucene/pull/12029#discussion_r1058588307
##########
lucene/core/src/test/org/apache/lucene/search/TestKnnByteVectorQuery.java:
##########
@@ -73,6 +73,14 @@ public void testToString() {
assertEquals("KnnByteVectorQuery:f1[0,...][10]", q1.toString("ignored"));
}
+ public void testGetTarget() {
+ byte[] queryVectorBytes = floatToBytes(new float[] {0, 1});
+ BytesRef targetQueryVector = new BytesRef(queryVectorBytes);
+ KnnByteVectorQuery q1 = new KnnByteVectorQuery("f1", queryVectorBytes, 10);
+
+ assertEquals(targetQueryVector, q1.getTargetCopy());
Review Comment:
Let's also assert that it's not the *same* BytesRef; i.e. enforce making a copy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] rmuir commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1362847643
needs javadocs and tests, otherwise it looks like these getters are unnecessary and should be factored out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1361578619
Thanks, @rmuir for the explanation, it seems reasonable and definitely, I won't argue with that.
For the sake of my needs, just the getters would be fine(and I'll clone the query, changing the filter).
I checked around and getters seem to be tolerated (see org.apache.lucene.search.FuzzyQuery).
Any problem with this?
I updated the Pull Request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on PR #12029:
URL: https://github.com/apache/lucene/pull/12029#issuecomment-1398524390
waiting for the checks and then I'll merge tonight!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] alessandrobenedetti commented on a diff in pull request #12029: introduce support in KnnVectorQuery for getters/setters
Posted by GitBox <gi...@apache.org>.
alessandrobenedetti commented on code in PR #12029:
URL: https://github.com/apache/lucene/pull/12029#discussion_r1082676520
##########
lucene/core/src/test/org/apache/lucene/search/TestKnnVectorQuery.java:
##########
@@ -33,6 +33,7 @@
import org.apache.lucene.store.Directory;
import org.apache.lucene.util.TestVectorUtil;
import org.apache.lucene.util.VectorUtil;
+import org.junit.Assert;
Review Comment:
done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org