[GitHub] [lucene-solr] jpountz commented on a change in pull request #1440: LUCENE-9330: Make SortFields responsible for index sorting and serialization

[GitBox]

jpountz commented on a change in pull request #1440:
URL: https://github.com/apache/lucene-solr/pull/1440#discussion_r412317472



##########
File path: lucene/core/src/java/org/apache/lucene/search/SortField.java
##########
@@ -120,6 +126,99 @@ public SortField(String field, Type type, boolean reverse) 
{
     this.reverse = reverse;
   }
 
+  /** A SortFieldProvider for field sorts */
+  public static final class Provider extends SortFieldProvider {
+
+    /** The name this Provider is registered under */
+    public static final String NAME = "field";

Review comment:
       maybe reuse the class name like we do for codecs, ie. `SortField`?

##########
File path: 
lucene/core/src/java/org/apache/lucene/index/DefaultIndexingChain.java
##########
@@ -527,45 +589,61 @@ private void indexPoint(PerField fp, IndexableField 
field) throws IOException {
     fp.pointValuesWriter.addPackedValue(docState.docID, field.binaryValue());
   }
 
-  private void validateIndexSortDVType(Sort indexSort, String fieldName, 
DocValuesType dvType) {
+  private void validateIndexSortDVType(Sort indexSort, String fieldToValidate, 
DocValuesType dvType) throws IOException {
     for (SortField sortField : indexSort.getSort()) {
-      if (sortField.getField().equals(fieldName)) {
-        switch (dvType) {
-          case NUMERIC:
-            if (sortField.getType().equals(SortField.Type.INT) == false &&
-                  sortField.getType().equals(SortField.Type.LONG) == false &&
-                  sortField.getType().equals(SortField.Type.FLOAT) == false &&
-                  sortField.getType().equals(SortField.Type.DOUBLE) == false) {
-              throw new IllegalArgumentException("invalid doc value type:" + 
dvType + " for sortField:" + sortField);
-            }
-            break;
+      IndexSorter sorter = sortField.getIndexSorter();
+      assert sorter != null;

Review comment:
       should it be an actual exception instead of an assertion? Or are we 
checking it up-front already?




----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org