Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using distance measure in the geo context precision #29273

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 46 additions & 1 deletion server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java
Expand Up @@ -24,10 +24,10 @@
import org.apache.lucene.spatial.prefix.tree.QuadPrefixTree;
import org.apache.lucene.util.SloppyMath;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.unit.DistanceUnit;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentParser.Token;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.fielddata.FieldData;
import org.elasticsearch.index.fielddata.GeoPointValues;
import org.elasticsearch.index.fielddata.MultiGeoPointValues;
Expand Down Expand Up @@ -459,6 +459,51 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
}
}

/**
* Parse a precision that can be expressed as an integer or a distance measure like "1km", "10m".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention that this "precision" is the geohash length?

*
* The precision is expressed as a number between 1 and 12 and indicates the length of geohash
* used to represent geo points.
*
* @param parser {@link XContentParser} to parse the value from
* @return int representing precision
*/
public static int parsePrecision(XContentParser parser) throws IOException, ElasticsearchParseException {
XContentParser.Token token = parser.currentToken();
if (token.equals(XContentParser.Token.VALUE_NUMBER)) {
return XContentMapValues.nodeIntegerValue(parser.intValue());
} else {
String precision = parser.text();
try {
// we want to treat simple integer strings as precision levels, not distances
return XContentMapValues.nodeIntegerValue(precision);
} catch (NumberFormatException e) {
// try to parse as a distance value
final int parsedPrecision = GeoUtils.geoHashLevelsForPrecision(precision);
try {
return checkPrecisionRange(parsedPrecision);
} catch (IllegalArgumentException e2) {
// this happens when distance too small, so precision > 12. We'd like to see the original string
throw new IllegalArgumentException("precision too high [" + precision + "]", e2);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we catch separately the call to checkPrecisionRange and geoHashLevelsForPrecision?

}
}
}

/**
* Checks that the precision is within range supported by elasticsearch - between 1 and 12
*
* Returns the precision value if it is in the range and throws an IllegalArgumentException if it
* is outside the range.
*/
public static int checkPrecisionRange(int precision) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a short javadoc?

if ((precision < 1) || (precision > 12)) {
throw new IllegalArgumentException("Invalid geohash aggregation precision of " + precision
+ ". Must be between 1 and 12.");
}
return precision;
}

