From 4d98839a1ff7bed8325552cac17350451234adec Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Fri, 13 Jan 2023 10:46:55 +0100 Subject: [PATCH 1/6] Alternative algorithm for inflated bounds The BoundedGeoHexGridTiler inflates the bounding box for parent cell searches since the parent cells bounds do not match the descendents. This algorithm does a slightly more refined calculation of the inflation factor by considering all four corners and inflating in the four directions independently. --- .../bucket/geogrid/GeoHexGridTiler.java | 126 ++++++- ...BoundedGeoHexGridTilerAllCornersTests.java | 47 +++ .../BoundedGeoHexGridTilerMaxWidthTests.java | 44 +++ .../geogrid/BoundedGeoHexGridTilerTests.java | 307 ++++++++++++++++++ 4 files changed, 509 insertions(+), 15 deletions(-) create mode 100644 x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java create mode 100644 x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java create mode 100644 x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java index c773ea7cab01f..89948900385ae 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java @@ -18,6 +18,8 @@ import java.io.IOException; +import static org.elasticsearch.common.geo.GeoUtils.centeredModulus; + /** * Implements the logic for the GeoHex aggregation over a geoshape doc value. */ @@ -235,7 +237,7 @@ static long calcMaxAddresses(int precision) { * parent cells, since child cells can exceed the bounds of their parent. We inflate the bounds * by half of the width and half of the height. */ - private static class BoundedGeoHexGridTiler extends GeoHexGridTiler { + static class BoundedGeoHexGridTiler extends GeoHexGridTiler { private final GeoBoundingBox[] inflatedBboxes; private final GeoBoundingBox bbox; private final GeoHexVisitor visitor; @@ -249,20 +251,22 @@ private static class BoundedGeoHexGridTiler extends GeoHexGridTiler { this.resolution = resolution; inflatedBboxes = new GeoBoundingBox[resolution]; for (int i = 0; i < resolution; i++) { - inflatedBboxes[i] = inflateBbox(i, bbox); + inflatedBboxes[i] = inflateBbox(i, bbox, FACTOR); } } - private static GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox) { - /* - * Here is the tricky part of this approach. We need to be able to filter cells at higher precisions - * because they are not in bounds, but we need to make sure we don't filter too much. - * - * We use h3 bins at the given resolution to check the height ands width at that level, and we add half of it. - * - * The values have been tune using test GeoHexTilerTests#testLargeShapeWithBounds - */ - final double factor = FACTOR; + /** + * Since H3 cells do not fully contain their child cells, we need to take care that when + * filtering cells at a lower precision than the final precision, we must not exclude + * parents that do not match the filter, but their own children or descendents might match. + * For this reason the filter needs to be expanded to cover all descendent cells. + * + * This is done by taking the H3 cells at two corners, and expanding the filter width + * by 50% of the max width of those cells, and filter height by 50% of the max height of those cells. + * + * The inflation factor of 50% has been verified using test GeoHexTilerTests#testLargeShapeWithBounds + */ + static GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor) { final Rectangle minMin = H3CartesianUtil.toBoundingBox(H3.geoToH3(bbox.bottom(), bbox.left(), precision)); final Rectangle maxMax = H3CartesianUtil.toBoundingBox(H3.geoToH3(bbox.top(), bbox.right(), precision)); // compute height and width at the given precision @@ -281,11 +285,11 @@ private static GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox) { } } - private static double height(Rectangle rectangle) { + static double height(Rectangle rectangle) { return rectangle.getMaxY() - rectangle.getMinY(); } - private static double width(Rectangle rectangle) { + static double width(Rectangle rectangle) { if (rectangle.getMinX() > rectangle.getMaxX()) { return 360d + rectangle.getMaxX() - rectangle.getMinX(); } else { @@ -293,7 +297,7 @@ private static double width(Rectangle rectangle) { } } - private static double width(GeoBoundingBox bbox) { + static double width(GeoBoundingBox bbox) { if (bbox.left() > bbox.right()) { return 360d + bbox.right() - bbox.left(); } else { @@ -301,6 +305,98 @@ private static double width(GeoBoundingBox bbox) { } } + /** + * Since H3 cells do not fully contain their child cells, we need to take care that when + * filtering cells at a lower precision than the final precision, we must not exclude + * parents that do not match the filter, but their own children or descendents might match. + * For this reason the filter needs to be expanded to cover all descendent cells. + * + * This is done by taking the H3 cells at the four corners, and expanding the filter + * to cover 50% of the height and width of those cells. + * + * The inflation factor of 50% has been verified using test GeoHexTilerTests#testLargeShapeWithBounds + */ + static GeoBoundingBox inflateBbox2(final int precision, final GeoBoundingBox bbox, final double factor) { + final long minMinCell = H3.geoToH3(bbox.bottom(), bbox.left(), precision); + final long maxMaxCell = H3.geoToH3(bbox.top(), bbox.right(), precision); + final long minMaxCell = H3.geoToH3(bbox.bottom(), bbox.right(), precision); + final long maxMinCell = H3.geoToH3(bbox.top(), bbox.left(), precision); + // Calculated inflated bounds to cover all cells at all corners + final Rectangle boundsOfCells = boundsOfCells(minMinCell, maxMaxCell, minMaxCell, maxMinCell); + double boundsLeft = boundsOfCells.getMinX(); + double boundsRight = boundsOfCells.getMaxX(); + double bboxLeft = bbox.left(); + double bboxRight = bbox.right(); + boolean boundsCrossesDateline = boundsLeft > boundsRight; + boolean bboxCrossesDateline = bboxLeft > bboxRight; + if (bboxCrossesDateline || boundsCrossesDateline) { + bboxRight = bboxCrossesDateline ? bboxRight + 360 : bboxRight; + boundsRight = boundsCrossesDateline ? boundsRight + 360 : boundsRight; + if (bboxCrossesDateline == false && bboxLeft < 0) { + // Other crosses dateline, but bbox does not, make sure they have comparable signs + bboxLeft += 360; + bboxRight += 360; + } + if (boundsCrossesDateline == false && boundsLeft < 0) { + // Bbox crosses dateline, but bounds does not, make sure they have comparable signs + boundsLeft += 360; + boundsRight += 360; + } + } + // inflate the bbox to a weighted average of the bbox and the cell bounds using the factor + final double minY = Math.max(wAvg(bbox.bottom(), boundsOfCells.getMinY(), factor), -90d); + final double maxY = Math.min(wAvg(bbox.top(), boundsOfCells.getMaxY(), factor), 90d); + final double left = normalizeLon(wAvg(bboxLeft, boundsLeft, factor)); + final double right = normalizeLon(wAvg(bboxRight, boundsRight, factor)); + if (right - left >= 360d) { + // if the total width bigger than the world, then it covers all longitude range. + return new GeoBoundingBox(new GeoPoint(maxY, -180d), new GeoPoint(minY, 180d)); + } else { + return new GeoBoundingBox(new GeoPoint(maxY, left), new GeoPoint(minY, right)); + } + } + + /** Similar to GeoUtils.normalizeLon, but we allow -180 for bounding box min values */ + private static double normalizeLon(double lon) { + if (lon > 180d || lon < -180d) { + lon = centeredModulus(lon, 360); + } + // avoid -0.0 + return lon + 0d; + } + + /** Calculate weighted average: 1 means second dominates, 0 means first dominates */ + static double wAvg(double first, double second, double weight) { + double width = (second - first) * weight; + return first + width; + } + + /** Calling H3CartesianUtil.toBoundingBox has a cost, so we only call it for unique cells */ + private static Rectangle boundsOfCells(long... cells) { + Rectangle bounds = H3CartesianUtil.toBoundingBox(cells[0]); + double minX = bounds.getMinX(); + double maxX = bounds.getMaxX(); + double minY = bounds.getMinY(); + double maxY = bounds.getMaxY(); + for (int i = 1; i < cells.length; i++) { + boolean unique = true; + for (int j = 0; j < i; j++) { + if (cells[i] == cells[j]) { + unique = false; + break; + } + } + if (unique) { + bounds = H3CartesianUtil.toBoundingBox(cells[i]); + minX = Math.min(minX, bounds.getMinX()); + maxX = Math.max(maxX, bounds.getMaxX()); + minY = Math.min(minY, bounds.getMinY()); + maxY = Math.max(maxY, bounds.getMaxY()); + } + } + return new Rectangle(minX, maxX, maxY, minY); + } + @Override protected long getMaxCells() { // TODO: Calculate correctly based on bounds diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java new file mode 100644 index 0000000000000..9697bd1f3ecd8 --- /dev/null +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java @@ -0,0 +1,47 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.geometry.Rectangle; + +import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.wAvg; +import static org.hamcrest.Matchers.equalTo; + +/** + * The new algorithm calculates the inflation as a weighted average of the original bounds + * and the bounds of the cells at all four corners of the bbox. + */ +public class BoundedGeoHexGridTilerAllCornersTests extends BoundedGeoHexGridTilerTests { + + @Override + protected GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor) { + return GeoHexGridTiler.BoundedGeoHexGridTiler.inflateBbox2(precision, bbox, factor); + } + + @Override + /* Calculate the bounds of the h3 cell assuming the test bbox is entirely within the cell */ + protected Rectangle getFullBounds(Rectangle bounds, GeoBoundingBox bbox) { + return bounds; + } + + public void testBoundedTilerInflation_WeightedAverages() { + assertThat("Weighted average 0.5 (pos)", wAvg(10, 20, 0.5), equalTo(15.0)); + assertThat("Weighted average 0.5 (neg)", wAvg(-10, -20, 0.5), equalTo(-15.0)); + assertThat("Weighted average 0.5", wAvg(-10, 10, 0.5), equalTo(0.0)); + assertThat("Weighted average 0.0 (pos)", wAvg(10, 20, 0.0), equalTo(10.0)); + assertThat("Weighted average 0.0 (neg)", wAvg(-10, -20, 0.0), equalTo(-10.0)); + assertThat("Weighted average 0.0", wAvg(-10, 10, 0.0), equalTo(-10.0)); + assertThat("Weighted average 1.0 (pos)", wAvg(10, 20, 1.0), equalTo(20.0)); + assertThat("Weighted average 1.0 (neg)", wAvg(-10, -20, 1.0), equalTo(-20.0)); + assertThat("Weighted average 1.0", wAvg(-10, 10, 1.0), equalTo(10.0)); + assertThat("Weighted average 2.0 (pos)", wAvg(10, 20, 2.0), equalTo(30.0)); + assertThat("Weighted average 2.0 (neg)", wAvg(-10, -20, 2.0), equalTo(-30.0)); + assertThat("Weighted average 2.0", wAvg(-10, 10, 2.0), equalTo(30.0)); + } +} diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java new file mode 100644 index 0000000000000..726e2dd9d0102 --- /dev/null +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java @@ -0,0 +1,44 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoUtils; +import org.elasticsearch.geometry.Rectangle; + +import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.height; +import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.width; + +/** + * The original algorithm inflates using the max size of cells at two corners of the bbox. + */ +public class BoundedGeoHexGridTilerMaxWidthTests extends BoundedGeoHexGridTilerTests { + + @Override + protected GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor) { + return GeoHexGridTiler.BoundedGeoHexGridTiler.inflateBbox(precision, bbox, factor); + } + + @Override + /* Calculate the bounds of the h3 cell assuming the test bbox is entirely within the cell */ + protected Rectangle getFullBounds(Rectangle bounds, GeoBoundingBox bbox) { + final double height = height(bounds); + final double width = width(bounds); + // inflate the coordinates by the full width and height + final double minY = Math.max(bbox.bottom() - height, -90d); + final double maxY = Math.min(bbox.top() + height, 90d); + final double left = GeoUtils.normalizeLon(bbox.left() - width); + final double right = GeoUtils.normalizeLon(bbox.right() + width); + if (2 * width + width(bbox) >= 360d) { + // if the total width is bigger than the world, then it covers all longitude range. + return new Rectangle(-180, 180, maxY, minY); + } else { + return new Rectangle(left, right, maxY, minY); + } + } +} diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java new file mode 100644 index 0000000000000..75c24f568112a --- /dev/null +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java @@ -0,0 +1,307 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.geometry.Point; +import org.elasticsearch.geometry.Rectangle; +import org.elasticsearch.h3.H3; +import org.elasticsearch.h3.LatLng; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.spatial.common.H3CartesianUtil; +import org.hamcrest.BaseMatcher; +import org.hamcrest.Description; + +import static org.elasticsearch.common.geo.GeoUtils.normalizeLon; +import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.wAvg; +import static org.hamcrest.Matchers.closeTo; +import static org.hamcrest.Matchers.greaterThan; +import static org.hamcrest.Matchers.lessThan; +import static org.hamcrest.Matchers.not; + +public abstract class BoundedGeoHexGridTilerTests extends ESTestCase { + + /** Call the appropriate algorithms for inflating the bounding box */ + protected abstract GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor); + + /** Calculate the bounds of the h3 cell assuming the test bbox is entirely within the cell */ + protected abstract Rectangle getFullBounds(Rectangle bounds, GeoBoundingBox bbox); + + public void testBoundedTilerInflation() { + for (int i = 0; i < 10000; i++) { + Point point = GeometryTestUtils.randomPoint(); + int precision = random().nextInt(0, 1); + assertH3CellInflation(point, precision); + } + } + + /** Test with cell bounds crossing dateline */ + public void testBoundedTilerInflation_805bfffffffffff() { + for (int precision = 0; precision < 2; precision++) { + long h3 = H3.stringToH3("805bfffffffffff"); + LatLng centroid = H3.h3ToLatLng(h3); + assertH3CellInflation(new Point(centroid.getLonDeg(), centroid.getLatDeg()), precision); + } + } + + /** Test for north pole with bounds width 360 */ + public void testBoundedTilerInflation_8001fffffffffff() { + for (int precision = 0; precision < 2; precision++) { + long h3 = H3.stringToH3("8001fffffffffff"); + LatLng centroid = H3.h3ToLatLng(h3); + assertH3CellInflation(new Point(centroid.getLonDeg(), centroid.getLatDeg()), precision); + } + } + + /** Test with inner bbox crossing dateline */ + public void testBoundedTilerInflation_80bbfffffffffff() { + for (int precision = 0; precision < 2; precision++) { + long h3 = H3.stringToH3("80bbfffffffffff"); + LatLng centroid = H3.h3ToLatLng(h3); + assertH3CellInflation(new Point(centroid.getLonDeg(), centroid.getLatDeg()), precision); + } + } + + /** Test with inner bbox on different side of dateline than inflated bbox */ + public void testBoundedTilerInflation_80ebfffffffffff() { + for (int precision = 0; precision < 2; precision++) { + long h3 = H3.stringToH3("80ebfffffffffff"); + LatLng centroid = H3.h3ToLatLng(h3); + assertH3CellInflation(new Point(centroid.getLonDeg(), centroid.getLatDeg()), precision); + } + } + + /** Test with inner bbox and cell crossing line of zero longitude */ + public void testBoundedTilerInflation_8075fffffffffff() { + for (int precision = 0; precision < 2; precision++) { + long h3 = H3.stringToH3("8075fffffffffff"); + LatLng centroid = H3.h3ToLatLng(h3); + assertH3CellInflation(new Point(centroid.getLonDeg(), centroid.getLatDeg()), precision); + } + } + + private void assertH3CellInflation(Point point, int precision) { + long h3 = H3.geoToH3(point.getLat(), point.getLon(), precision); + LatLng centroid = H3.h3ToLatLng(h3); + String cell = "H3[" + H3.h3ToString(h3) + "]:" + precision; + Rectangle cellBounds = H3CartesianUtil.toBoundingBox(h3); + boolean cellCrossesDateline = cellBounds.getMinX() > cellBounds.getMaxX(); + double maxX = cellCrossesDateline ? 360 + cellBounds.getMaxX() : cellBounds.getMaxX(); + double centroidLeft = centroid.getLonDeg(); + double centroidRight = centroid.getLonDeg(); + if (cellCrossesDateline) { + centroidLeft = normLonAgainst(centroid.getLonDeg(), cellBounds.getMinX()); + centroidRight = normLonAgainst(centroid.getLonDeg(), maxX); + } + double left = normalizeLon(wAvg(centroidLeft, cellBounds.getMinX(), 0.1)); + double right = normalizeLon(wAvg(centroidRight, maxX, 0.1)); + double top = wAvg(centroid.getLatDeg(), cellBounds.getMaxY(), 0.1); + double bottom = wAvg(centroid.getLatDeg(), cellBounds.getMinY(), 0.1); + GeoBoundingBox bbox = new GeoBoundingBox(new GeoPoint(top, left), new GeoPoint(bottom, right)); + + // Inflating with factor 0.0 should match the original bounding box + GeoBoundingBox inflated0 = inflateBbox(precision, bbox, 0.0); + assertThat(cell + " Full-inflated", inflated0, matchesBounds(bbox)); + + // Test inflation factors 1.0 should match the outer cell bounds + GeoBoundingBox inflated1 = inflateBbox(precision, bbox, 1.0); + Rectangle fullyInflated = getFullBounds(cellBounds, bbox); + assertThat(cell + " Full-inflated", inflated1, matchesBounds(fullyInflated)); + + // Now test a range of inflation factors + for (double factor = 0.1; factor <= 0.95; factor += 0.1) { + GeoBoundingBox inflated = inflateBbox(precision, bbox, factor); + // Compare inflated to outer bounds (should be smaller) + assertThat(cell + " Inflated(" + factor + ")", inflated, withinBounds(fullyInflated)); + // Compare inflated to inner bounds (should be larger) + assertThat(cell + " Inflated(" + factor + ")", inflated, containsBounds(bbox)); + } + } + + // We usually got longitude values the same side of the dateline by adding 360, never subtracting 360 + static double normLonAgainst(double value, double other) { + if (other > 0 && value < 0) { + value += 360; + } + return value; + } + + public void testBoundsMatcher() { + GeoBoundingBox bounds = new GeoBoundingBox(new GeoPoint(10, -10), new GeoPoint(-10, 10)); + GeoBoundingBox bigger = new GeoBoundingBox(new GeoPoint(20, -20), new GeoPoint(-20, 20)); + GeoBoundingBox smaller = new GeoBoundingBox(new GeoPoint(5, -5), new GeoPoint(-5, 5)); + GeoBoundingBox intersecting = new GeoBoundingBox(new GeoPoint(20, -20), new GeoPoint(0, 20)); + GeoBoundingBox nonIntersecting = new GeoBoundingBox(new GeoPoint(20, -10), new GeoPoint(15, 10)); + { + assertThat("equals", bounds, matchesBounds(bounds)); + assertThat("contains", bounds, containsBounds(smaller)); + assertThat("within", bounds, withinBounds(bigger)); + } + { + assertThat("not equals", bounds, not(matchesBounds(smaller))); + assertThat("not equals", bounds, not(matchesBounds(bigger))); + assertThat("not equals", bounds, not(matchesBounds(intersecting))); + assertThat("not equals", bounds, not(matchesBounds(nonIntersecting))); + } + { + assertThat("does not contain", bounds, not(containsBounds(bigger))); + assertThat("does not contain", bounds, not(containsBounds(intersecting))); + assertThat("does not contain", bounds, not(containsBounds(nonIntersecting))); + } + { + assertThat("is not within", bounds, not(withinBounds(smaller))); + assertThat("is not within", bounds, not(withinBounds(intersecting))); + assertThat("is not within", bounds, not(withinBounds(nonIntersecting))); + } + } + + private static TestCompareBounds matchesBounds(GeoBoundingBox other) { + return new TestCompareBounds(other, 0); + } + + private static TestCompareBounds matchesBounds(Rectangle other) { + return new TestCompareBounds(other, 0); + } + + private static TestCompareBounds containsBounds(GeoBoundingBox other) { + return new TestCompareBounds(other, 1); + } + + private static TestCompareBounds withinBounds(Rectangle other) { + return new TestCompareBounds(other, -1); + } + + private static TestCompareBounds withinBounds(GeoBoundingBox other) { + return new TestCompareBounds(other, -1); + } + + private static class TestCompareBounds extends BaseMatcher { + + private final GeoBoundingBox other; + private final int comparison; + private boolean matchedTop; + private boolean matchedBottom; + private boolean matchedLeft; + private boolean matchedRight; + + private TestCompareBounds(Rectangle rect, int comparison) { + this.other = new GeoBoundingBox(new GeoPoint(rect.getMaxY(), rect.getMinX()), new GeoPoint(rect.getMinY(), rect.getMaxX())); + this.comparison = comparison; + } + + private TestCompareBounds(GeoBoundingBox other, int comparison) { + this.other = other; + this.comparison = comparison; + } + + @Override + public boolean matches(Object actual) { + if (actual instanceof GeoBoundingBox bbox) { + if (comparison == 0) { + matchedTop = closeTo(bbox.top(), 1e-10).matches(other.top()); + matchedBottom = closeTo(bbox.bottom(), 1e-10).matches(other.bottom()); + matchedLeft = closeTo(posLon(bbox.left()), 1e-10).matches(posLon(other.left())); + matchedRight = closeTo(posLon(bbox.right()), 1e-10).matches(posLon(other.right())); + } else { + if (comparison > 0) { + // assert that 'bbox' is larger than and entirely contains 'other' + setBoxWithinBox(other, bbox); + } else { + // assert that 'bbox' is smaller than and entirely contained within 'other' + setBoxWithinBox(bbox, other); + } + } + return matchedTop && matchedBottom && matchedLeft && matchedRight; + } + return false; + } + + private void setBoxWithinBox(GeoBoundingBox smaller, GeoBoundingBox larger) { + double smallerRight = smaller.right(); + double largerRight = larger.right(); + double smallerLeft = smaller.left(); + double largerLeft = larger.left(); + boolean smallerCrossesDateline = smaller.left() > smaller.right(); + boolean largerCrossesDateline = larger.left() > larger.right(); + if (smallerCrossesDateline || largerCrossesDateline) { + smallerRight = smallerCrossesDateline ? smallerRight + 360 : smallerRight; + largerRight = largerCrossesDateline ? largerRight + 360 : largerRight; + if (smallerCrossesDateline == false && smallerLeft < 0) { + // Larger crosses dateline, but smaller does not, make sure they have comparable signs + smallerLeft += 360; + smallerRight += 360; + } + if (largerCrossesDateline == false && largerLeft < 0) { + // Smaller crosses dateline, but larger does not, make sure they have comparable signs + largerLeft += 360; + largerRight += 360; + } + } + assert smallerLeft < smallerRight; + assert largerLeft < largerRight; + // assert that 'smaller' is smaller than and entirely contained within 'larger' + matchedTop = larger.top() >= 90 || greaterThan(smaller.top()).matches(larger.top()); + matchedBottom = larger.bottom() <= -90 || lessThan(smaller.bottom()).matches(larger.bottom()); + // Only check smaller longitudes are within larger longitudes if larger does not cover all longitudes (eg. polar cells) + boolean largerGlobalLongitude = largerRight - largerLeft >= 360; + matchedLeft = largerGlobalLongitude || lessThan(smallerLeft).matches(largerLeft); + matchedRight = largerGlobalLongitude || greaterThan(smallerRight).matches(largerRight); + } + + private static double posLon(double value) { + if (value < 0) return value + 360; + return value; + } + + @Override + public void describeTo(Description description) { + if (comparison == 0) { + description.appendText("To match " + other); + } else if (comparison > 0) { + description.appendText("To contain " + other); + } else { + description.appendText("To be contained by " + other); + } + } + + @Override + public void describeMismatch(Object item, Description description) { + super.describeMismatch(item, description); + if (item instanceof GeoBoundingBox bbox) { + if (matchedTop == false) { + describeMismatchOf(description, "top", other.top(), bbox.top(), true); + } + if (matchedBottom == false) { + describeMismatchOf(description, "bottom", other.bottom(), bbox.bottom(), false); + } + if (matchedLeft == false) { + describeMismatchOf(description, "left", other.left(), bbox.left(), false); + } + if (matchedRight == false) { + describeMismatchOf(description, "right", other.right(), bbox.right(), true); + } + } + } + + private void describeMismatchOf(Description description, String field, double thisValue, double thatValue, boolean max) { + description.appendText("\n\t and " + field + " "); + description.appendValue(thisValue).appendText(" was not").appendText(describeComparison(max)).appendValue(thatValue); + } + + private String describeComparison(boolean max) { + return switch (comparison) { + case 0 -> " equal to "; + case -1 -> max ? " greater than " : " less than "; + case 1 -> max ? " less than " : " greater than "; + default -> " UNKNOWN "; + }; + } + } +} From f8d6f6009e7f53477181cbacb114bd4a2f9fee19 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 18 Jan 2023 15:55:14 +0100 Subject: [PATCH 2/6] Remove alternative algorithm for bbox inflation The new algorithm was faster than the old for the same inflation factor but had more failures, and a longer tail of failures (which needed higher inflation factors). So it would take a lot more work (probably) to find and fix these edge cases. An easier option is to keep the original algorithm. --- .../bucket/geogrid/GeoHexGridTiler.java | 94 ------------------- ...BoundedGeoHexGridTilerAllCornersTests.java | 47 ---------- .../BoundedGeoHexGridTilerMaxWidthTests.java | 44 --------- .../geogrid/BoundedGeoHexGridTilerTests.java | 41 ++++++-- 4 files changed, 32 insertions(+), 194 deletions(-) delete mode 100644 x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java delete mode 100644 x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java index 89948900385ae..36b0b5b6cf1ba 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java @@ -18,8 +18,6 @@ import java.io.IOException; -import static org.elasticsearch.common.geo.GeoUtils.centeredModulus; - /** * Implements the logic for the GeoHex aggregation over a geoshape doc value. */ @@ -305,98 +303,6 @@ static double width(GeoBoundingBox bbox) { } } - /** - * Since H3 cells do not fully contain their child cells, we need to take care that when - * filtering cells at a lower precision than the final precision, we must not exclude - * parents that do not match the filter, but their own children or descendents might match. - * For this reason the filter needs to be expanded to cover all descendent cells. - * - * This is done by taking the H3 cells at the four corners, and expanding the filter - * to cover 50% of the height and width of those cells. - * - * The inflation factor of 50% has been verified using test GeoHexTilerTests#testLargeShapeWithBounds - */ - static GeoBoundingBox inflateBbox2(final int precision, final GeoBoundingBox bbox, final double factor) { - final long minMinCell = H3.geoToH3(bbox.bottom(), bbox.left(), precision); - final long maxMaxCell = H3.geoToH3(bbox.top(), bbox.right(), precision); - final long minMaxCell = H3.geoToH3(bbox.bottom(), bbox.right(), precision); - final long maxMinCell = H3.geoToH3(bbox.top(), bbox.left(), precision); - // Calculated inflated bounds to cover all cells at all corners - final Rectangle boundsOfCells = boundsOfCells(minMinCell, maxMaxCell, minMaxCell, maxMinCell); - double boundsLeft = boundsOfCells.getMinX(); - double boundsRight = boundsOfCells.getMaxX(); - double bboxLeft = bbox.left(); - double bboxRight = bbox.right(); - boolean boundsCrossesDateline = boundsLeft > boundsRight; - boolean bboxCrossesDateline = bboxLeft > bboxRight; - if (bboxCrossesDateline || boundsCrossesDateline) { - bboxRight = bboxCrossesDateline ? bboxRight + 360 : bboxRight; - boundsRight = boundsCrossesDateline ? boundsRight + 360 : boundsRight; - if (bboxCrossesDateline == false && bboxLeft < 0) { - // Other crosses dateline, but bbox does not, make sure they have comparable signs - bboxLeft += 360; - bboxRight += 360; - } - if (boundsCrossesDateline == false && boundsLeft < 0) { - // Bbox crosses dateline, but bounds does not, make sure they have comparable signs - boundsLeft += 360; - boundsRight += 360; - } - } - // inflate the bbox to a weighted average of the bbox and the cell bounds using the factor - final double minY = Math.max(wAvg(bbox.bottom(), boundsOfCells.getMinY(), factor), -90d); - final double maxY = Math.min(wAvg(bbox.top(), boundsOfCells.getMaxY(), factor), 90d); - final double left = normalizeLon(wAvg(bboxLeft, boundsLeft, factor)); - final double right = normalizeLon(wAvg(bboxRight, boundsRight, factor)); - if (right - left >= 360d) { - // if the total width bigger than the world, then it covers all longitude range. - return new GeoBoundingBox(new GeoPoint(maxY, -180d), new GeoPoint(minY, 180d)); - } else { - return new GeoBoundingBox(new GeoPoint(maxY, left), new GeoPoint(minY, right)); - } - } - - /** Similar to GeoUtils.normalizeLon, but we allow -180 for bounding box min values */ - private static double normalizeLon(double lon) { - if (lon > 180d || lon < -180d) { - lon = centeredModulus(lon, 360); - } - // avoid -0.0 - return lon + 0d; - } - - /** Calculate weighted average: 1 means second dominates, 0 means first dominates */ - static double wAvg(double first, double second, double weight) { - double width = (second - first) * weight; - return first + width; - } - - /** Calling H3CartesianUtil.toBoundingBox has a cost, so we only call it for unique cells */ - private static Rectangle boundsOfCells(long... cells) { - Rectangle bounds = H3CartesianUtil.toBoundingBox(cells[0]); - double minX = bounds.getMinX(); - double maxX = bounds.getMaxX(); - double minY = bounds.getMinY(); - double maxY = bounds.getMaxY(); - for (int i = 1; i < cells.length; i++) { - boolean unique = true; - for (int j = 0; j < i; j++) { - if (cells[i] == cells[j]) { - unique = false; - break; - } - } - if (unique) { - bounds = H3CartesianUtil.toBoundingBox(cells[i]); - minX = Math.min(minX, bounds.getMinX()); - maxX = Math.max(maxX, bounds.getMaxX()); - minY = Math.min(minY, bounds.getMinY()); - maxY = Math.max(maxY, bounds.getMaxY()); - } - } - return new Rectangle(minX, maxX, maxY, minY); - } - @Override protected long getMaxCells() { // TODO: Calculate correctly based on bounds diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java deleted file mode 100644 index 9697bd1f3ecd8..0000000000000 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerAllCornersTests.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid; - -import org.elasticsearch.common.geo.GeoBoundingBox; -import org.elasticsearch.geometry.Rectangle; - -import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.wAvg; -import static org.hamcrest.Matchers.equalTo; - -/** - * The new algorithm calculates the inflation as a weighted average of the original bounds - * and the bounds of the cells at all four corners of the bbox. - */ -public class BoundedGeoHexGridTilerAllCornersTests extends BoundedGeoHexGridTilerTests { - - @Override - protected GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor) { - return GeoHexGridTiler.BoundedGeoHexGridTiler.inflateBbox2(precision, bbox, factor); - } - - @Override - /* Calculate the bounds of the h3 cell assuming the test bbox is entirely within the cell */ - protected Rectangle getFullBounds(Rectangle bounds, GeoBoundingBox bbox) { - return bounds; - } - - public void testBoundedTilerInflation_WeightedAverages() { - assertThat("Weighted average 0.5 (pos)", wAvg(10, 20, 0.5), equalTo(15.0)); - assertThat("Weighted average 0.5 (neg)", wAvg(-10, -20, 0.5), equalTo(-15.0)); - assertThat("Weighted average 0.5", wAvg(-10, 10, 0.5), equalTo(0.0)); - assertThat("Weighted average 0.0 (pos)", wAvg(10, 20, 0.0), equalTo(10.0)); - assertThat("Weighted average 0.0 (neg)", wAvg(-10, -20, 0.0), equalTo(-10.0)); - assertThat("Weighted average 0.0", wAvg(-10, 10, 0.0), equalTo(-10.0)); - assertThat("Weighted average 1.0 (pos)", wAvg(10, 20, 1.0), equalTo(20.0)); - assertThat("Weighted average 1.0 (neg)", wAvg(-10, -20, 1.0), equalTo(-20.0)); - assertThat("Weighted average 1.0", wAvg(-10, 10, 1.0), equalTo(10.0)); - assertThat("Weighted average 2.0 (pos)", wAvg(10, 20, 2.0), equalTo(30.0)); - assertThat("Weighted average 2.0 (neg)", wAvg(-10, -20, 2.0), equalTo(-30.0)); - assertThat("Weighted average 2.0", wAvg(-10, 10, 2.0), equalTo(30.0)); - } -} diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java deleted file mode 100644 index 726e2dd9d0102..0000000000000 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerMaxWidthTests.java +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid; - -import org.elasticsearch.common.geo.GeoBoundingBox; -import org.elasticsearch.common.geo.GeoUtils; -import org.elasticsearch.geometry.Rectangle; - -import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.height; -import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.width; - -/** - * The original algorithm inflates using the max size of cells at two corners of the bbox. - */ -public class BoundedGeoHexGridTilerMaxWidthTests extends BoundedGeoHexGridTilerTests { - - @Override - protected GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor) { - return GeoHexGridTiler.BoundedGeoHexGridTiler.inflateBbox(precision, bbox, factor); - } - - @Override - /* Calculate the bounds of the h3 cell assuming the test bbox is entirely within the cell */ - protected Rectangle getFullBounds(Rectangle bounds, GeoBoundingBox bbox) { - final double height = height(bounds); - final double width = width(bounds); - // inflate the coordinates by the full width and height - final double minY = Math.max(bbox.bottom() - height, -90d); - final double maxY = Math.min(bbox.top() + height, 90d); - final double left = GeoUtils.normalizeLon(bbox.left() - width); - final double right = GeoUtils.normalizeLon(bbox.right() + width); - if (2 * width + width(bbox) >= 360d) { - // if the total width is bigger than the world, then it covers all longitude range. - return new Rectangle(-180, 180, maxY, minY); - } else { - return new Rectangle(left, right, maxY, minY); - } - } -} diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java index 75c24f568112a..08850c982c206 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/BoundedGeoHexGridTilerTests.java @@ -9,6 +9,7 @@ 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; @@ -20,19 +21,14 @@ import org.hamcrest.Description; import static org.elasticsearch.common.geo.GeoUtils.normalizeLon; -import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.wAvg; +import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.height; +import static org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid.GeoHexGridTiler.BoundedGeoHexGridTiler.width; import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.not; -public abstract class BoundedGeoHexGridTilerTests extends ESTestCase { - - /** Call the appropriate algorithms for inflating the bounding box */ - protected abstract GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor); - - /** Calculate the bounds of the h3 cell assuming the test bbox is entirely within the cell */ - protected abstract Rectangle getFullBounds(Rectangle bounds, GeoBoundingBox bbox); +public class BoundedGeoHexGridTilerTests extends ESTestCase { public void testBoundedTilerInflation() { for (int i = 0; i < 10000; i++) { @@ -125,14 +121,41 @@ private void assertH3CellInflation(Point point, int precision) { } } + /** Calculate weighted average: 1 means second dominates, 0 means first dominates */ + private static double wAvg(double first, double second, double weight) { + double width = (second - first) * weight; + return first + width; + } + // We usually got longitude values the same side of the dateline by adding 360, never subtracting 360 - static double normLonAgainst(double value, double other) { + private static double normLonAgainst(double value, double other) { if (other > 0 && value < 0) { value += 360; } return value; } + private GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor) { + return GeoHexGridTiler.BoundedGeoHexGridTiler.inflateBbox(precision, bbox, factor); + } + + /* Calculate the bounds of the h3 cell assuming the test bbox is entirely within the cell */ + private Rectangle getFullBounds(Rectangle bounds, GeoBoundingBox bbox) { + final double height = height(bounds); + final double width = width(bounds); + // inflate the coordinates by the full width and height + final double minY = Math.max(bbox.bottom() - height, -90d); + final double maxY = Math.min(bbox.top() + height, 90d); + final double left = GeoUtils.normalizeLon(bbox.left() - width); + final double right = GeoUtils.normalizeLon(bbox.right() + width); + if (2 * width + width(bbox) >= 360d) { + // if the total width is bigger than the world, then it covers all longitude range. + return new Rectangle(-180, 180, maxY, minY); + } else { + return new Rectangle(left, right, maxY, minY); + } + } + public void testBoundsMatcher() { GeoBoundingBox bounds = new GeoBoundingBox(new GeoPoint(10, -10), new GeoPoint(-10, 10)); GeoBoundingBox bigger = new GeoBoundingBox(new GeoPoint(20, -20), new GeoPoint(-20, 20)); From 99cb5415ac60dbb3e14eef9c3919a4ab47e8bff6 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Wed, 18 Jan 2023 15:59:50 +0100 Subject: [PATCH 3/6] Reduce bbox inflation factor to 0.25 This increases performance by about 10% (a factor of 0.22 increased performance by about 13%). We found the error rate for `testGeoShapeGeoHex` was about 0.5% for inflation factor 0.19 and 0.2% for inflation factor 0.22. These last 2/1000 failed tests required inflation factors of 0.27 0.30 respectively, so we'll investigate them individually. --- .../search/aggregations/bucket/geogrid/GeoHexGridTiler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java index 36b0b5b6cf1ba..f196e91e30987 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java @@ -240,7 +240,7 @@ static class BoundedGeoHexGridTiler extends GeoHexGridTiler { private final GeoBoundingBox bbox; private final GeoHexVisitor visitor; private final int resolution; - private static final double FACTOR = 0.5; + private static final double FACTOR = 0.25; BoundedGeoHexGridTiler(int resolution, GeoBoundingBox bbox) { super(resolution); From 883d38280abe89398a6baa7ef7998084c2b451a1 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Mon, 23 Jan 2023 11:48:07 +0100 Subject: [PATCH 4/6] Increase bbox inflation factor to 0.35 Since there were three failures discovered for 0.25, all of which passed at 0.30, we decided to set the inflation factor to 0.35 initially for the sake of caution, and consider revising it down again later with more careful testing. --- .../search/aggregations/bucket/geogrid/GeoHexGridTiler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java index f196e91e30987..c4ec7552ec8a2 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java @@ -240,7 +240,7 @@ static class BoundedGeoHexGridTiler extends GeoHexGridTiler { private final GeoBoundingBox bbox; private final GeoHexVisitor visitor; private final int resolution; - private static final double FACTOR = 0.25; + private static final double FACTOR = 0.35; BoundedGeoHexGridTiler(int resolution, GeoBoundingBox bbox) { super(resolution); From b959241d164ba45fb67e20550096ffc015074224 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 24 Jan 2023 15:06:49 +0100 Subject: [PATCH 5/6] Fixed comment about bounded tiler inflation --- .../search/aggregations/bucket/geogrid/GeoHexGridTiler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java index c4ec7552ec8a2..c7b6b6931b20f 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java @@ -262,7 +262,7 @@ static class BoundedGeoHexGridTiler extends GeoHexGridTiler { * This is done by taking the H3 cells at two corners, and expanding the filter width * by 50% of the max width of those cells, and filter height by 50% of the max height of those cells. * - * The inflation factor of 50% has been verified using test GeoHexTilerTests#testLargeShapeWithBounds + * The inflation factor of 35% has been verified using test GeoHexTilerTests#testLargeShapeWithBounds */ static GeoBoundingBox inflateBbox(int precision, GeoBoundingBox bbox, double factor) { final Rectangle minMin = H3CartesianUtil.toBoundingBox(H3.geoToH3(bbox.bottom(), bbox.left(), precision)); From 8930a6745e0b9d71e3202059a43cba508f32a629 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 24 Jan 2023 15:08:24 +0100 Subject: [PATCH 6/6] Fixed comment about bounded tiler inflation --- .../search/aggregations/bucket/geogrid/GeoHexGridTiler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java index c7b6b6931b20f..5073cb44dd5d3 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexGridTiler.java @@ -260,7 +260,7 @@ static class BoundedGeoHexGridTiler extends GeoHexGridTiler { * For this reason the filter needs to be expanded to cover all descendent cells. * * This is done by taking the H3 cells at two corners, and expanding the filter width - * by 50% of the max width of those cells, and filter height by 50% of the max height of those cells. + * by 35% of the max width of those cells, and filter height by 35% of the max height of those cells. * * The inflation factor of 35% has been verified using test GeoHexTilerTests#testLargeShapeWithBounds */