Fix bug when creating empty geo_lines (elastic#97509)

During reduce, only merge as non-overlapping if all geo-lines are non-overlapping. --------- Co-authored-by: Craig Taverner <craig@amanzi.com>
iverase · Jul 10, 2023 · 6e562f4 · 6e562f4
1 parent c35543e
commit 6e562f4
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 5 deletions.
diff --git a/docs/changelog/97509.yaml 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/...al/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java b/...al/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/InternalGeoLine.java
@@ -117,20 +117,25 @@ public InternalAggregation reduce(List<InternalAggregation> aggregations, Aggreg
         int mergedSize = 0;
         boolean reducedComplete = true;
         boolean reducedIncludeSorts = true;
+        boolean reducedNonOverlapping = this.nonOverlapping;
+        boolean reducedSimplified = this.simplified;
         List<InternalGeoLine> internalGeoLines = new ArrayList<>(aggregations.size());
         for (InternalAggregation aggregation : aggregations) {
             InternalGeoLine geoLine = (InternalGeoLine) aggregation;
             internalGeoLines.add(geoLine);
             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/...rc/test/java/org/elasticsearch/xpack/spatial/search/aggregations/MergedGeoLinesTests.java b/...rc/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<InternalGeoLine> sorted = makeGeoLines(docsPerLine, numLines, simplify, sortOrder);
+            // Shuffle to ensure the tests cover geo_lines coming from data nodes in random order
+            List<InternalGeoLine> shuffled = shuffleGeoLines(sorted);
+            ArrayList<InternalAggregation> 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<InternalGeoLine> lines, int docsPerLine, int finalLength, MergedGeoLines mergedGeoLines) {
         double[] values = mergedGeoLines.getFinalSortValues();
         long[] points = mergedGeoLines.getFinalPoints();
@@ -162,8 +186,10 @@ private void assertLinesTruncated(List<InternalGeoLine> lines, int docsPerLine,
     }
 
     private void assertLinesSimplified(List<InternalGeoLine> 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<InternalGeoLine> 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);