diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index bbe20c8a13e..1263f786da5 100644 --- a/solr/CHANGES.txt +++ b/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 diff --git a/solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java b/solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java index 4ba31ddfcad..25ce47917ed 100644 --- a/solr/core/src/java/org/apache/solr/schema/AbstractSpatialFieldType.java +++ b/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 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 extend private final Cache fieldStrategyCache = CacheBuilder.newBuilder().build(); + protected final Set supportedScoreModes; + + protected AbstractSpatialFieldType() { + this(Collections.emptySet()); + } + + protected AbstractSpatialFieldType(Set moreScoreModes) { + Set 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 args) { super.init(schema, args); @@ -290,16 +312,27 @@ public abstract class AbstractSpatialFieldType extend return new FilteredQuery(functionQuery, filter); } + /** The set of values supported for the score local-param. Not null. */ + public Set 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)) { - double multiplier = 1.0;//TODO support units=kilometers - return strategy.makeDistanceValueSource(spatialArgs.getShape().getCenter(), multiplier); - } else if ("recipDistance".equals(score)) { - return strategy.makeRecipDistanceValueSource(spatialArgs.getShape()); - } else { - throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'score' local-param must be one of 'none', 'distance', or 'recipDistance'"); + } + switch (score) { + case NONE: + case "": + return null; + case DISTANCE: + double multiplier = 1.0;//TODO support units=kilometers + return strategy.makeDistanceValueSource(spatialArgs.getShape().getCenter(), multiplier); + case RECIP_DISTANCE: + return strategy.makeRecipDistanceValueSource(spatialArgs.getShape()); + default: + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "'score' local-param must be one of " + supportedScoreModes); } } diff --git a/solr/core/src/java/org/apache/solr/schema/BBoxField.java b/solr/core/src/java/org/apache/solr/schema/BBoxField.java index 286c72cdae5..9c7a6610291 100644 --- a/solr/core/src/java/org/apache/solr/schema/BBoxField.java +++ b/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 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 args) { super.init(schema, args); @@ -125,9 +137,12 @@ public class BBoxField extends AbstractSpatialFieldType 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 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: diff --git a/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java b/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java index ca7e296b2a9..12c57254a19 100644 --- a/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java +++ b/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial.java @@ -51,7 +51,7 @@ public class TestSolr4Spatial extends SolrTestCaseJ4 { @ParametersFactory public static Iterable 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)); - assertU(adoc("id", "circ", fieldName, circ)); - assertU(commit()); + if (!fieldName.equals("bbox")) { + assertU(adoc("id", "circ", fieldName, circ)); + assertU(commit()); + } //only testing no error assertJQ(req("q", "{!field f=" + fieldName + "}Intersects(" + rect + ")")); - assertJQ(req("q", "{!field f=" + fieldName + "}Intersects(" + circ + ")")); + 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); } } diff --git a/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java b/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java index fe2a84ec79b..1dc7d6c472b 100644 --- a/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java +++ b/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); + } + }