You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ds...@apache.org on 2020/09/21 03:36:39 UTC
[lucene-solr] branch master updated: SOLR-14802: geodist: Support
most spatial field types as an arg FunctionQParser: overload
parseValueSourceList with flags
This is an automated email from the ASF dual-hosted git repository.
dsmiley pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
The following commit(s) were added to refs/heads/master by this push:
new fa756b1 SOLR-14802: geodist: Support most spatial field types as an arg FunctionQParser: overload parseValueSourceList with flags
fa756b1 is described below
commit fa756b1c316f1d775fcd4cdcb46e482b31c02fbd
Author: Tom Edge <to...@autotrader.co.uk>
AuthorDate: Sun Sep 20 23:19:48 2020 -0400
SOLR-14802: geodist: Support most spatial field types as an arg
FunctionQParser: overload parseValueSourceList with flags
---
solr/CHANGES.txt | 3 +
.../org/apache/solr/search/FunctionQParser.java | 14 +++-
.../distance/GeoDistValueSourceParser.java | 68 ++++++++++++-------
.../org/apache/solr/search/TestSolr4Spatial2.java | 78 +++++++++++++++++++---
4 files changed, 129 insertions(+), 34 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 8c33e79..b7d437b 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -181,6 +181,9 @@ Improvements
* SOLR-13438: On deleting a collection, its config set will also be deleted iff it has been auto-created, and not used by any other collection (Anderson Dorow)
+* SOLR-14802: geodist: Support most (all?) spatial field types as an argument like LLPSF, SRPTFT, and others.
+ (Tom Edge, Craig Wrigglesworth)
+
Optimizations
---------------------
diff --git a/solr/core/src/java/org/apache/solr/search/FunctionQParser.java b/solr/core/src/java/org/apache/solr/search/FunctionQParser.java
index 7743640..ad5ed3d 100644
--- a/solr/core/src/java/org/apache/solr/search/FunctionQParser.java
+++ b/solr/core/src/java/org/apache/solr/search/FunctionQParser.java
@@ -239,9 +239,21 @@ public class FunctionQParser extends QParser {
* @return List<ValueSource>
*/
public List<ValueSource> parseValueSourceList() throws SyntaxError {
+ return parseValueSourceList(FLAG_DEFAULT | FLAG_CONSUME_DELIMITER);
+ }
+
+ /**
+ * Parse a list of ValueSource. Must be the final set of arguments
+ * to a ValueSource.
+ *
+ * @param flags - customize parsing behavior
+ *
+ * @return List<ValueSource>
+ */
+ public List<ValueSource> parseValueSourceList(int flags) throws SyntaxError {
List<ValueSource> sources = new ArrayList<>(3);
while (hasMoreArguments()) {
- sources.add(parseValueSource(FLAG_DEFAULT | FLAG_CONSUME_DELIMITER));
+ sources.add(parseValueSource(flags));
}
return sources;
}
diff --git a/solr/core/src/java/org/apache/solr/search/function/distance/GeoDistValueSourceParser.java b/solr/core/src/java/org/apache/solr/search/function/distance/GeoDistValueSourceParser.java
index 4c845cd..9a6e69ca 100644
--- a/solr/core/src/java/org/apache/solr/search/function/distance/GeoDistValueSourceParser.java
+++ b/solr/core/src/java/org/apache/solr/search/function/distance/GeoDistValueSourceParser.java
@@ -16,6 +16,7 @@
*/
package org.apache.solr.search.function.distance;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
@@ -26,7 +27,6 @@ import org.apache.lucene.queries.function.valuesource.DoubleConstValueSource;
import org.apache.lucene.queries.function.valuesource.MultiValueSource;
import org.apache.lucene.queries.function.valuesource.VectorValueSource;
import org.apache.lucene.spatial.SpatialStrategy;
-import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.SpatialParams;
import org.apache.solr.schema.AbstractSpatialFieldType;
import org.apache.solr.schema.FieldType;
@@ -34,12 +34,17 @@ import org.apache.solr.schema.SchemaField;
import org.apache.solr.search.FunctionQParser;
import org.apache.solr.search.SyntaxError;
import org.apache.solr.search.ValueSourceParser;
+import org.apache.solr.search.function.FieldNameValueSource;
import org.apache.solr.util.DistanceUnits;
import org.apache.solr.util.SpatialUtils;
import org.locationtech.spatial4j.context.SpatialContext;
import org.locationtech.spatial4j.distance.DistanceUtils;
import org.locationtech.spatial4j.shape.Point;
+import static org.apache.solr.search.FunctionQParser.FLAG_CONSUME_DELIMITER;
+import static org.apache.solr.search.FunctionQParser.FLAG_DEFAULT;
+import static org.apache.solr.search.FunctionQParser.FLAG_USE_FIELDNAME_SOURCE;
+
/**
* Parses "geodist" creating {@link HaversineConstFunction} or {@link HaversineFunction}
* or calling {@link SpatialStrategy#makeDistanceValueSource(org.locationtech.spatial4j.shape.Point,double)}.
@@ -50,21 +55,8 @@ public class GeoDistValueSourceParser extends ValueSourceParser {
public ValueSource parse(FunctionQParser fp) throws SyntaxError {
// TODO: dispatch through SpatialQueryable in the future?
- //note: parseValueSourceList can't handle a field reference to an AbstractSpatialFieldType,
- // so those fields are expressly handled via sfield=
- List<ValueSource> sources;
- try {
- sources = fp.parseValueSourceList();
- } catch (SolrException e) {
- if (e.getMessage().equals("A ValueSource isn't directly available from this field. " +
- "Instead try a query using the distance as the score.")) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "geodist() does not support field names in its arguments " +
- "when stated fields are solr.LatLonPointSpatialField spatial type, requires sfield param instead");
- }
- else {
- throw e;
- }
- }
+ //note: return fields as FieldNameValueSource from parser to support AbstractSpatialFieldType as geodist argument
+ List<ValueSource> sources = transformFieldSources(fp, fp.parseValueSourceList(FLAG_DEFAULT | FLAG_CONSUME_DELIMITER | FLAG_USE_FIELDNAME_SOURCE));
// "m" is a multi-value source, "x" is a single-value source
// allow (m,m) (m,x,x) (x,x,m) (x,x,x,x)
@@ -93,7 +85,6 @@ public class GeoDistValueSourceParser extends ValueSourceParser {
}
} else if (sources.size()==3) {
ValueSource vs1 = sources.get(0);
- ValueSource vs2 = sources.get(1);
if (vs1 instanceof MultiValueSource) { // (m,x,x)
mv1 = (MultiValueSource)vs1;
mv2 = makeMV(sources.subList(1, 3), sources);
@@ -108,7 +99,7 @@ public class GeoDistValueSourceParser extends ValueSourceParser {
} else if (sources.size()==4) {
mv1 = makeMV(sources.subList(0, 2), sources);
mv2 = makeMV(sources.subList(2, 4), sources);
- } else if (sources.size() > 4) {
+ } else {
throw new SyntaxError("geodist - invalid parameters:" + sources);
}
@@ -139,14 +130,14 @@ public class GeoDistValueSourceParser extends ValueSourceParser {
// * HaversineConstFunction
// * HaversineFunction
- // sfield can only be in mv2, according to the logic above
- if (mv2 instanceof SpatialStrategyMultiValueSource) {
+ SpatialStrategyMultiValueSource spatialStrategyMultiValueSource = findSpatialStrategyMultiValueSource(mv1, mv2);
+ if (spatialStrategyMultiValueSource != null) {
if (constants == null)
throw new SyntaxError("When using AbstractSpatialFieldType (e.g. RPT not LatLonType)," +
" the point must be supplied as constants");
// note: uses Haversine by default but can be changed via distCalc=...
- SpatialStrategy strategy = ((SpatialStrategyMultiValueSource) mv2).strategy;
- DistanceUnits distanceUnits = ((SpatialStrategyMultiValueSource) mv2).distanceUnits;
+ SpatialStrategy strategy = spatialStrategyMultiValueSource.strategy;
+ DistanceUnits distanceUnits = spatialStrategyMultiValueSource.distanceUnits;
Point queryPoint = strategy.getSpatialContext().makePoint(constants[1], constants[0]);
return ValueSource.fromDoubleValuesSource(strategy.makeDistanceValueSource(queryPoint, distanceUnits.multiplierFromDegreesToThisUnit()));
}
@@ -158,6 +149,29 @@ public class GeoDistValueSourceParser extends ValueSourceParser {
return new HaversineFunction(mv1, mv2, DistanceUtils.EARTH_MEAN_RADIUS_KM, true);
}
+ private List<ValueSource> transformFieldSources(FunctionQParser fp, List<ValueSource> sources) throws SyntaxError {
+ List<ValueSource> result = new ArrayList<>(sources.size());
+ for (ValueSource valueSource : sources) {
+ if (valueSource instanceof FieldNameValueSource) {
+ String fieldName = ((FieldNameValueSource) valueSource).getFieldName();
+ result.add(getMultiValueSource(fp, fieldName));
+ } else {
+ result.add(valueSource);
+ }
+ }
+ return result;
+ }
+
+ private SpatialStrategyMultiValueSource findSpatialStrategyMultiValueSource(MultiValueSource mv1, MultiValueSource mv2) {
+ if (mv1 instanceof SpatialStrategyMultiValueSource) {
+ return (SpatialStrategyMultiValueSource) mv1;
+ } else if (mv2 instanceof SpatialStrategyMultiValueSource) {
+ return (SpatialStrategyMultiValueSource) mv2;
+ } else {
+ return null;
+ }
+ }
+
/** make a MultiValueSource from two non MultiValueSources */
private VectorValueSource makeMV(List<ValueSource> sources, List<ValueSource> orig) throws SyntaxError {
ValueSource vs1 = sources.get(0);
@@ -169,17 +183,17 @@ public class GeoDistValueSourceParser extends ValueSourceParser {
return new VectorValueSource(sources);
}
- private MultiValueSource parsePoint(FunctionQParser fp) throws SyntaxError {
+ private MultiValueSource parsePoint(FunctionQParser fp) {
String ptStr = fp.getParam(SpatialParams.POINT);
if (ptStr == null) return null;
Point point = SpatialUtils.parsePointSolrException(ptStr, SpatialContext.GEO);
//assume Lat Lon order
return new VectorValueSource(
- Arrays.<ValueSource>asList(new DoubleConstValueSource(point.getY()), new DoubleConstValueSource(point.getX())));
+ Arrays.asList(new DoubleConstValueSource(point.getY()), new DoubleConstValueSource(point.getX())));
}
private double[] getConstants(MultiValueSource vs) {
- if (!(vs instanceof VectorValueSource)) return null;
+ if (vs instanceof SpatialStrategyMultiValueSource || !(vs instanceof VectorValueSource)) return null;
List<ValueSource> sources = ((VectorValueSource)vs).getSources();
if (sources.get(0) instanceof ConstNumberSource && sources.get(1) instanceof ConstNumberSource) {
return new double[] { ((ConstNumberSource) sources.get(0)).getDouble(), ((ConstNumberSource) sources.get(1)).getDouble()};
@@ -190,6 +204,10 @@ public class GeoDistValueSourceParser extends ValueSourceParser {
private MultiValueSource parseSfield(FunctionQParser fp) throws SyntaxError {
String sfield = fp.getParam(SpatialParams.FIELD);
if (sfield == null) return null;
+ return getMultiValueSource(fp, sfield);
+ }
+
+ private MultiValueSource getMultiValueSource(FunctionQParser fp, String sfield) throws SyntaxError {
SchemaField sf = fp.getReq().getSchema().getField(sfield);
FieldType type = sf.getType();
if (type instanceof AbstractSpatialFieldType) {
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 0f5feed..807bac2 100644
--- a/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java
+++ b/solr/core/src/test/org/apache/solr/search/TestSolr4Spatial2.java
@@ -373,14 +373,76 @@ public class TestSolr4Spatial2 extends SolrTestCaseJ4 {
assertQ(req(params), "*[count(//doc)=1]", "count(//lst[@name='highlighting']/*)=1");
}
- @Test
- public void testErrorHandlingGeodist() throws Exception{
- assertU(adoc("id", "1", "llp", "32.7693246, -79.9289094"));
- assertQEx("wrong test exception message","sort param could not be parsed as a query, " +
- "and is not a field that exists in the index: geodist(llp,47.36667,8.55)",
- req(
+ @Test // SOLR-14802
+ public void testGeodistSortPossibleWithLatLonPointSpatialFieldOrSpatialRecursivePrefixTreeField() throws Exception {
+ assertU(adoc("id", "1", "llp", "53.4721936,-2.24703", "srpt_quad", "53.425272,-2.322356"));
+ assertU(commit());
+
+ assertJQ(req(
+ "q", "*:*",
+ "fq", "{!geofilt}",
+ "d", "50",
+ "pt", "53.4721936,-2.24703",
+ "sfield", "srpt_quad",
+ "sort", "min(geodist(),geodist(llp,53.4721936,-2.24703)) asc"
+ ),
+ "/response/docs/[0]/id=='1'");
+
+ assertJQ(req(
+ "q", "*:*",
+ "fq", "{!geofilt}",
+ "d", "50",
+ "pt", "53.4721936,-2.24703",
+ "sfield", "srpt_quad",
+ "sort", "min(geodist(),geodist(53.4721936,-2.24703,llp)) asc" // moved llp to the end
+ ),
+ "/response/docs/[0]/id=='1'");
+
+ assertJQ(req(
+ "q", "*:*",
+ "fq", "{!geofilt}",
+ "d", "50",
+ "pt", "53.4721936,-2.24703",
+ "sfield", "llp", // trying another field type
+ "sort", "min(geodist(),geodist(53.4721936,-2.24703,srpt_quad)) asc"
+ ),
+ "/response/docs/[0]/id=='1'");
+ }
+
+ @Test // SOLR-14802
+ public void testGeodistSortOrderCorrectWithLatLonPointSpatialFieldAndSpatialRecursivePrefixTreeField() throws Exception {
+ assertU(adoc("id", "1", "llp", "53.4721936,-2.24703", "srpt_quad", "53.4721936,-2.24703"));
+ assertU(adoc("id", "2", "llp", "53.425272,-2.322356", "srpt_quad", "55.4721936,-2.24703"));
+ assertU(commit());
+
+ assertJQ(req(
+ "q", "*:*",
+ "fq", "{!geofilt}",
+ "d", "50",
+ "pt", "53.431669,-2.318720",
+ "sfield", "srpt_quad",
+ "sort", "min(geodist(),geodist(llp,53.431669,-2.318720)) asc"
+ ),
+ "/response/docs/[0]/id=='2'");
+
+ assertJQ(req(
+ "q", "*:*",
+ "fq", "{!geofilt}",
+ "d", "50",
+ "pt", "53.4721936,-2.24703",
+ "sfield", "srpt_quad",
+ "sort", "min(geodist(),geodist(53.4721936,-2.24703,llp)) asc"
+ ),
+ "/response/docs/[0]/id=='1'");
+
+ assertJQ(req(
"q", "*:*",
- "sort", "geodist(llp,47.36667,8.55) asc"
- ), SolrException.ErrorCode.BAD_REQUEST);
+ "fq", "{!geofilt}",
+ "d", "50",
+ "pt", "55.4721936,-2.24703",
+ "sfield", "srpt_quad",
+ "sort", "min(geodist(),geodist(55.4721936,-2.24703,llp)) asc"
+ ),
+ "/response/docs/[0]/id=='2'");
}
}