Skip to content

Commit cc776be

Browse files
talevytalevy
authored
Fix geo_line agg behavior with missing values (#69395)
The geo_line aggregation incorrectly handled results with missing values. Tests to verify this behavior were incorrectly checking results. This commit resolves these in three ways - explicitly testing missing sort field values - explicitly testing handling missing point field values - explicitly testing handling mixed scenarios where some documents have one and not the other, or missing both. Fixes elastic/kibana#90653. Fixes #69346.
1 parent 7ad0201 commit cc776be

File tree

2 files changed

+113
-21
lines changed

2 files changed

+113
-21
lines changed

‎x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineBucketedSort.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,13 @@ public long sizeOf(long bucket) {
5353
}
5454
long start = inHeapMode(bucket) ? rootIndex : (rootIndex + getNextGatherOffset(rootIndex) + 1);
5555
long end = rootIndex + bucketSize;
56-
return end - start;
56+
long size = 0;
57+
for (long index = start; index < end; index++) {
58+
if (((Extra) extra).empty.isEmpty(index) == false) {
59+
size += 1;
60+
}
61+
}
62+
return size;
5763
}
5864

5965
/**
@@ -70,11 +76,13 @@ public double[] getSortValues(long bucket) {
7076
}
7177
long start = inHeapMode(bucket) ? rootIndex : (rootIndex + getNextGatherOffset(rootIndex) + 1);
7278
long end = rootIndex + bucketSize;
73-
double[] result = new double[(int)(end - start)];
79+
double[] result = new double[(int) sizeOf(bucket)];
7480
int i = 0;
7581
for (long index = start; index < end; index++) {
76-
double timestampValue = ((DoubleArray)values()).get(index);
77-
result[i++] = timestampValue;
82+
if (((Extra) extra).empty.isEmpty(index) == false) {
83+
double timestampValue = ((DoubleArray)values()).get(index);
84+
result[i++] = timestampValue;
85+
}
7886
}
7987
return result;
8088
}
@@ -92,11 +100,13 @@ public long[] getPoints(long bucket) {
92100
}
93101
long start = inHeapMode(bucket) ? rootIndex : (rootIndex + getNextGatherOffset(rootIndex) + 1);
94102
long end = rootIndex + bucketSize;
95-
long[] result = new long[(int)(end - start)];
103+
long[] result = new long[(int) sizeOf(bucket)];
96104
int i = 0;
97105
for (long index = start; index < end; index++) {
98-
long geoPointValue = ((Extra) extra).values.get(index);
99-
result[i++] = geoPointValue;
106+
if (((Extra) extra).empty.isEmpty(index) == false) {
107+
long geoPointValue = ((Extra) extra).values.get(index);
108+
result[i++] = geoPointValue;
109+
}
100110
}
101111
return result;
102112
}

‎x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregatorTests.java‎

Lines changed: 96 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
*/
77
package org.elasticsearch.xpack.spatial.search.aggregations;
88

9+
import org.apache.lucene.document.Field;
910
import org.apache.lucene.document.LatLonDocValuesField;
1011
import org.apache.lucene.document.SortedDocValuesField;
1112
import org.apache.lucene.document.SortedNumericDocValuesField;
@@ -35,6 +36,7 @@
3536
import org.elasticsearch.xpack.spatial.SpatialPlugin;
3637

3738
import java.io.IOException;
39+
import java.util.ArrayList;
3840
import java.util.Arrays;
3941
import java.util.Collections;
4042
import java.util.HashMap;
@@ -52,7 +54,7 @@ protected List<SearchPlugin> getSearchPlugins() {
5254
}
5355

