Add build() method to SortBuilder implementations #17146
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,17 +19,34 @@ | |
|
|
||
| package org.elasticsearch.search.sort; | ||
|
|
||
| import org.apache.lucene.index.LeafReaderContext; | ||
| import org.apache.lucene.index.NumericDocValues; | ||
| import org.apache.lucene.search.DocIdSetIterator; | ||
| import org.apache.lucene.search.FieldComparator; | ||
| import org.apache.lucene.search.SortField; | ||
| import org.apache.lucene.util.BitSet; | ||
| import org.elasticsearch.ElasticsearchParseException; | ||
| import org.elasticsearch.Version; | ||
| import org.elasticsearch.common.geo.GeoDistance; | ||
| import org.elasticsearch.common.geo.GeoDistance.FixedSourceDistance; | ||
| import org.elasticsearch.common.geo.GeoPoint; | ||
| import org.elasticsearch.common.geo.GeoUtils; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.common.unit.DistanceUnit; | ||
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
| import org.elasticsearch.common.xcontent.XContentParser; | ||
| import org.elasticsearch.index.fielddata.IndexFieldData; | ||
| import org.elasticsearch.index.fielddata.IndexFieldData.XFieldComparatorSource.Nested; | ||
| import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; | ||
| import org.elasticsearch.index.fielddata.MultiGeoPointValues; | ||
| import org.elasticsearch.index.fielddata.NumericDoubleValues; | ||
| import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; | ||
| import org.elasticsearch.index.mapper.MappedFieldType; | ||
| import org.elasticsearch.index.query.QueryBuilder; | ||
| import org.elasticsearch.index.query.QueryParseContext; | ||
| import org.elasticsearch.index.query.QueryShardContext; | ||
| import org.elasticsearch.search.MultiValueMode; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
|
|
@@ -405,13 +422,10 @@ public GeoDistanceSortBuilder fromXContent(QueryParseContext context, String ele | |
|
|
||
| fieldName = currentName; | ||
| } else if (token == XContentParser.Token.START_OBJECT) { | ||
| if ("nested_filter".equals(currentName) || "nestedFilter".equals(currentName)) { | ||
| nestedFilter = context.parseInnerQueryBuilder(); | ||
| } else { | ||
| // the json in the format of -> field : { lat : 30, lon : 12 } | ||
| fieldName = currentName; | ||
| GeoPoint point = new GeoPoint(); | ||
| GeoUtils.parseGeoPoint(parser, point); | ||
|
|
@@ -461,7 +475,89 @@ public GeoDistanceSortBuilder fromXContent(QueryParseContext context, String ele | |
| result.coerce(coerce); | ||
| result.ignoreMalformed(ignoreMalformed); | ||
| return result; | ||
| } | ||
|
|
||
| @Override | ||
| public SortField build(QueryShardContext context) throws IOException { | ||
| final boolean indexCreatedBeforeV2_0 = context.indexVersionCreated().before(Version.V_2_0_0); | ||
| // validation was not available prior to 2.x, so to support bwc percolation queries we only ignore_malformed on 2.x created indexes | ||
| List<GeoPoint> localPoints = new ArrayList<GeoPoint>(); | ||
| for (GeoPoint geoPoint : this.points) { | ||
| localPoints.add(new GeoPoint(geoPoint)); | ||
| } | ||
|
|
||
| if (!indexCreatedBeforeV2_0 && !ignoreMalformed) { | ||
| for (GeoPoint point : localPoints) { | ||
| if (point.lat() > 90.0 || point.lat() < -90.0) { | ||
|
There was a problem hiding this comment. Why not use this: GeoUtils.isValidLatitude() |
||
| throw new ElasticsearchParseException("illegal latitude value [{}] for [GeoDistanceSort]", point.lat()); | ||
| } | ||
| if (point.lon() > 180.0 || point.lon() < -180) { | ||
|
There was a problem hiding this comment. See above, there's another method for longitude validation. |
||
| throw new ElasticsearchParseException("illegal longitude value [{}] for [GeoDistanceSort]", point.lon()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (coerce) { | ||
| for (GeoPoint point : localPoints) { | ||
| GeoUtils.normalizePoint(point, coerce, coerce); | ||
| } | ||
| } | ||
|
|
||
| boolean reverse = (order == SortOrder.DESC); | ||
| final MultiValueMode finalSortMode; | ||
| if (sortMode == null) { | ||
| finalSortMode = reverse ? MultiValueMode.MAX : MultiValueMode.MIN; | ||
| } else { | ||
| finalSortMode = MultiValueMode.fromString(sortMode.toString()); | ||
| } | ||
|
|
||
| if (sortMode == SortMode.SUM) { | ||
| throw new IllegalArgumentException("sort_mode [sum] isn't supported for sorting by geo distance"); | ||
|
There was a problem hiding this comment. https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-sort.html ... says that "geo distance sorting supports sort_mode options: min, max and avg. Looking here: https://github.com/elastic/elasticsearch/blob/7ccf7adbdf748e3dc2cb37c027bcf30a1bae54a9/core/src/main/java/org/elasticsearch/search/sort/SortMode.java we have min, max, avg, sum and median We forbid sum above - but what about median? Bug in the docs? Bug in the original implementation? There was a problem hiding this comment. Two things here, need to do some more digging:
There was a problem hiding this comment. MultiValueMode.MEDIAN seems to work with GeoDistanceSort, I added a test to GeoDistanceSortBuilderIT. The check for correct SortMode in build() can be removed because we already check this in the setter and there should be no other way to set the SortMode. |
||
| } | ||
|
|
||
| MappedFieldType fieldType = context.fieldMapper(fieldName); | ||
| if (fieldType == null) { | ||
| throw new IllegalArgumentException("failed to find mapper for [" + fieldName + "] for geo distance based sort"); | ||
| } | ||
| final IndexGeoPointFieldData geoIndexFieldData = context.getForField(fieldType); | ||
| final FixedSourceDistance[] distances = new FixedSourceDistance[localPoints.size()]; | ||
| for (int i = 0; i< localPoints.size(); i++) { | ||
| distances[i] = geoDistance.fixedSourceDistance(localPoints.get(i).lat(), localPoints.get(i).lon(), unit); | ||
| } | ||
|
|
||
| final Nested nested = resolveNested(context, nestedPath, nestedFilter); | ||
|
|
||
| IndexFieldData.XFieldComparatorSource geoDistanceComparatorSource = new IndexFieldData.XFieldComparatorSource() { | ||
|
|
||
| @Override | ||
| public SortField.Type reducedType() { | ||
| return SortField.Type.DOUBLE; | ||
| } | ||
|
|
||
| @Override | ||
| public FieldComparator<?> newComparator(String fieldname, int numHits, int sortPos, boolean reversed) throws IOException { | ||
| return new FieldComparator.DoubleComparator(numHits, null, null) { | ||
| @Override | ||
| protected NumericDocValues getNumericDocValues(LeafReaderContext context, String field) throws IOException { | ||
| final MultiGeoPointValues geoPointValues = geoIndexFieldData.load(context).getGeoPointValues(); | ||
| final SortedNumericDoubleValues distanceValues = GeoDistance.distanceValues(geoPointValues, distances); | ||
| final NumericDoubleValues selectedValues; | ||
| if (nested == null) { | ||
| selectedValues = finalSortMode.select(distanceValues, Double.MAX_VALUE); | ||
| } else { | ||
| final BitSet rootDocs = nested.rootDocs(context); | ||
| final DocIdSetIterator innerDocs = nested.innerDocs(context); | ||
| selectedValues = finalSortMode.select(distanceValues, Double.MAX_VALUE, rootDocs, innerDocs, | ||
| context.reader().maxDoc()); | ||
| } | ||
| return selectedValues.getRawDoubleValues(); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| }; | ||
|
|
||
| return new SortField(fieldName, geoDistanceComparatorSource, reverse); | ||
| } | ||
|
|
||
| static void parseGeoPoints(XContentParser parser, List<GeoPoint> geoPoints) throws IOException { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having code commented out here - would it be better to track that in some follow up issue? (I know the comment was there before the refactoring, but code that's commented out always smells strange IMHO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just talked to @martijnvg, this is old code and can be removed.