From fdf980c8d0292673fe714d2f5ec33f3ed52dba38 Mon Sep 17 00:00:00 2001 From: Ignacio Vera Date: Tue, 11 Jul 2023 07:54:44 +0800 Subject: [PATCH] Fix bug when creating empty geo_lines (#97509) During reduce, only merge as non-overlapping if all geo-lines are non-overlapping. --------- Co-authored-by: Craig Taverner --- docs/changelog/97509.yaml | 6 ++++ .../search/aggregations/InternalGeoLine.java | 11 +++++-- .../aggregations/MergedGeoLinesTests.java | 30 +++++++++++++++++-- 3 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 docs/changelog/97509.yaml diff --git a/docs/changelog/97509.yaml b/docs/changelog/97509.yaml new file mode 100644 index 0000000000000..dd371f65fd658 --- /dev/null +++ b/docs/changelog/97509.yaml @@ -0,0 +1,6 @@ +pr: 97509 +summary: Fix bug when creating empty `geo_lines` +area: Geo +type: bug +issues: + - 97311 diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java index a520e84a9918c..e8a210549d0b7 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java @@ -116,6 +116,8 @@ public InternalAggregation reduce(List aggregations, Aggreg int mergedSize = 0; boolean reducedComplete = true; boolean reducedIncludeSorts = true; + boolean reducedNonOverlapping = this.nonOverlapping; + boolean reducedSimplified = this.simplified; List internalGeoLines = new ArrayList<>(aggregations.size()); for (InternalAggregation aggregation : aggregations) { InternalGeoLine geoLine = (InternalGeoLine) aggregation; @@ -123,13 +125,16 @@ public InternalAggregation reduce(List aggregations, Aggreg mergedSize += geoLine.line.length; reducedComplete &= geoLine.complete; reducedIncludeSorts &= geoLine.includeSorts; + reducedNonOverlapping &= geoLine.nonOverlapping; + reducedSimplified |= geoLine.simplified; } reducedComplete &= mergedSize <= size; int finalSize = Math.min(mergedSize, size); - MergedGeoLines mergedGeoLines = nonOverlapping - ? new MergedGeoLines.NonOverlapping(internalGeoLines, finalSize, sortOrder, simplified) - : new MergedGeoLines.Overlapping(internalGeoLines, finalSize, sortOrder, simplified); + // If all geo_lines are marked as non-overlapping, then we can optimize the merge-sort + MergedGeoLines mergedGeoLines = reducedNonOverlapping + ? new MergedGeoLines.NonOverlapping(internalGeoLines, finalSize, sortOrder, reducedSimplified) + : new MergedGeoLines.Overlapping(internalGeoLines, finalSize, sortOrder, reducedSimplified); mergedGeoLines.merge(); return new InternalGeoLine( name, diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/MergedGeoLinesTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/MergedGeoLinesTests.java index 67287a5e72daa..40321847048ed 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/MergedGeoLinesTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/MergedGeoLinesTests.java @@ -8,16 +8,19 @@ import org.apache.lucene.geo.GeoEncodingUtils; import org.elasticsearch.common.util.ArrayUtils; +import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.test.ESTestCase; import org.hamcrest.CoreMatchers; import org.hamcrest.Matcher; +import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Map; import java.util.TreeSet; import static org.hamcrest.Matchers.equalTo; @@ -145,6 +148,27 @@ public void testMergeNonOverlappingGeoLinesAppendAndSimplify() { } } + public void testEmptyAndNonOverlappingGeoLines() throws IOException { + int docsPerLine = 10; + int numLines = 10; + int finalLength = 25; // should get entire 100 points simplified down to 25 + boolean simplify = true; + for (SortOrder sortOrder : new SortOrder[] { SortOrder.ASC, SortOrder.DESC }) { + InternalAggregation empty = makeEmptyGeoLine(sortOrder, finalLength); + List sorted = makeGeoLines(docsPerLine, numLines, simplify, sortOrder); + // Shuffle to ensure the tests cover geo_lines coming from data nodes in random order + List shuffled = shuffleGeoLines(sorted); + ArrayList aggregations = new ArrayList<>(shuffled); + InternalGeoLine reduced = (InternalGeoLine) empty.reduce(aggregations, null); + assertLinesSimplified(sorted, sortOrder, finalLength, reduced.sortVals(), reduced.line()); + } + } + + private InternalGeoLine makeEmptyGeoLine(SortOrder sortOrder, int size) throws IOException { + // Make sure this matches the contents of 'GeoLineAggregator.buildEmptyAggregation' + return new InternalGeoLine("test", new long[0], new double[0], Map.of(), true, randomBoolean(), sortOrder, size, true, false); + } + private void assertLinesTruncated(List lines, int docsPerLine, int finalLength, MergedGeoLines mergedGeoLines) { double[] values = mergedGeoLines.getFinalSortValues(); long[] points = mergedGeoLines.getFinalPoints(); @@ -162,8 +186,10 @@ private void assertLinesTruncated(List lines, int docsPerLine, } private void assertLinesSimplified(List lines, SortOrder sortOrder, int finalLength, MergedGeoLines mergedGeoLines) { - double[] values = mergedGeoLines.getFinalSortValues(); - long[] points = mergedGeoLines.getFinalPoints(); + assertLinesSimplified(lines, sortOrder, finalLength, mergedGeoLines.getFinalSortValues(), mergedGeoLines.getFinalPoints()); + } + + private void assertLinesSimplified(List lines, SortOrder sortOrder, int finalLength, double[] values, long[] points) { assertThat("Same length arrays", values.length, equalTo(points.length)); assertThat("Geo-line is simplified", values.length, equalTo(finalLength)); GeoLineAggregatorTests.TestLine simplified = makeSimplifiedLine(lines, sortOrder, finalLength);