5456
// test that missing values are ignored
55-
public void testMissingValues() throws IOException {
57+
public void testMixedMissingValues() throws IOException {
5658
MultiValuesSourceFieldConfig valueConfig = new MultiValuesSourceFieldConfig.Builder()
5759
.setFieldName("value_field")
5860
.build();
@@ -68,9 +70,98 @@ public void testMissingValues() throws IOException {
6870
.subAggregation(lineAggregationBuilder);
6971

7072
long lonLat = (((long) GeoEncodingUtils.encodeLongitude(90.0)) << 32) | GeoEncodingUtils.encodeLatitude(45.0) & 0xffffffffL;
73+
// input documents for testing
74+
// ----------------------------
75+
// | sort_field | value_field |
76+
// ----------------------------
77+
// | N/A | lonLat |
78+
// | 1 | N/A |
79+
// | 2 | lonLat |
80+
// | N/A | N/A |
81+
// | 4 | lonLat |
82+
// ----------------------------
83+
double[] sortValues = new double[]{ -1, 1, 2, -1, 4 };
84+
long[] points = new long[] { lonLat, -1, lonLat, -1,lonLat };
85+
//expected
86+
long[] expectedAggPoints = new long[] { lonLat, lonLat };
87+
double[] expectedAggSortValues = new double[]{
88+
NumericUtils.doubleToSortableLong(2),
89+
NumericUtils.doubleToSortableLong(4)
90+
};
91+
92+
testCase(new MatchAllDocsQuery(), aggregationBuilder, iw -> {
93+
for (int i = 0; i < points.length; i++) {
94+
List<Field> fields = new ArrayList<>();
95+
fields.add(new SortedDocValuesField("group_id", new BytesRef("group")));
96+
if (sortValues[i] != -1) {
97+
fields.add(new SortedNumericDocValuesField("sort_field", NumericUtils.doubleToSortableLong(sortValues[i])));
98+
}
99+
if (points[i] != -1) {
100+
fields.add(new LatLonDocValuesField("value_field", 45.0, 90.0));
101+
}
102+
iw.addDocument(fields);
103+
}
104+
}, terms -> {
105+
assertThat(terms.getBuckets().size(), equalTo(1));
106+
InternalGeoLine geoLine = terms.getBuckets().get(0).getAggregations().get("_name");
107+
assertThat(geoLine.length(), equalTo(2));
108+
assertTrue(geoLine.isComplete());
109+
assertArrayEquals(expectedAggPoints, geoLine.line());
110+
assertArrayEquals(expectedAggSortValues, geoLine.sortVals(), 0d);
111+
});
112+
}
113+
114+
public void testMissingGeoPointValueField() throws IOException {
115+
MultiValuesSourceFieldConfig valueConfig = new MultiValuesSourceFieldConfig.Builder()
116+
.setFieldName("value_field")
117+
.build();
118+
MultiValuesSourceFieldConfig sortConfig = new MultiValuesSourceFieldConfig.Builder().setFieldName("sort_field").build();
119+
GeoLineAggregationBuilder lineAggregationBuilder = new GeoLineAggregationBuilder("_name")
120+
.point(valueConfig)
121+
.sortOrder(SortOrder.ASC)
122+
.sort(sortConfig)
123+
.size(10);
124+
125+
TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name")
126+
.field("group_id")
127+
.subAggregation(lineAggregationBuilder);
128+
71129
//input
72-
long[] points = new long[] {lonLat, 0, lonLat, 0,lonLat, lonLat, lonLat};
73130
double[] sortValues = new double[]{1, 0, 2, 0, 3, 4, 5};
131+
132+
testCase(new MatchAllDocsQuery(), aggregationBuilder, iw -> {
133+
for (int i = 0; i < sortValues.length; i++) {
134+
iw.addDocument(Arrays.asList(
135+
new SortedNumericDocValuesField("sort_field", NumericUtils.doubleToSortableLong(sortValues[i])),
136+
new SortedDocValuesField("group_id", new BytesRef("group")
137+
)));
138+
}
139+
}, terms -> {
140+
assertThat(terms.getBuckets().size(), equalTo(1));
141+
InternalGeoLine geoLine = terms.getBuckets().get(0).getAggregations().get("_name");
142+
assertThat(geoLine.length(), equalTo(0));
143+
assertTrue(geoLine.isComplete());
144+
});
145+
}
146+
147+
public void testMissingSortField() throws IOException {
148+
MultiValuesSourceFieldConfig valueConfig = new MultiValuesSourceFieldConfig.Builder()
149+
.setFieldName("value_field")
150+
.build();
151+
MultiValuesSourceFieldConfig sortConfig = new MultiValuesSourceFieldConfig.Builder().setFieldName("sort_field").build();
152+
GeoLineAggregationBuilder lineAggregationBuilder = new GeoLineAggregationBuilder("_name")
153+
.point(valueConfig)
154+
.sortOrder(SortOrder.ASC)
155+
.sort(sortConfig)
156+
.size(10);
157+
158+
TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name")
159+
.field("group_id")
160+
.subAggregation(lineAggregationBuilder);
161+
162+
long lonLat = (((long) GeoEncodingUtils.encodeLongitude(90.0)) << 32) | GeoEncodingUtils.encodeLatitude(45.0) & 0xffffffffL;
163+
//input
164+
long[] points = new long[] {lonLat, 0, lonLat, 0,lonLat, lonLat, lonLat};
74165
//expected
75166
long[] expectedAggPoints = new long[] {lonLat, lonLat, lonLat, lonLat, lonLat};
76167
double[] expectedAggSortValues = new double[]{
@@ -82,24 +173,15 @@ public void testMissingValues() throws IOException {
82173
};
83174

84175
testCase(new MatchAllDocsQuery(), aggregationBuilder, iw -> {
85-
86176
for (int i = 0; i < points.length; i++) {
87-
if (points[i] == 0) {
88-
// do not index value
89-
iw.addDocument(Collections.singletonList(new SortedDocValuesField("group_id", new BytesRef("group"))));
90-
} else {
91-
iw.addDocument(Arrays.asList(new LatLonDocValuesField("value_field", 45.0, 90.0),
92-
new SortedNumericDocValuesField("sort_field", NumericUtils.doubleToSortableLong(sortValues[i])),
93-
new SortedDocValuesField("group_id", new BytesRef("group"))));
94-
}
177+
iw.addDocument(Arrays.asList(new LatLonDocValuesField("value_field", 45.0, 90.0),
178+
new SortedDocValuesField("group_id", new BytesRef("group"))));
95179
}
96180
}, terms -> {
97181
assertThat(terms.getBuckets().size(), equalTo(1));
98182
InternalGeoLine geoLine = terms.getBuckets().get(0).getAggregations().get("_name");
99-
assertThat(geoLine.length(), equalTo(5));
183+
assertThat(geoLine.length(), equalTo(0));
100184
assertTrue(geoLine.isComplete());
101-
assertArrayEquals(expectedAggPoints, geoLine.line());
102-
assertArrayEquals(expectedAggSortValues, geoLine.sortVals(), 0d);
103185
});
104186
}
105187

0 commit comments

Comments
 (0)