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.

Sign up for GitHub

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

Support cartesian_bounds aggregation on point and shape #91298

Merged
merged 17 commits into from
Nov 14, 2022
Merged

Support cartesian_bounds aggregation on point and shape #91298

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2af3c58
Support cartesian_bounds aggregation on point and shape
craigtaverner Nov 3, 2022
21d7a72
Update docs/changelog/91298.yaml
craigtaverner Nov 3, 2022
50ddaed
Update docs/changelog/91298.yaml
craigtaverner Nov 3, 2022
f94ee9c
Fixed cartesian parse exceptions to mimic previous behavior
craigtaverner Nov 4, 2022
28e917c
Re-added removed interfaces to fix failing tests
craigtaverner Nov 4, 2022
e4317ae
Generalized AbstractGeoTestCase for use in Cartesian tests
craigtaverner Nov 4, 2022
794cb53
Fixed bug in encoding missing cartesian points
craigtaverner Nov 10, 2022
d40ba6e
Simplify cartesian bounds
craigtaverner Nov 10, 2022
596ee0d
Fixed geo comment
craigtaverner Nov 10, 2022
5b2f9e5
Delete incorrectly added file
craigtaverner Nov 10, 2022
4194cf0
Set correct minimum version for new cartesian aggs
craigtaverner Nov 10, 2022
84cc481
Fixed specific types for ShapeValues
craigtaverner Nov 10, 2022
bb5ad2d
Completed Centroid/Bounds aggregation integration tests refactoring
craigtaverner Nov 10, 2022
ee930e6
Removed licensing comment
craigtaverner Nov 10, 2022
2b502c1
GeoBoundsIT expects wrapLongitude(false)
craigtaverner Nov 14, 2022
c5ff54e
Fixed CartesianBoundsIT flakiness and removed geo-specific code
craigtaverner Nov 14, 2022
f0c9ea2
Reduced CartesianCentroidIT flakiness by normalizing to floats
craigtaverner Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/91298.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 91298
summary: Support `cartesian_bounds` aggregation on point and shape
area: Geo
type: enhancement
issues:
- 90157
Binary file added jffi4926421121997545579.dylib
View file
Open in desktop
Binary file not shown.
126 changes: 82 additions & 44 deletions ...c/internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/GeoBoundsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,18 @@

import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.SpatialPoint;
import org.elasticsearch.common.util.BigArray;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.bucket.global.Global;
import org.elasticsearch.search.aggregations.bucket.terms.Terms;
import org.elasticsearch.search.aggregations.bucket.terms.Terms.Bucket;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.geo.RandomGeoGenerator;

import java.util.List;

