introduce support in KnnVectorQuery for getters

[alessandrobenedetti]

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:
apache/solr#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.

[rmuir]

queries should be immutable, see Query.java documentation. Hence I don't think we should add getter/setters or remove final keywords.

[alessandrobenedetti]

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.

[rmuir]

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

[rmuir]

the getTarget() getters are unsafe as they return mutable things (float[], BytesRef)

[alessandrobenedetti]

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

[alessandrobenedetti]

@rmuir you are right again, I gave it another try, using copies(ArraysUtils for arrays and clone for byteref), 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.

[rmuir]

BytesRef.clone won't do what we want here. it is a shallow clone.

[dsmiley]

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.

[alessandrobenedetti]

Thanks again @rmuir, moved to a deep copy!

[rmuir]

needs javadocs and tests, otherwise it looks like these getters are unnecessary and should be factored out.

[msokolov]

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.

[alessandrobenedetti]

waiting for the checks and then I'll merge tonight!

[risdenk]

Re: immutable Query in Solr - See https://issues.apache.org/jira/browse/SOLR-16509 and apache/solr#1146