Skip to content

Commit

Permalink
Don't generate bounding box touching tiles in GeoTileGridAggregatorTe…
Browse files Browse the repository at this point in the history 
…sts (elastic#101137)
  • Loading branch information
@iverase
iverase authored Oct 25, 2023
1 parent 3111853 commit 126412c
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 32 deletions.
2 changes: 1 addition & 1 deletion ...java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ protected Point randomPoint() {
}

@Override
protected GeoBoundingBox randomBBox() {
protected GeoBoundingBox randomBBox(int precision) {
Rectangle rectangle = GeometryTestUtils.randomRectangle();
return new GeoBoundingBox(
new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()),
Expand Down
59 changes: 42 additions & 17 deletions ...java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@

package org.elasticsearch.search.aggregations.bucket.geogrid;

import org.apache.lucene.geo.GeoEncodingUtils;
import org.apache.lucene.tests.util.LuceneTestCase;
import org.elasticsearch.common.geo.GeoBoundingBox;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.GeoUtils;
import org.elasticsearch.geo.GeometryTestUtils;
import org.elasticsearch.geometry.Point;
import org.elasticsearch.geometry.Rectangle;

@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/101077")
import static org.apache.lucene.geo.GeoEncodingUtils.decodeLatitude;
import static org.apache.lucene.geo.GeoEncodingUtils.decodeLongitude;
import static org.apache.lucene.geo.GeoEncodingUtils.encodeLatitude;
import static org.apache.lucene.geo.GeoEncodingUtils.encodeLongitude;

public class GeoTileGridAggregatorTests extends GeoGridAggregatorTestCase<InternalGeoTileGridBucket> {

@Override
Expand All @@ -39,7 +41,7 @@ protected Point randomPoint() {
}

@Override
protected GeoBoundingBox randomBBox() {
protected GeoBoundingBox randomBBox(int precision) {
GeoBoundingBox bbox = randomValueOtherThanMany(
(b) -> b.top() > GeoTileUtils.LATITUDE_MASK || b.bottom() < -GeoTileUtils.LATITUDE_MASK,
() -> {
Expand All @@ -50,26 +52,49 @@ protected GeoBoundingBox randomBBox() {
);
}
);
// Avoid numerical errors for sub-atomic values
double left = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(bbox.left()));
double right = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(bbox.right()));
double top = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(bbox.top()));
double bottom = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(bbox.bottom()));
final int tiles = 1 << precision;

// Due to the way GeoTileBoundedPredicate works that adjust the given bounding box when it is touching the tiles, we need to
// adjust here in order not to generate bounding boxes touching tiles or the test will fail

// compute tile at the top left
final Rectangle minTile = GeoTileUtils.toBoundingBox(
GeoTileUtils.getXTile(bbox.left(), tiles),
GeoTileUtils.getYTile(bbox.top(), tiles),
precision
);
// adjust if it is touching the tile
final int encodedLeft = encodeLongitude(bbox.left());
final double left = encodeLongitude(minTile.getMaxX()) == encodedLeft
? decodeLongitude(encodedLeft + 1)
: decodeLongitude(encodedLeft);
final int encodedTop = encodeLatitude(bbox.top());
final double bottom = encodeLatitude(minTile.getMinY()) == encodedTop
? decodeLongitude(encodedTop + 1)
: decodeLatitude(encodedTop);
// compute tile at the bottom right
final Rectangle maxTile = GeoTileUtils.toBoundingBox(
GeoTileUtils.getXTile(bbox.right(), tiles),
GeoTileUtils.getYTile(bbox.bottom(), tiles),
precision
);
// adjust if it is touching the tile
final int encodedRight = encodeLongitude(bbox.right());
final double right = encodeLongitude(maxTile.getMinX()) == encodedRight
? decodeLongitude(encodedRight)
: decodeLongitude(encodedRight + 1);
final int encodedBottom = encodeLatitude(bbox.bottom());
final double top = encodeLatitude(maxTile.getMaxY()) == encodedBottom
? decodeLatitude(encodedBottom)
: decodeLatitude(encodedBottom + 1);

bbox.topLeft().reset(top, left);
bbox.bottomRight().reset(bottom, right);
return bbox;
}

@Override
protected Rectangle getTile(double lng, double lat, int precision) {
int tiles = 1 << precision;
int x = GeoTileUtils.getXTile(lng, tiles);
int y = GeoTileUtils.getYTile(lat, tiles);
Rectangle r1 = GeoTileUtils.toBoundingBox(x, y, precision);
Rectangle r2 = GeoTileUtils.toBoundingBox(GeoTileUtils.longEncode(lng, lat, precision));
if (r1.equals(r2) == false) {
int a = 0;
}
return GeoTileUtils.toBoundingBox(GeoTileUtils.longEncode(lng, lat, precision));
}

Expand Down
28 changes: 15 additions & 13 deletions .../java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public abstract class GeoGridAggregatorTestCase<T extends InternalGeoGridBucket>
/**
* Return a random {@link GeoBoundingBox} within the bounds of the tile grid.
*/
protected abstract GeoBoundingBox randomBBox();
protected abstract GeoBoundingBox randomBBox(int precision);

/**
* Return the bounding tile as a {@link Rectangle} for a given point
Expand Down Expand Up @@ -131,23 +131,24 @@ public void testUnmappedMissing() throws IOException {
}

public void testSingletonDocs() throws IOException {
testWithSeveralDocs(() -> true, null);
testWithSeveralDocs(() -> true, null, randomPrecision());
}

public void testBoundedSingletonDocs() throws IOException {
testWithSeveralDocs(() -> true, randomBBox());
int precision = randomPrecision();
testWithSeveralDocs(() -> true, randomBBox(precision), precision);
}

public void testMultiValuedDocs() throws IOException {
testWithSeveralDocs(LuceneTestCase::rarely, null);
testWithSeveralDocs(LuceneTestCase::rarely, null, randomPrecision());
}

public void testBoundedMultiValuedDocs() throws IOException {
testWithSeveralDocs(LuceneTestCase::rarely, randomBBox());
int precision = randomPrecision();
testWithSeveralDocs(LuceneTestCase::rarely, randomBBox(precision), precision);
}

private void testWithSeveralDocs(BooleanSupplier supplier, GeoBoundingBox bbox) throws IOException {
int precision = randomPrecision();
private void testWithSeveralDocs(BooleanSupplier supplier, GeoBoundingBox bbox, int precision) throws IOException {
int numPoints = randomIntBetween(8, 128);
Map<String, Integer> expectedCountPerGeoHash = new HashMap<>();
testCase(new MatchAllDocsQuery(), FIELD_NAME, precision, bbox, geoHashGrid -> {
Expand Down Expand Up @@ -185,23 +186,24 @@ private void testWithSeveralDocs(BooleanSupplier supplier, GeoBoundingBox bbox)
}

public void testSingletonDocsAsSubAgg() throws IOException {
testWithSeveralDocsAsSubAgg(() -> true, null);
testWithSeveralDocsAsSubAgg(() -> true, null, randomPrecision());
}

public void testBoundedSingletonDocsAsSubAgg() throws IOException {
testWithSeveralDocsAsSubAgg(() -> true, randomBBox());
int precision = randomPrecision();
testWithSeveralDocsAsSubAgg(() -> true, randomBBox(precision), precision);
}

public void testMultiValuedDocsAsSubAgg() throws IOException {
testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, null);
testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, null, randomPrecision());
}

public void testBoundedMultiValuedDocsAsSubAgg() throws IOException {
testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, randomBBox());
int precision = randomPrecision();
testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, randomBBox(precision), precision);
}

private void testWithSeveralDocsAsSubAgg(BooleanSupplier supplier, GeoBoundingBox bbox) throws IOException {
int precision = randomPrecision();
private void testWithSeveralDocsAsSubAgg(BooleanSupplier supplier, GeoBoundingBox bbox, int precision) throws IOException {
int numPoints = randomIntBetween(8, 128);
Map<String, Map<String, Long>> expectedCountPerTPerGeoHash = new TreeMap<>();
TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("t").field("t").size(numPoints);
Expand Down
2 changes: 1 addition & 1 deletion ...elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexAggregatorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ protected Point randomPoint() {
}

@Override
protected GeoBoundingBox randomBBox() {
protected GeoBoundingBox randomBBox(int precision) {
GeoBoundingBox bbox = randomValueOtherThanMany(
(b) -> b.top() > GeoTileUtils.LATITUDE_MASK || b.bottom() < -GeoTileUtils.LATITUDE_MASK,
() -> {
Expand Down

0 comments on commit 126412c

Please sign in to comment.