cpoerschke commented on code in PR #1958:
URL: https://github.com/apache/solr/pull/1958#discussion_r1340538175
##########
solr/core/src/test/org/apache/solr/search/neural/KnnQParserTest.java:
##########
@@ -39,7 +39,7 @@ public class KnnQParserTest extends SolrTestCaseJ4 {
@Before
public void prepareIndex() throws Exception {
/* vectorDimension="4" similarityFunction="cosine" */
- initCore("solrconfig-basic.xml", "schema-densevector.xml");
+ initCore("solrconfig_codec.xml", "schema-densevector.xml");
Review Comment:
1/3 I've been looking at the `DenseVectorFieldTest` test and also arrived at
the conclusion that something other than `solrconfig-basic.xml` is needed there
so that `SchemaCodecFactory` is used ...
##########
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##########
@@ -145,4 +151,34 @@ public Codec getCodec() {
assert core != null : "inform must be called first";
return codec;
}
+
+ /**
+ * This class exists because Lucene95HnswVectorsFormat's getMaxDimensions
method is final and we
+ * need to workaround that constraint to allow more than the default number
of dimensions
+ */
Review Comment:
3/3 ... this is a nice workaround to get our max dimensions.
Perhaps overengineering but something like
https://github.com/apache/lucene/blob/releases/lucene/9.8.0/lucene/core/src/test/org/apache/lucene/index/TestFilterCodecReader.java#L30
could be added as a test to ensure all that needs delegating is delegated.
And perhaps the workaround class could be final itself and even private?
Though that is both subjective of course.
##########
solr/core/src/java/org/apache/solr/core/SchemaCodecFactory.java:
##########
@@ -127,7 +132,8 @@ public KnnVectorsFormat getKnnVectorsFormatForField(String
field) {
if (DenseVectorField.HNSW_ALGORITHM.equals(knnAlgorithm)) {
int maxConn = vectorType.getHnswMaxConn();
int beamWidth = vectorType.getHnswBeamWidth();
- return new Lucene95HnswVectorsFormat(maxConn, beamWidth);
+ var delegate = new Lucene95HnswVectorsFormat(maxConn,
beamWidth);
+ return new SolrDelegatingKnnVectorsFormat(delegate,
vectorType.getDimension());
Review Comment:
2/3 ... but then yes we get to here but `Lucene95HnswVectorsFormat` being
final about its `getMaxDimensions` is a problem ...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
