SOLR-6784: BBoxField's 'score' mode should have been optional.

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1641670 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -441,6 +441,8 @@ Bug Fixes
 
 * SOLR-6781: BBoxField didn't support dynamic fields. (David Smiley)
 
+* SOLR-6784: BBoxField's 'score' mode should have been optional. (David Smiley)
+
 ==================  4.10.2 ==================
 
 Bug Fixes

solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java

@@ -59,6 +59,8 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 
@@ -76,6 +78,11 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
    */
   public static final String FILTER_PARAM = "filter";
 
+  //score param values:
+  public static final String DISTANCE = "distance";
+  public static final String RECIP_DISTANCE = "recipDistance";
+  public static final String NONE = "none";
+
   protected final Logger log = LoggerFactory.getLogger( getClass() );
 
   protected SpatialContext ctx;
@@ -83,6 +90,21 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
 
   private final Cache<String, T> fieldStrategyCache = CacheBuilder.newBuilder().build();
 
+  protected final Set<String> supportedScoreModes;
+
+  protected AbstractSpatialFieldType() {
+    this(Collections.emptySet());
+  }
+
+  protected AbstractSpatialFieldType(Set<String> moreScoreModes) {
+    Set<String> set = new TreeSet<>();//sorted for consistent display order
+    set.add(NONE);
+    set.add(DISTANCE);
+    set.add(RECIP_DISTANCE);
+    set.addAll(moreScoreModes);
+    supportedScoreModes = Collections.unmodifiableSet(set);
+  }
+
   @Override
   protected void init(IndexSchema schema, Map<String, String> args) {
     super.init(schema, args);
@@ -290,16 +312,27 @@ public abstract class AbstractSpatialFieldType<T extends SpatialStrategy> extend
     return new FilteredQuery(functionQuery, filter);
   }
 
+  /** The set of values supported for the score local-param. Not null. */
+  public Set<String> getSupportedScoreModes() {
+    return supportedScoreModes;
+  }
+
   protected ValueSource getValueSourceFromSpatialArgs(QParser parser, SchemaField field, SpatialArgs spatialArgs, String score, T strategy) {
-    if (score == null || "none".equals(score) || "".equals(score)) {
+    if (score == null) {
       return null;
-    } else if ("distance".equals(score)) {
+    }
+    switch (score) {
+      case NONE:
+      case "":
+        return null;
+      case DISTANCE:
         double multiplier = 1.0;//TODO support units=kilometers
         return strategy.makeDistanceValueSource(spatialArgs.getShape().getCenter(), multiplier);
-    } else if ("recipDistance".equals(score)) {
+      case RECIP_DISTANCE:
         return strategy.makeRecipDistanceValueSource(spatialArgs.getShape());
-    } else {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'score' local-param must be one of 'none', 'distance', or 'recipDistance'");
+      default:
+        throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
+            "'score' local-param must be one of " + supportedScoreModes);
     }
   }
 

solr/core/src/java/org/apache/solr/schema/BBoxField.java