/** Returns the maximum distance/radius (in meters) from the point 'center' before overlapping */
public static double maxRadialDistanceMeters(final double centerLat, final double centerLon) {
if (Math.abs(centerLat) == MAX_LAT) {
Expand Down
Expand Up @@ -54,6 +54,8 @@
import java.util.Map;
import java.util.Objects;

import static org.elasticsearch.common.geo.GeoUtils.parsePrecision;

public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder<ValuesSource.GeoPoint, GeoGridAggregationBuilder>
implements MultiBucketAggregationBuilder {
public static final String NAME = "geohash_grid";
Expand All @@ -64,29 +66,8 @@ public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder<Va
static {
PARSER = new ObjectParser<>(GeoGridAggregationBuilder.NAME);
ValuesSourceParserHelper.declareGeoFields(PARSER, false, false);
PARSER.declareField((parser, builder, context) -> {
XContentParser.Token token = parser.currentToken();
if (token.equals(XContentParser.Token.VALUE_NUMBER)) {
builder.precision(XContentMapValues.nodeIntegerValue(parser.intValue()));
} else {
String precision = parser.text();
try {
// we want to treat simple integer strings as precision levels, not distances
builder.precision(XContentMapValues.nodeIntegerValue(Integer.parseInt(precision)));
} catch (NumberFormatException e) {
// try to parse as a distance value
try {
builder.precision(GeoUtils.geoHashLevelsForPrecision(precision));
} catch (NumberFormatException e2) {
// can happen when distance unit is unknown, in this case we simply want to know the reason
throw e2;
} catch (IllegalArgumentException e3) {
// this happens when distance too small, so precision > 12. We'd like to see the original string
throw new IllegalArgumentException("precision too high [" + precision + "]", e3);
}
}
}
}, GeoHashGridParams.FIELD_PRECISION, org.elasticsearch.common.xcontent.ObjectParser.ValueType.INT);
PARSER.declareField((parser, builder, context) -> builder.precision(parsePrecision(parser)), GeoHashGridParams.FIELD_PRECISION,
org.elasticsearch.common.xcontent.ObjectParser.ValueType.INT);
PARSER.declareInt(GeoGridAggregationBuilder::size, GeoHashGridParams.FIELD_SIZE);
PARSER.declareInt(GeoGridAggregationBuilder::shardSize, GeoHashGridParams.FIELD_SHARD_SIZE);
}
Expand Down Expand Up @@ -133,7 +114,7 @@ protected void innerWriteTo(StreamOutput out) throws IOException {
}

public GeoGridAggregationBuilder precision(int precision) {
this.precision = GeoHashGridParams.checkPrecision(precision);
this.precision = GeoUtils.checkPrecisionRange(precision);
return this;
}

Expand Down
Expand Up @@ -30,15 +30,6 @@ final class GeoHashGridParams {
static final ParseField FIELD_SIZE = new ParseField("size");
static final ParseField FIELD_SHARD_SIZE = new ParseField("shard_size");


static int checkPrecision(int precision) {
if ((precision < 1) || (precision > 12)) {
throw new IllegalArgumentException("Invalid geohash aggregation precision of " + precision
+ ". Must be between 1 and 12.");
}
return precision;
}

private GeoHashGridParams() {
throw new AssertionError("No instances intended");
}
Expand Down
Expand Up @@ -33,6 +33,7 @@
import java.util.List;
import java.util.Objects;

import static org.elasticsearch.common.geo.GeoUtils.parsePrecision;
import static org.elasticsearch.search.suggest.completion.context.GeoContextMapping.CONTEXT_BOOST;
import static org.elasticsearch.search.suggest.completion.context.GeoContextMapping.CONTEXT_NEIGHBOURS;
import static org.elasticsearch.search.suggest.completion.context.GeoContextMapping.CONTEXT_PRECISION;
Expand Down Expand Up @@ -115,10 +116,10 @@ public static Builder builder() {
static {
GEO_CONTEXT_PARSER.declareField((parser, geoQueryContext, geoContextMapping) -> geoQueryContext.setGeoPoint(GeoUtils.parseGeoPoint(parser)), new ParseField(CONTEXT_VALUE), ObjectParser.ValueType.OBJECT);
GEO_CONTEXT_PARSER.declareInt(GeoQueryContext.Builder::setBoost, new ParseField(CONTEXT_BOOST));
// TODO : add string support for precision for GeoUtils.geoHashLevelsForPrecision()
GEO_CONTEXT_PARSER.declareInt(GeoQueryContext.Builder::setPrecision, new ParseField(CONTEXT_PRECISION));
// TODO : add string array support for precision for GeoUtils.geoHashLevelsForPrecision()
GEO_CONTEXT_PARSER.declareIntArray(GeoQueryContext.Builder::setNeighbours, new ParseField(CONTEXT_NEIGHBOURS));
GEO_CONTEXT_PARSER.declareField((parser, builder, context) -> builder.setPrecision(parsePrecision(parser)),
new ParseField(CONTEXT_PRECISION), ObjectParser.ValueType.INT);
GEO_CONTEXT_PARSER.declareFieldArray(GeoQueryContext.Builder::setNeighbours, (parser, builder) -> parsePrecision(parser),
new ParseField(CONTEXT_NEIGHBOURS), ObjectParser.ValueType.INT_ARRAY);
GEO_CONTEXT_PARSER.declareDouble(GeoQueryContext.Builder::setLat, new ParseField("lat"));
GEO_CONTEXT_PARSER.declareDouble(GeoQueryContext.Builder::setLon, new ParseField("lon"));
}
Expand Down
@@ -0,0 +1,71 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.common.geo;

import org.elasticsearch.common.CheckedConsumer;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.test.ESTestCase;

import java.io.IOException;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;

public class GeoUtilTests extends ESTestCase {

public void testPrecisionParser() throws IOException {
assertEquals(10, parsePrecision(builder -> builder.field("test", 10)));
assertEquals(10, parsePrecision(builder -> builder.field("test", 10.2)));
assertEquals(6, parsePrecision(builder -> builder.field("test", "6")));
assertEquals(7, parsePrecision(builder -> builder.field("test", "1km")));
assertEquals(7, parsePrecision(builder -> builder.field("test", "1.1km")));
}

public void testIncorrectPrecisionParser() {
expectThrows(NumberFormatException.class, () -> parsePrecision(builder -> builder.field("test", "10.1.1.1")));
expectThrows(NumberFormatException.class, () -> parsePrecision(builder -> builder.field("test", "364.4smoots")));
assertEquals(
"precision too high [0.01mm]",
expectThrows(IllegalArgumentException.class, () -> parsePrecision(builder -> builder.field("test", "0.01mm"))).getMessage()
);
}

/**
* Invokes GeoUtils.parsePrecision parser on the value generated by tokenGenerator
* <p>
* The supplied tokenGenerator should generate a single field that contains the precision in
* one of the supported formats or malformed precision value if error handling is tested. The
* method return the parsed value or throws an exception, if precision value is malformed.
*/
private int parsePrecision(CheckedConsumer<XContentBuilder, IOException> tokenGenerator) throws IOException {
XContentBuilder builder = jsonBuilder().startObject();
tokenGenerator.accept(builder);
builder.endObject();
XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder));
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); // {
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // field name
assertTrue(parser.nextToken().isValue()); // field value
int precision = GeoUtils.parsePrecision(parser);
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken()); // }
assertNull(parser.nextToken()); // no more tokens
return precision;
}
}
Expand Up @@ -19,15 +19,20 @@

package org.elasticsearch.search.suggest.completion;

import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.search.suggest.completion.context.GeoQueryContext;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.equalTo;

public class GeoQueryContextTests extends QueryContextTestCase<GeoQueryContext> {
Expand Down Expand Up @@ -105,4 +110,36 @@ public void testIllegalArguments() {
assertEquals(e.getMessage(), "neighbour value must be between 1 and 12");
}
}

public void testStringPrecision() throws IOException {
XContentBuilder builder = jsonBuilder().startObject();
{
builder.startObject("context").field("lat", 23.654242).field("lon", 90.047153).endObject();
builder.field("boost", 10);
builder.field("precision", 12);
builder.array("neighbours", 1, 2);
}
builder.endObject();
XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder));
parser.nextToken();
GeoQueryContext queryContext = fromXContent(parser);
assertEquals(10, queryContext.getBoost());
assertEquals(12, queryContext.getPrecision());
assertEquals(Arrays.asList(1, 2), queryContext.getNeighbours());

builder = jsonBuilder().startObject();
{
builder.startObject("context").field("lat", 23.654242).field("lon", 90.047153).endObject();
builder.field("boost", 10);
builder.field("precision", "12m");
builder.array("neighbours", "4km", "10km");
}
builder.endObject();
parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(builder));
parser.nextToken();
queryContext = fromXContent(parser);
assertEquals(10, queryContext.getBoost());
assertEquals(9, queryContext.getPrecision());
assertEquals(Arrays.asList(6, 5), queryContext.getNeighbours());
}
}