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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org