@@ -28,17 +28,29 @@ import org.apache.solr.common.SolrException;
 import org.apache.solr.search.QParser;
 
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 
 public class BBoxField extends AbstractSpatialFieldType<BBoxStrategy> implements SchemaAware {
   private static final String PARAM_QUERY_TARGET_PROPORTION = "queryTargetProportion";
   private static final String PARAM_MIN_SIDE_LENGTH = "minSideLength";
+
+  //score modes:
+  private static final String OVERLAP_RATIO = "overlapRatio";
+  private static final String AREA = "area";
+  private static final String AREA2D = "area2D";
+
   private String numberTypeName;//required
   private String booleanTypeName = "boolean";
 
   private IndexSchema schema;
 
+  public BBoxField() {
+    super(new HashSet<>(Arrays.asList(OVERLAP_RATIO, AREA, AREA2D)));
+  }
+
   @Override
   protected void init(IndexSchema schema, Map<String, String> args) {
     super.init(schema, args);
@@ -125,9 +137,12 @@ public class BBoxField extends AbstractSpatialFieldType<BBoxStrategy> implements
 
   @Override
   protected ValueSource getValueSourceFromSpatialArgs(QParser parser, SchemaField field, SpatialArgs spatialArgs, String scoreParam, BBoxStrategy strategy) {
+    if (scoreParam == null) {
+      return null;
+    }
     switch (scoreParam) {
       //TODO move these to superclass after LUCENE-5804 ?
-      case "overlapRatio":
+      case OVERLAP_RATIO:
         double queryTargetProportion = 0.25;//Suggested default; weights towards target area
 
         String v = parser.getParam(PARAM_QUERY_TARGET_PROPORTION);
@@ -144,10 +159,10 @@ public class BBoxField extends AbstractSpatialFieldType<BBoxStrategy> implements
             (Rectangle) spatialArgs.getShape(),
             queryTargetProportion, minSideLength);
 
-      case "area":
+      case AREA:
         return new ShapeAreaValueSource(strategy.makeShapeValueSource(), ctx, ctx.isGeo());
 
-      case "area2D":
+      case AREA2D:
         return new ShapeAreaValueSource(strategy.makeShapeValueSource(), ctx, false);
 
       default:

solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java

@@ -51,7 +51,7 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 {
   @ParametersFactory
   public static Iterable<Object[]> parameters() {
     return Arrays.asList(new Object[][]{
-        {"srpt_geohash"}, {"srpt_quad"}, {"stqpt_geohash"}, {"pointvector"}
+        {"srpt_geohash"}, {"srpt_quad"}, {"stqpt_geohash"}, {"pointvector"}, {"bbox"}
     });
   }
 
@@ -158,7 +158,7 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 {
 
     assertQ(req(
         "fl", "id," + fieldName, "q", "*:*", "rows", "1000",
-        "fq", "{!geofilt sfield="+fieldName+" pt="+IN+" d=9}"),
+        "fq", "{!bbox sfield="+fieldName+" pt="+IN+" d=9}"),
         "//result/doc/*[@name='" + fieldName + "']//text()='" + OUT + "'");
   }
 
@@ -172,6 +172,9 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 {
   }
 
   private void checkHits(String fieldName, boolean exact, String ptStr, double distKM, int count, int ... docIds) throws ParseException {
+    if (exact && fieldName.equalsIgnoreCase("bbox")) {
+      return; // bbox field only supports rectangular query
+    }
     String [] tests = new String[docIds != null && docIds.length > 0 ? docIds.length + 1 : 1];
     //test for presence of required ids first
     int i = 0;
@@ -322,8 +325,10 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 {
 
   private String radiusQuery(double lat, double lon, double dDEG, String score, String filter) {
     //Choose between the Solr/Geofilt syntax, and the Lucene spatial module syntax
-    if (random().nextBoolean()) {
-      return "{!geofilt " +
+    if (fieldName.equals("bbox") || random().nextBoolean()) {
+      //we cheat for bbox strategy which doesn't do radius, only rect.
+      final String qparser = fieldName.equals("bbox") ? "bbox" : "geofilt";
+      return "{!" + qparser + " " +
           "sfield=" + fieldName + " "
           + (score != null ? "score="+score : "") + " "
           + (filter != null ? "filter="+filter : "") + " "
@@ -338,7 +343,8 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 {
 
   @Test
   public void testSortMultiVal() throws Exception {
-    RandomizedTest.assumeFalse("Multivalue not supported for this field", fieldName.equals("pointvector"));
+    RandomizedTest.assumeFalse("Multivalue not supported for this field",
+        fieldName.equals("pointvector") || fieldName.equals("bbox"));
 
     assertU(adoc("id", "100", fieldName, "1,2"));//1 point
     assertU(adoc("id", "101", fieldName, "4,-1", fieldName, "3,5"));//2 points, 2nd is pretty close to query point
@@ -373,12 +379,24 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 {
 
     //show we can index this (without an error)
     assertU(adoc("id", "rect", fieldName, rect));
+    if (!fieldName.equals("bbox")) {
       assertU(adoc("id", "circ", fieldName, circ));
       assertU(commit());
+    }
 
     //only testing no error
     assertJQ(req("q", "{!field f=" + fieldName + "}Intersects(" + rect + ")"));
+    if (!fieldName.equals("bbox")) {
       assertJQ(req("q", "{!field f=" + fieldName + "}Intersects(" + circ + ")"));
     }
+  }
+
+  @Test
+  public void testBadScoreParam() throws Exception {
+    assertQEx("expect friendly error message",
+        "none",
+        req(radiusQuery(0, 0, 0, "bogus", "false")),
+        SolrException.ErrorCode.BAD_REQUEST);
+  }
 
 }

solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java

@@ -18,6 +18,7 @@ package org.apache.solr.search;
  */
 
 import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
 import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
@@ -88,4 +89,13 @@ public class TestSolr4Spatial2 extends SolrTestCaseJ4 {
     );
   }
 
+  @Test
+  public void testBadScoreParam() throws Exception {
+    String fieldName = "bbox";
+    assertQEx("expect friendly error message",
+        "area2D",
+        req("{!field f="+fieldName+" filter=false score=bogus}Intersects(ENVELOPE(0,0,12,12))"),
+        SolrException.ErrorCode.BAD_REQUEST);
+  }
+
 }