import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.search.aggregations.AggregationBuilders.geoBounds;
import static org.elasticsearch.search.aggregations.AggregationBuilders.global;
import static org.elasticsearch.search.aggregations.AggregationBuilders.terms;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
Expand All @@ -43,15 +44,15 @@ public void testSingleValuedField() throws Exception {

assertSearchResponse(response);

GeoBounds geoBounds = response.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be use here GeoBounds instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was in the process of refactoring. I'll finish the job and then you can decide.

assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
GeoPoint bottomRight = geoBounds.bottomRight();
assertThat(topLeft.lat(), closeTo(singleTopLeft.lat(), GEOHASH_TOLERANCE));
assertThat(topLeft.lon(), closeTo(singleTopLeft.lon(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lat(), closeTo(singleBottomRight.lat(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lon(), closeTo(singleBottomRight.lon(), GEOHASH_TOLERANCE));
assertThat(topLeft.getY(), closeTo(singleTopLeft.getY(), GEOHASH_TOLERANCE));
assertThat(topLeft.getX(), closeTo(singleTopLeft.getX(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getY(), closeTo(singleBottomRight.getY(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getX(), closeTo(singleBottomRight.getX(), GEOHASH_TOLERANCE));
}

public void testSingleValuedField_getProperty() throws Exception {
Expand All @@ -69,25 +70,28 @@ public void testSingleValuedField_getProperty() throws Exception {
assertThat(global.getAggregations(), notNullValue());
assertThat(global.getAggregations().asMap().size(), equalTo(1));

GeoBounds geobounds = global.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geobounds = global.getAggregations().get(aggName);
Copy link
Contributor

Choose a reason for hiding this comment

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

The same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this for the two dateline tests, but all the rest moved into shared code.

assertThat(geobounds, notNullValue());
assertThat(geobounds.getName(), equalTo(aggName));
assertThat((GeoBounds) ((InternalAggregation) global).getProperty(aggName), sameInstance(geobounds));
assertThat((SpatialBounds<?>) ((InternalAggregation) global).getProperty(aggName), sameInstance(geobounds));
GeoPoint topLeft = geobounds.topLeft();
GeoPoint bottomRight = geobounds.bottomRight();
assertThat(topLeft.lat(), closeTo(singleTopLeft.lat(), GEOHASH_TOLERANCE));
assertThat(topLeft.lon(), closeTo(singleTopLeft.lon(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lat(), closeTo(singleBottomRight.lat(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lon(), closeTo(singleBottomRight.lon(), GEOHASH_TOLERANCE));
assertThat((double) ((InternalAggregation) global).getProperty(aggName + ".top"), closeTo(singleTopLeft.lat(), GEOHASH_TOLERANCE));
assertThat((double) ((InternalAggregation) global).getProperty(aggName + ".left"), closeTo(singleTopLeft.lon(), GEOHASH_TOLERANCE));
assertThat(topLeft.getY(), closeTo(singleTopLeft.getY(), GEOHASH_TOLERANCE));
assertThat(topLeft.getX(), closeTo(singleTopLeft.getX(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getY(), closeTo(singleBottomRight.getY(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getX(), closeTo(singleBottomRight.getX(), GEOHASH_TOLERANCE));
assertThat((double) ((InternalAggregation) global).getProperty(aggName + ".top"), closeTo(singleTopLeft.getY(), GEOHASH_TOLERANCE));
assertThat(
(double) ((InternalAggregation) global).getProperty(aggName + ".left"),
closeTo(singleTopLeft.getX(), GEOHASH_TOLERANCE)
);
assertThat(
(double) ((InternalAggregation) global).getProperty(aggName + ".bottom"),
closeTo(singleBottomRight.lat(), GEOHASH_TOLERANCE)
closeTo(singleBottomRight.getY(), GEOHASH_TOLERANCE)
);
assertThat(
(double) ((InternalAggregation) global).getProperty(aggName + ".right"),
closeTo(singleBottomRight.lon(), GEOHASH_TOLERANCE)
closeTo(singleBottomRight.getX(), GEOHASH_TOLERANCE)
);
}

Expand All @@ -98,15 +102,15 @@ public void testMultiValuedField() throws Exception {

assertSearchResponse(response);

GeoBounds geoBounds = response.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);
Copy link
Contributor

@iverase iverase Nov 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

And here, and every place where we know the concrete class

assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
GeoPoint bottomRight = geoBounds.bottomRight();
assertThat(topLeft.lat(), closeTo(multiTopLeft.lat(), GEOHASH_TOLERANCE));
assertThat(topLeft.lon(), closeTo(multiTopLeft.lon(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lat(), closeTo(multiBottomRight.lat(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lon(), closeTo(multiBottomRight.lon(), GEOHASH_TOLERANCE));
assertThat(topLeft.getY(), closeTo(multiTopLeft.getY(), GEOHASH_TOLERANCE));
assertThat(topLeft.getX(), closeTo(multiTopLeft.getX(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getY(), closeTo(multiBottomRight.getY(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getX(), closeTo(multiBottomRight.getX(), GEOHASH_TOLERANCE));
}

public void testUnmapped() throws Exception {
Expand All @@ -116,7 +120,7 @@ public void testUnmapped() throws Exception {

assertSearchResponse(response);

GeoBounds geoBounds = response.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);
assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
Expand All @@ -132,15 +136,15 @@ public void testPartiallyUnmapped() throws Exception {

assertSearchResponse(response);

GeoBounds geoBounds = response.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);
assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
GeoPoint bottomRight = geoBounds.bottomRight();
assertThat(topLeft.lat(), closeTo(singleTopLeft.lat(), GEOHASH_TOLERANCE));
assertThat(topLeft.lon(), closeTo(singleTopLeft.lon(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lat(), closeTo(singleBottomRight.lat(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lon(), closeTo(singleBottomRight.lon(), GEOHASH_TOLERANCE));
assertThat(topLeft.getY(), closeTo(singleTopLeft.getY(), GEOHASH_TOLERANCE));
assertThat(topLeft.getX(), closeTo(singleTopLeft.getX(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getY(), closeTo(singleBottomRight.getY(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getX(), closeTo(singleBottomRight.getX(), GEOHASH_TOLERANCE));
}

public void testEmptyAggregation() throws Exception {
Expand All @@ -150,7 +154,7 @@ public void testEmptyAggregation() throws Exception {
.get();

assertThat(searchResponse.getHits().getTotalHits().value, equalTo(0L));
GeoBounds geoBounds = searchResponse.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = searchResponse.getAggregations().get(aggName);
assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
Expand All @@ -169,15 +173,15 @@ public void testSingleValuedFieldNearDateLine() throws Exception {
GeoPoint geoValuesTopLeft = new GeoPoint(38, -179);
GeoPoint geoValuesBottomRight = new GeoPoint(-24, 178);

GeoBounds geoBounds = response.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);
assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
GeoPoint bottomRight = geoBounds.bottomRight();
assertThat(topLeft.lat(), closeTo(geoValuesTopLeft.lat(), GEOHASH_TOLERANCE));
assertThat(topLeft.lon(), closeTo(geoValuesTopLeft.lon(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lat(), closeTo(geoValuesBottomRight.lat(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lon(), closeTo(geoValuesBottomRight.lon(), GEOHASH_TOLERANCE));
assertThat(topLeft.getY(), closeTo(geoValuesTopLeft.getY(), GEOHASH_TOLERANCE));
assertThat(topLeft.getX(), closeTo(geoValuesTopLeft.getX(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getY(), closeTo(geoValuesBottomRight.getY(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getX(), closeTo(geoValuesBottomRight.getX(), GEOHASH_TOLERANCE));
}

public void testSingleValuedFieldNearDateLineWrapLongitude() throws Exception {
Expand All @@ -190,15 +194,15 @@ public void testSingleValuedFieldNearDateLineWrapLongitude() throws Exception {

assertSearchResponse(response);

GeoBounds geoBounds = response.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);
assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
GeoPoint bottomRight = geoBounds.bottomRight();
assertThat(topLeft.lat(), closeTo(geoValuesTopLeft.lat(), GEOHASH_TOLERANCE));
assertThat(topLeft.lon(), closeTo(geoValuesTopLeft.lon(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lat(), closeTo(geoValuesBottomRight.lat(), GEOHASH_TOLERANCE));
assertThat(bottomRight.lon(), closeTo(geoValuesBottomRight.lon(), GEOHASH_TOLERANCE));
assertThat(topLeft.getY(), closeTo(geoValuesTopLeft.getY(), GEOHASH_TOLERANCE));
assertThat(topLeft.getX(), closeTo(geoValuesTopLeft.getX(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getY(), closeTo(geoValuesBottomRight.getY(), GEOHASH_TOLERANCE));
assertThat(bottomRight.getX(), closeTo(geoValuesBottomRight.getX(), GEOHASH_TOLERANCE));
}

/**
Expand All @@ -223,7 +227,7 @@ public void testSingleValuedFieldAsSubAggToHighCardTermsAgg() {
Bucket bucket = buckets.get(i);
assertThat(bucket, notNullValue());
assertThat("InternalBucket " + bucket.getKey() + " has wrong number of documents", bucket.getDocCount(), equalTo(1L));
GeoBounds geoBounds = bucket.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = bucket.getAggregations().get(aggName);
assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
assertThat(geoBounds.topLeft().getLat(), allOf(greaterThanOrEqualTo(-90.0), lessThanOrEqualTo(90.0)));
Expand All @@ -240,14 +244,48 @@ public void testSingleValuedFieldWithZeroLon() throws Exception {

assertSearchResponse(response);

GeoBounds geoBounds = response.getAggregations().get(aggName);
SpatialBounds<GeoPoint> geoBounds = response.getAggregations().get(aggName);
assertThat(geoBounds, notNullValue());
assertThat(geoBounds.getName(), equalTo(aggName));
GeoPoint topLeft = geoBounds.topLeft();
GeoPoint bottomRight = geoBounds.bottomRight();
assertThat(topLeft.lat(), closeTo(1.0, GEOHASH_TOLERANCE));
assertThat(topLeft.lon(), closeTo(0.0, GEOHASH_TOLERANCE));
assertThat(bottomRight.lat(), closeTo(1.0, GEOHASH_TOLERANCE));
assertThat(bottomRight.lon(), closeTo(0.0, GEOHASH_TOLERANCE));
assertThat(topLeft.getY(), closeTo(1.0, GEOHASH_TOLERANCE));
assertThat(topLeft.getX(), closeTo(0.0, GEOHASH_TOLERANCE));
assertThat(bottomRight.getY(), closeTo(1.0, GEOHASH_TOLERANCE));
assertThat(bottomRight.getX(), closeTo(0.0, GEOHASH_TOLERANCE));
}

public static GeoBoundsAggregationBuilder geoBounds(String name) {
return new GeoBoundsAggregationBuilder(name);
}

@Override
protected String fieldTypeName() {
return "geo_point";
}

@Override
protected GeoPoint makePoint(double x, double y) {
return new GeoPoint(y, x);
}

@Override
protected GeoPoint randomPoint() {
return RandomGeoGenerator.randomPoint(random());
}

@Override
protected void resetX(SpatialPoint point, double x) {
((GeoPoint) point).resetLon(x);
}

@Override
protected void resetY(SpatialPoint point, double y) {
((GeoPoint) point).resetLat(y);
}

@Override
protected GeoPoint reset(SpatialPoint point, double x, double y) {
return ((GeoPoint) point).reset(y, x);
}
}
57 changes: 49 additions & 8 deletions ...internalClusterTest/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,16 @@

import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.SpatialPoint;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGrid;
import org.elasticsearch.search.aggregations.bucket.global.Global;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.test.geo.RandomGeoGenerator;

import java.util.List;

import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.search.aggregations.AggregationBuilders.geoCentroid;
import static org.elasticsearch.search.aggregations.AggregationBuilders.geohashGrid;
import static org.elasticsearch.search.aggregations.AggregationBuilders.global;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
Expand Down Expand Up @@ -109,15 +110,21 @@ public void testSingleValueFieldGetProperty() {
assertThat((GeoCentroid) ((InternalAggregation) global).getProperty(aggName), sameInstance(geoCentroid));
assertSameCentroid(geoCentroid.centroid(), singleCentroid);
assertThat(
((GeoPoint) ((InternalAggregation) global).getProperty(aggName + ".value")).lat(),
closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE)
((GeoPoint) ((InternalAggregation) global).getProperty(aggName + ".value")).getY(),
closeTo(singleCentroid.getY(), GEOHASH_TOLERANCE)
);
assertThat(
((GeoPoint) ((InternalAggregation) global).getProperty(aggName + ".value")).lon(),
closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE)
((GeoPoint) ((InternalAggregation) global).getProperty(aggName + ".value")).getX(),
closeTo(singleCentroid.getX(), GEOHASH_TOLERANCE)
);
assertThat(
(double) ((InternalAggregation) global).getProperty(aggName + ".lat"),
closeTo(singleCentroid.getY(), GEOHASH_TOLERANCE)
);
assertThat(
(double) ((InternalAggregation) global).getProperty(aggName + ".lon"),
closeTo(singleCentroid.getX(), GEOHASH_TOLERANCE)
);
assertThat((double) ((InternalAggregation) global).getProperty(aggName + ".lat"), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
assertThat((double) ((InternalAggregation) global).getProperty(aggName + ".lon"), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
assertEquals(numDocs, (long) ((InternalAggregation) global).getProperty(aggName + ".count"));
}

Expand Down Expand Up @@ -149,9 +156,43 @@ public void testSingleValueFieldAsSubAggToGeohashGrid() {
List<? extends GeoGrid.Bucket> buckets = grid.getBuckets();
for (GeoGrid.Bucket cell : buckets) {
String geohash = cell.getKeyAsString();
GeoPoint expectedCentroid = expectedCentroidsForGeoHash.get(geohash);
SpatialPoint expectedCentroid = expectedCentroidsForGeoHash.get(geohash);
GeoCentroid centroidAgg = cell.getAggregations().get(aggName);
assertSameCentroid(centroidAgg.centroid(), expectedCentroid);
}
}

public static GeoCentroidAggregationBuilder geoCentroid(String name) {
return new GeoCentroidAggregationBuilder(name);
}

@Override
protected String fieldTypeName() {
return "geo_point";
}

@Override
protected GeoPoint makePoint(double x, double y) {
return new GeoPoint(y, x);
}

@Override
protected GeoPoint randomPoint() {
return RandomGeoGenerator.randomPoint(random());
}

@Override
protected void resetX(SpatialPoint point, double x) {
((GeoPoint) point).resetLon(x);
}

@Override
protected void resetY(SpatialPoint point, double y) {
((GeoPoint) point).resetLat(y);
}

@Override
protected GeoPoint reset(SpatialPoint point, double x, double y) {
return ((GeoPoint) point).reset(y, x);
}
}
14 changes: 1 addition & 13 deletions server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoBounds.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,8 @@
package org.elasticsearch.search.aggregations.metrics;

import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.search.aggregations.Aggregation;

/**
* An aggregation that computes a bounding box in which all documents of the current bucket are.
*/
public interface GeoBounds extends Aggregation {

/**
* Get the top-left location of the bounding box.
*/
GeoPoint topLeft();

/**
* Get the bottom-right location of the bounding box.
*/
GeoPoint bottomRight();
}
public interface GeoBounds extends SpatialBounds<GeoPoint> {}
Loading