[GitHub] [solr] magibney commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0

["magibney (via GitHub)" <gi...@apache.org>]

magibney commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1110386683


##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -359,14 +373,26 @@ public PointValues getPointValues(String field) {
   }
 
   @Override
-  public VectorValues getVectorValues(String field) {
+  public FloatVectorValues getFloatVectorValues(String field) {
+    ensureOpen();
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public ByteVectorValues getByteVectorValues(String field) {
     ensureOpen();
-    return VectorValues.EMPTY;
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public TopDocs searchNearestVectors(
+      String field, float[] target, int k, Bits acceptDocs, int visitedLimit) {
+    return null;
   }
 
   @Override
   public TopDocs searchNearestVectors(
-      String field, float[] target, int k, Bits acceptDocs, int visitedLimit) throws IOException {
+      String field, byte[] target, int k, Bits acceptDocs, int visitedLimit) {

Review Comment:
   I think removing the `throws IOException` from the `float[]` variant of `searchNearestVectors()` is out-of-scope for this PR (the method it inherits throws IOException). Among other things, it makes the diff less clear wrt what's been added:
   
   To clarify, this upgrade adds a `byte[]` variant of `searchNearestVectors()` and leaves the existing `float[]` variant as-is. True, because this class is final, there's probably no negative consequence to removing the `throws IOException` (from either variant, `byte[]` or `float[]`).
   
   But my inclination would be to try to keep changes as tightly scoped as possible, and probably even follow existing precedent and add `throws IOException` to the new new `byte[]` variant.
   
   Curious what others think? I definitely appreciate the clarity in the diff that results from restoring the `throws IOException` here.



##########
solr/core/src/java/org/apache/solr/index/SlowCompositeReaderWrapper.java:
##########
@@ -328,6 +331,17 @@ public Fields getTermVectors(int docID) throws IOException {
     return in.getTermVectors(docID);
   }
 
+  @Override
+  public TermVectors termVectors() throws IOException {
+    return in.termVectors();

Review Comment:
   I know this follows precedent from the existing `termVectors()` method above, but I wonder why these don't call `ensureOpen()` the wall all the other "content-access" methods seem to do?



##########
solr/core/src/java/org/apache/solr/update/processor/ClassificationUpdateProcessor.java:
##########
@@ -77,17 +77,21 @@ public ClassificationUpdateProcessor(
     }
     switch (classificationAlgorithm) {
       case KNN:
-        classifier =
-            new KNearestNeighborDocumentClassifier(
-                indexReader,
-                null,
-                classificationParams.getTrainingFilterQuery(),
-                classificationParams.getK(),
-                classificationParams.getMinDf(),
-                classificationParams.getMinTf(),
-                trainingClassField,
-                field2analyzer,
-                inputFieldNamesWithBoost);
+        try {
+          classifier =
+              new KNearestNeighborDocumentClassifier(
+                  indexReader,
+                  null,
+                  classificationParams.getTrainingFilterQuery(),
+                  classificationParams.getK(),
+                  classificationParams.getMinDf(),
+                  classificationParams.getMinTf(),
+                  trainingClassField,
+                  field2analyzer,
+                  inputFieldNamesWithBoost);
+        } catch (IOException e) {
+          throw new RuntimeException(e);
+        }

Review Comment:
   Prefer to wrap this in SolrException with more information about context (as opposed to a generic RuntimeException).



-- 
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@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org