Skip to content

Commit

Permalink
Refactor GeoPoint and GeoShape with generics (#89388)
Browse files Browse the repository at this point in the history 
* Refactor GeoPoint and GeoShape with generics

In preparation for supporting CartesianPoint and CartesianShape
in aggregations, this PR adds a common interface between GeoPoint
and CartesianPoint, and then uses that to split out some key common
code that will be used in CartesianPoint and CartesianShape aggregations

* Simplify generics (by Ignacio)

Co-authored-by: Ignacio Vera <[email protected]>

* Refactor ElasticPoint to SpatialPoint

* Rename ShapeValuesProvider to ShapeValuesSource

It extends ValuesSource, and is extended by GeoShapeValuesSource.
There is no reason for the suffix `Provider`.

* Code review, mostly AbstractShapeIndexFieldData

* Reverted trivial refactoring

* Removed unused Writable interface implementation

* Further generics refinements

Based on Ignacio's work in 050df95,
we fix the BoundingBox generics, and also add a little more specificity
to the previous generics (replace <?> with <? extends SpatialPoint>).

* Removed some geo-specific code from BoundingBox

Co-authored-by: Ignacio Vera <[email protected]>
  • Loading branch information
@craigtaverner @iverase
craigtaverner and iverase authored Sep 7, 2022
1 parent 97f0e98 commit b68d62b
Show file tree
Hide file tree
Showing 83 changed files with 2,139 additions and 1,487 deletions.
4 changes: 2 additions & 2 deletions ...ang-expression/src/main/java/org/elasticsearch/script/expression/GeoEmptyValueSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.DoubleValues;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.LeafGeoPointFieldData;
import org.elasticsearch.index.fielddata.MultiGeoPointValues;
import org.elasticsearch.index.fielddata.plain.LeafGeoPointFieldData;

import java.io.IOException;

Expand All @@ -28,7 +28,7 @@ final class GeoEmptyValueSource extends FieldDataBasedDoubleValuesSource {
@Override
public DoubleValues getValues(LeafReaderContext leaf, DoubleValues scores) {
LeafGeoPointFieldData leafData = (LeafGeoPointFieldData) fieldData.load(leaf);
final MultiGeoPointValues values = leafData.getGeoPointValues();
final MultiGeoPointValues values = leafData.getPointValues(); // shade
return new DoubleValues() {
@Override
public double doubleValue() {
Expand Down
4 changes: 2 additions & 2 deletions ...-expression/src/main/java/org/elasticsearch/script/expression/GeoLatitudeValueSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.DoubleValues;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.LeafGeoPointFieldData;
import org.elasticsearch.index.fielddata.MultiGeoPointValues;
import org.elasticsearch.index.fielddata.plain.LeafGeoPointFieldData;

import java.io.IOException;

Expand All @@ -28,7 +28,7 @@ final class GeoLatitudeValueSource extends FieldDataBasedDoubleValuesSource {
@Override
public DoubleValues getValues(LeafReaderContext leaf, DoubleValues scores) {
LeafGeoPointFieldData leafData = (LeafGeoPointFieldData) fieldData.load(leaf);
final MultiGeoPointValues values = leafData.getGeoPointValues();
final MultiGeoPointValues values = leafData.getPointValues();
return new DoubleValues() {
@Override
public double doubleValue() throws IOException {
Expand Down
7 changes: 3 additions & 4 deletions ...expression/src/main/java/org/elasticsearch/script/expression/GeoLongitudeValueSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.DoubleValues;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.LeafGeoPointFieldData;
import org.elasticsearch.index.fielddata.MultiGeoPointValues;
import org.elasticsearch.index.fielddata.plain.LeafGeoPointFieldData;

import java.io.IOException;

Expand All @@ -28,7 +28,7 @@ final class GeoLongitudeValueSource extends FieldDataBasedDoubleValuesSource {
@Override
public DoubleValues getValues(LeafReaderContext leaf, DoubleValues scores) {
LeafGeoPointFieldData leafData = (LeafGeoPointFieldData) fieldData.load(leaf);
final MultiGeoPointValues values = leafData.getGeoPointValues();
final MultiGeoPointValues values = leafData.getPointValues();
return new DoubleValues() {
@Override
public double doubleValue() throws IOException {
Expand All @@ -53,8 +53,7 @@ public boolean equals(Object obj) {
if (obj == null) return false;
if (getClass() != obj.getClass()) return false;
GeoLongitudeValueSource other = (GeoLongitudeValueSource) obj;
if (fieldData.equals(other.fieldData) == false) return false;
return true;
return fieldData.equals(other.fieldData);
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions ...er/src/internalClusterTest/java/org/elasticsearch/search/aggregations/MissingValueIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ public void testGeoCentroid() {
assertSearchResponse(response);
GeoCentroid centroid = response.getAggregations().get("centroid");
GeoPoint point = new GeoPoint(1.5, 1.5);
assertThat(point.lat(), closeTo(centroid.centroid().lat(), 1E-5));
assertThat(point.lon(), closeTo(centroid.centroid().lon(), 1E-5));
assertThat(point.getY(), closeTo(centroid.centroid().getY(), 1E-5));
assertThat(point.getX(), closeTo(centroid.centroid().getX(), 1E-5));
}
}
37 changes: 9 additions & 28 deletions ...internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@ public void testEmptyAggregation() throws Exception {
assertThat(response.getHits().getTotalHits().value, equalTo(0L));
assertThat(geoCentroid, notNullValue());
assertThat(geoCentroid.getName(), equalTo(aggName));
GeoPoint centroid = geoCentroid.centroid();
assertThat(centroid, equalTo(null));
assertThat(geoCentroid.centroid(), equalTo(null));
assertEquals(0, geoCentroid.count());
}

Expand All @@ -59,8 +58,7 @@ public void testUnmapped() throws Exception {
GeoCentroid geoCentroid = response.getAggregations().get(aggName);
assertThat(geoCentroid, notNullValue());
assertThat(geoCentroid.getName(), equalTo(aggName));
GeoPoint centroid = geoCentroid.centroid();
assertThat(centroid, equalTo(null));
assertThat(geoCentroid.centroid(), equalTo(null));
assertEquals(0, geoCentroid.count());
}

Expand All @@ -73,9 +71,7 @@ public void testPartiallyUnmapped() throws Exception {
GeoCentroid geoCentroid = response.getAggregations().get(aggName);
assertThat(geoCentroid, notNullValue());
assertThat(geoCentroid.getName(), equalTo(aggName));
GeoPoint centroid = geoCentroid.centroid();
assertThat(centroid.lat(), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
assertThat(centroid.lon(), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
assertSameCentroid(geoCentroid.centroid(), singleCentroid);
assertEquals(numDocs, geoCentroid.count());
}

Expand All @@ -89,13 +85,11 @@ public void testSingleValuedField() throws Exception {
GeoCentroid geoCentroid = response.getAggregations().get(aggName);
assertThat(geoCentroid, notNullValue());
assertThat(geoCentroid.getName(), equalTo(aggName));
GeoPoint centroid = geoCentroid.centroid();
assertThat(centroid.lat(), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
assertThat(centroid.lon(), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
assertSameCentroid(geoCentroid.centroid(), singleCentroid);
assertEquals(numDocs, geoCentroid.count());
}

public void testSingleValueFieldGetProperty() throws Exception {
public void testSingleValueFieldGetProperty() {
SearchResponse response = client().prepareSearch(IDX_NAME)
.setQuery(matchAllQuery())
.addAggregation(global("global").subAggregation(geoCentroid(aggName).field(SINGLE_VALUED_FIELD_NAME)))
Expand All @@ -113,9 +107,7 @@ public void testSingleValueFieldGetProperty() throws Exception {
assertThat(geoCentroid, notNullValue());
assertThat(geoCentroid.getName(), equalTo(aggName));
assertThat((GeoCentroid) ((InternalAggregation) global).getProperty(aggName), sameInstance(geoCentroid));
GeoPoint centroid = geoCentroid.centroid();
assertThat(centroid.lat(), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
assertThat(centroid.lon(), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
assertSameCentroid(geoCentroid.centroid(), singleCentroid);
assertThat(
((GeoPoint) ((InternalAggregation) global).getProperty(aggName + ".value")).lat(),
closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE)
Expand All @@ -139,13 +131,11 @@ public void testMultiValuedField() throws Exception {
GeoCentroid geoCentroid = searchResponse.getAggregations().get(aggName);
assertThat(geoCentroid, notNullValue());
assertThat(geoCentroid.getName(), equalTo(aggName));
GeoPoint centroid = geoCentroid.centroid();
assertThat(centroid.lat(), closeTo(multiCentroid.lat(), GEOHASH_TOLERANCE));
assertThat(centroid.lon(), closeTo(multiCentroid.lon(), GEOHASH_TOLERANCE));
assertSameCentroid(geoCentroid.centroid(), multiCentroid);
assertEquals(2 * numDocs, geoCentroid.count());
}

public void testSingleValueFieldAsSubAggToGeohashGrid() throws Exception {
public void testSingleValueFieldAsSubAggToGeohashGrid() {
SearchResponse response = client().prepareSearch(HIGH_CARD_IDX_NAME)
.addAggregation(
geohashGrid("geoGrid").field(SINGLE_VALUED_FIELD_NAME).subAggregation(geoCentroid(aggName).field(SINGLE_VALUED_FIELD_NAME))
Expand All @@ -161,16 +151,7 @@ public void testSingleValueFieldAsSubAggToGeohashGrid() throws Exception {
String geohash = cell.getKeyAsString();
GeoPoint expectedCentroid = expectedCentroidsForGeoHash.get(geohash);
GeoCentroid centroidAgg = cell.getAggregations().get(aggName);
assertThat(
"Geohash " + geohash + " has wrong centroid latitude ",
expectedCentroid.lat(),
closeTo(centroidAgg.centroid().lat(), GEOHASH_TOLERANCE)
);
assertThat(
"Geohash " + geohash + " has wrong centroid longitude",
expectedCentroid.lon(),
closeTo(centroidAgg.centroid().lon(), GEOHASH_TOLERANCE)
);
assertSameCentroid(centroidAgg.centroid(), expectedCentroid);
}
}
}
22 changes: 11 additions & 11 deletions .../src/internalClusterTest/java/org/elasticsearch/search/geo/GeoPointScriptDocValuesIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import org.apache.lucene.geo.GeoEncodingUtils;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.common.document.DocumentField;
import org.elasticsearch.common.geo.GeoBoundingBox;
import org.elasticsearch.common.geo.BoundingBox;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.geo.GeometryTestUtils;
import org.elasticsearch.index.fielddata.ScriptDocValues;
Expand Down Expand Up @@ -66,52 +66,52 @@ protected Map<String, Function<Map<String, Object>, Object>> pluginScripts() {

private double scriptHeight(Map<String, Object> vars) {
Map<?, ?> doc = (Map<?, ?>) vars.get("doc");
ScriptDocValues.Geometry<?> geometry = assertGeometry(doc);
ScriptDocValues.Geometry geometry = assertGeometry(doc);
if (geometry.size() == 0) {
return Double.NaN;
} else {
GeoBoundingBox boundingBox = geometry.getBoundingBox();
BoundingBox<GeoPoint> boundingBox = geometry.getBoundingBox();
return boundingBox.topLeft().lat() - boundingBox.bottomRight().lat();
}
}

private double scriptWidth(Map<String, Object> vars) {
Map<?, ?> doc = (Map<?, ?>) vars.get("doc");
ScriptDocValues.Geometry<?> geometry = assertGeometry(doc);
ScriptDocValues.Geometry geometry = assertGeometry(doc);
if (geometry.size() == 0) {
return Double.NaN;
} else {
GeoBoundingBox boundingBox = geometry.getBoundingBox();
BoundingBox<GeoPoint> boundingBox = geometry.getBoundingBox();
return boundingBox.bottomRight().lon() - boundingBox.topLeft().lon();
}
}

private double scriptLat(Map<String, Object> vars) {
Map<?, ?> doc = (Map<?, ?>) vars.get("doc");
ScriptDocValues.Geometry<?> geometry = assertGeometry(doc);
ScriptDocValues.Geometry geometry = assertGeometry(doc);
return geometry.size() == 0 ? Double.NaN : geometry.getCentroid().lat();
}

private double scriptLon(Map<String, Object> vars) {
Map<?, ?> doc = (Map<?, ?>) vars.get("doc");
ScriptDocValues.Geometry<?> geometry = assertGeometry(doc);
ScriptDocValues.Geometry geometry = assertGeometry(doc);
return geometry.size() == 0 ? Double.NaN : geometry.getCentroid().lon();
}

private double scriptLabelLat(Map<String, Object> vars) {
Map<?, ?> doc = (Map<?, ?>) vars.get("doc");
ScriptDocValues.Geometry<?> geometry = assertGeometry(doc);
ScriptDocValues.Geometry geometry = assertGeometry(doc);
return geometry.size() == 0 ? Double.NaN : geometry.getLabelPosition().lat();
}

private double scriptLabelLon(Map<String, Object> vars) {
Map<?, ?> doc = (Map<?, ?>) vars.get("doc");
ScriptDocValues.Geometry<?> geometry = assertGeometry(doc);
ScriptDocValues.Geometry geometry = assertGeometry(doc);
return geometry.size() == 0 ? Double.NaN : geometry.getLabelPosition().lon();
}

private ScriptDocValues.Geometry<?> assertGeometry(Map<?, ?> doc) {
ScriptDocValues.Geometry<?> geometry = (ScriptDocValues.Geometry<?>) doc.get("location");
private ScriptDocValues.Geometry assertGeometry(Map<?, ?> doc) {
ScriptDocValues.Geometry geometry = (ScriptDocValues.Geometry) doc.get("location");
if (geometry.size() == 0) {
assertThat(geometry.getBoundingBox(), Matchers.nullValue());
assertThat(geometry.getCentroid(), Matchers.nullValue());
Expand Down
Loading

0 comments on commit b68d62b

Please sign in to comment.