Skip to content

Commit f0e6b24

Browse files
jimczijimczi
committed
Fix date and ip sources in the composite aggregation (#29370)
This commit fixes the formatting of the values in the composite aggregation response. `date` fields should return timestamp as longs when used in a `terms` source and `ip` fields should always be formatted as strings. This commit also fixes the parsing of the `after` key for these field types. Finally, this commit disables the index optimization for the `ip` field and any source that provides a `missing` value.
1 parent a3e2423 commit f0e6b24

14 files changed

+376
-62
lines changed

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/BinaryValuesSource.java

+10-7
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,11 @@
2626
import org.apache.lucene.util.BytesRef;
2727
import org.elasticsearch.common.CheckedFunction;
2828
import org.elasticsearch.index.fielddata.SortedBinaryDocValues;
29+
import org.elasticsearch.index.mapper.KeywordFieldMapper;
2930
import org.elasticsearch.index.mapper.MappedFieldType;
31+
import org.elasticsearch.index.mapper.StringFieldType;
32+
import org.elasticsearch.index.mapper.TextFieldMapper;
33+
import org.elasticsearch.search.DocValueFormat;
3034
import org.elasticsearch.search.aggregations.LeafBucketCollector;
3135

3236
import java.io.IOException;
@@ -40,8 +44,8 @@ class BinaryValuesSource extends SingleDimensionValuesSource<BytesRef> {
4044
private BytesRef currentValue;
4145

4246
BinaryValuesSource(MappedFieldType fieldType, CheckedFunction<LeafReaderContext, SortedBinaryDocValues, IOException> docValuesFunc,
43-
int size, int reverseMul) {
44-
super(fieldType, size, reverseMul);
47+
DocValueFormat format, Object missing, int size, int reverseMul) {
48+
super(format, fieldType, missing, size, reverseMul);
4549
this.docValuesFunc = docValuesFunc;
4650
this.values = new BytesRef[size];
4751
}
@@ -72,10 +76,8 @@ int compareValues(BytesRef v1, BytesRef v2) {
7276

7377
@Override
7478
void setAfter(Comparable<?> value) {
75-
if (value.getClass() == BytesRef.class) {
76-
afterValue = (BytesRef) value;
77-
} else if (value.getClass() == String.class) {
78-
afterValue = new BytesRef((String) value);
79+
if (value.getClass() == String.class) {
80+
afterValue = format.parseBytesRef(value.toString());
7981
} else {
8082
throw new IllegalArgumentException("invalid value, expected string, got " + value.getClass().getSimpleName());
8183
}
@@ -120,7 +122,8 @@ public void collect(int doc, long bucket) throws IOException {
120122
@Override
121123
SortedDocsProducer createSortedDocsProducerOrNull(IndexReader reader, Query query) {
122124
if (checkIfSortedDocsIsApplicable(reader, fieldType) == false ||
123-
(query != null && query.getClass() != MatchAllDocsQuery.class)) {
125+
fieldType instanceof StringFieldType == false ||
126+
(query != null && query.getClass() != MatchAllDocsQuery.class)) {
124127
return null;
125128
}
126129
return new TermsSortedDocsProducer(fieldType.name());

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregator.java

+62-8
Original file line numberDiff line numberDiff line change
@@ -263,30 +263,84 @@ private static SingleDimensionValuesSource<?>[] createValuesSources(BigArrays bi
263263
final int reverseMul = configs[i].reverseMul();
264264
if (configs[i].valuesSource() instanceof ValuesSource.Bytes.WithOrdinals && reader instanceof DirectoryReader) {
265265
ValuesSource.Bytes.WithOrdinals vs = (ValuesSource.Bytes.WithOrdinals) configs[i].valuesSource();
266-
sources[i] = new GlobalOrdinalValuesSource(bigArrays, configs[i].fieldType(), vs::globalOrdinalsValues, size, reverseMul);
266+
sources[i] = new GlobalOrdinalValuesSource(
267+
bigArrays,
268+
configs[i].fieldType(),
269+
vs::globalOrdinalsValues,
270+
configs[i].format(),
271+
configs[i].missing(),
272+
size,
273+
reverseMul
274+
);
275+
267276
if (i == 0 && sources[i].createSortedDocsProducerOrNull(reader, query) != null) {
268277
// this the leading source and we can optimize it with the sorted docs producer but
269278
// we don't want to use global ordinals because the number of visited documents
270279
// should be low and global ordinals need one lookup per visited term.
271280
Releasables.close(sources[i]);
272-
sources[i] = new BinaryValuesSource(configs[i].fieldType(), vs::bytesValues, size, reverseMul);
281+
sources[i] = new BinaryValuesSource(
282+
configs[i].fieldType(),
283+
vs::bytesValues,
284+
configs[i].format(),
285+
configs[i].missing(),
286+
size,
287+
reverseMul
288+
);
273289
}
274290
} else if (configs[i].valuesSource() instanceof ValuesSource.Bytes) {
275291
ValuesSource.Bytes vs = (ValuesSource.Bytes) configs[i].valuesSource();
276-
sources[i] = new BinaryValuesSource(configs[i].fieldType(), vs::bytesValues, size, reverseMul);
292+
sources[i] = new BinaryValuesSource(
293+
configs[i].fieldType(),
294+
vs::bytesValues,
295+
configs[i].format(),
296+
configs[i].missing(),
297+
size,
298+
reverseMul
299+
);
300+
277301
} else if (configs[i].valuesSource() instanceof ValuesSource.Numeric) {
278302
final ValuesSource.Numeric vs = (ValuesSource.Numeric) configs[i].valuesSource();
279303
if (vs.isFloatingPoint()) {
280-
sources[i] = new DoubleValuesSource(bigArrays, configs[i].fieldType(), vs::doubleValues, size, reverseMul);
304+
sources[i] = new DoubleValuesSource(
305+
bigArrays,
306+
configs[i].fieldType(),
307+
vs::doubleValues,
308+
configs[i].format(),
309+
configs[i].missing(),
310+
size,
311+
reverseMul
312+
);
313+
281314
} else {
282315
if (vs instanceof RoundingValuesSource) {
283-
sources[i] = new LongValuesSource(bigArrays, configs[i].fieldType(), vs::longValues,
284-
((RoundingValuesSource) vs)::round, configs[i].format(), size, reverseMul);
316+
sources[i] = new LongValuesSource(
317+
bigArrays,
318+
configs[i].fieldType(),
319+
vs::longValues,
320+
((RoundingValuesSource) vs)::round,
321+
configs[i].format(),
322+
configs[i].missing(),
323+
size,
324+
reverseMul
325+
);
326+
285327
} else {
286-
sources[i] = new LongValuesSource(bigArrays, configs[i].fieldType(), vs::longValues,
287-
(value) -> value, configs[i].format(), size, reverseMul);
328+
sources[i] = new LongValuesSource(
329+
bigArrays,
330+
configs[i].fieldType(),
331+
vs::longValues,
332+
(value) -> value,
333+
configs[i].format(),
334+
configs[i].missing(),
335+
size,
336+
reverseMul
337+
);
338+
288339
}
289340
}
341+
} else {
342+
throw new IllegalArgumentException("Unknown value source: " + configs[i].valuesSource().getClass().getName() +
343+
" for field: " + sources[i].fieldType.name());
290344
}
291345
}
292346
return sources;

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java

+1
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,7 @@ public String format() {
291291
public final CompositeValuesSourceConfig build(SearchContext context) throws IOException {
292292
ValuesSourceConfig<?> config = ValuesSourceConfig.resolve(context.getQueryShardContext(),
293293
valueType, field, script, missing, null, format);
294+
294295
if (config.unmapped() && field != null && config.missing() == null) {
295296
// this source cannot produce any values so we refuse to build
296297
// since composite buckets are not created on null values

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceConfig.java

+20-1
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,25 @@ class CompositeValuesSourceConfig {
3232
private final ValuesSource vs;
3333
private final DocValueFormat format;
3434
private final int reverseMul;
35+
private final Object missing;
3536

36-
CompositeValuesSourceConfig(String name, @Nullable MappedFieldType fieldType, ValuesSource vs, DocValueFormat format, SortOrder order) {
37+
/**
38+
* Creates a new {@link CompositeValuesSourceConfig}.
39+
* @param name The name of the source.
40+
* @param fieldType The field type or null if the source is a script.
41+
* @param vs The underlying {@link ValuesSource}.
42+
* @param format The {@link DocValueFormat} of this source.
43+
* @param order The sort order associated with this source.
44+
* @param missing The missing value or null if documents with missing value should be ignored.
45+
*/
46+
CompositeValuesSourceConfig(String name, @Nullable MappedFieldType fieldType, ValuesSource vs, DocValueFormat format,
47+
SortOrder order, @Nullable Object missing) {
3748
this.name = name;
3849
this.fieldType = fieldType;
3950
this.vs = vs;
4051
this.format = format;
4152
this.reverseMul = order == SortOrder.ASC ? 1 : -1;
53+
this.missing = missing;
4254
}
4355

4456
/**
@@ -70,6 +82,13 @@ DocValueFormat format() {
7082
return format;
7183
}
7284

85+
/**
86+
* The missing value for this configuration or null if documents with missing value should be ignored.
87+
*/
88+
Object missing() {
89+
return missing;
90+
}
91+
7392
/**
7493
* The sort order for the values source (e.g. -1 for descending and 1 for ascending).
7594
*/

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import org.elasticsearch.search.DocValueFormat;
3434
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
3535
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
36-
import org.elasticsearch.search.aggregations.support.FieldContext;
3736
import org.elasticsearch.search.aggregations.support.ValueType;
3837
import org.elasticsearch.search.aggregations.support.ValuesSource;
3938
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
@@ -227,7 +226,7 @@ protected CompositeValuesSourceConfig innerBuild(SearchContext context, ValuesSo
227226
// is specified in the builder.
228227
final DocValueFormat docValueFormat = format() == null ? DocValueFormat.RAW : config.format();
229228
final MappedFieldType fieldType = config.fieldContext() != null ? config.fieldContext().fieldType() : null;
230-
return new CompositeValuesSourceConfig(name, fieldType, vs, docValueFormat, order());
229+
return new CompositeValuesSourceConfig(name, fieldType, vs, docValueFormat, order(), missing());
231230
} else {
232231
throw new IllegalArgumentException("invalid source, expected numeric, got " + orig.getClass().getSimpleName());
233232
}

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DoubleValuesSource.java

+6-3
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.elasticsearch.common.util.DoubleArray;
2929
import org.elasticsearch.index.fielddata.SortedNumericDoubleValues;
3030
import org.elasticsearch.index.mapper.MappedFieldType;
31+
import org.elasticsearch.search.DocValueFormat;
3132
import org.elasticsearch.search.aggregations.LeafBucketCollector;
3233

3334
import java.io.IOException;
@@ -42,8 +43,8 @@ class DoubleValuesSource extends SingleDimensionValuesSource<Double> {
4243

4344
DoubleValuesSource(BigArrays bigArrays, MappedFieldType fieldType,
4445
CheckedFunction<LeafReaderContext, SortedNumericDoubleValues, IOException> docValuesFunc,
45-
int size, int reverseMul) {
46-
super(fieldType, size, reverseMul);
46+
DocValueFormat format, Object missing, int size, int reverseMul) {
47+
super(format, fieldType, missing, size, reverseMul);
4748
this.docValuesFunc = docValuesFunc;
4849
this.values = bigArrays.newDoubleArray(size, false);
4950
}
@@ -77,7 +78,9 @@ void setAfter(Comparable<?> value) {
7778
if (value instanceof Number) {
7879
afterValue = ((Number) value).doubleValue();
7980
} else {
80-
afterValue = Double.parseDouble(value.toString());
81+
afterValue = format.parseDouble(value.toString(), false, () -> {
82+
throw new IllegalArgumentException("now() is not supported in [after] key");
83+
});
8184
}
8285
}
8386

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java

+8-7
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
import org.elasticsearch.common.util.BigArrays;
3131
import org.elasticsearch.common.util.LongArray;
3232
import org.elasticsearch.index.mapper.MappedFieldType;
33+
import org.elasticsearch.index.mapper.StringFieldType;
34+
import org.elasticsearch.search.DocValueFormat;
3335
import org.elasticsearch.search.aggregations.LeafBucketCollector;
3436

3537
import java.io.IOException;
@@ -52,8 +54,8 @@ class GlobalOrdinalValuesSource extends SingleDimensionValuesSource<BytesRef> {
5254

5355
GlobalOrdinalValuesSource(BigArrays bigArrays,
5456
MappedFieldType type, CheckedFunction<LeafReaderContext, SortedSetDocValues, IOException> docValuesFunc,
55-
int size, int reverseMul) {
56-
super(type, size, reverseMul);
57+
DocValueFormat format, Object missing, int size, int reverseMul) {
58+
super(format, type, missing, size, reverseMul);
5759
this.docValuesFunc = docValuesFunc;
5860
this.values = bigArrays.newLongArray(size, false);
5961
}
@@ -87,10 +89,8 @@ int compareCurrentWithAfter() {
8789

8890
@Override
8991
void setAfter(Comparable<?> value) {
90-
if (value instanceof BytesRef) {
91-
afterValue = (BytesRef) value;
92-
} else if (value instanceof String) {
93-
afterValue = new BytesRef(value.toString());
92+
if (value.getClass() == String.class) {
93+
afterValue = format.parseBytesRef(value.toString());
9494
} else {
9595
throw new IllegalArgumentException("invalid value, expected string, got " + value.getClass().getSimpleName());
9696
}
@@ -164,7 +164,8 @@ public void collect(int doc, long bucket) throws IOException {
164164
@Override
165165
SortedDocsProducer createSortedDocsProducerOrNull(IndexReader reader, Query query) {
166166
if (checkIfSortedDocsIsApplicable(reader, fieldType) == false ||
167-
(query != null && query.getClass() != MatchAllDocsQuery.class)) {
167+
fieldType instanceof StringFieldType == false ||
168+
(query != null && query.getClass() != MatchAllDocsQuery.class)) {
168169
return null;
169170
}
170171
return new TermsSortedDocsProducer(fieldType.name());

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ protected CompositeValuesSourceConfig innerBuild(SearchContext context, ValuesSo
115115
ValuesSource.Numeric numeric = (ValuesSource.Numeric) orig;
116116
final HistogramValuesSource vs = new HistogramValuesSource(numeric, interval);
117117
final MappedFieldType fieldType = config.fieldContext() != null ? config.fieldContext().fieldType() : null;
118-
return new CompositeValuesSourceConfig(name, fieldType, vs, config.format(), order());
118+
return new CompositeValuesSourceConfig(name, fieldType, vs, config.format(), order(), missing());
119119
} else {
120120
throw new IllegalArgumentException("invalid source, expected numeric, got " + orig.getClass().getSimpleName());
121121
}

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/LongValuesSource.java

+2-5
Original file line numberDiff line numberDiff line change
@@ -47,19 +47,16 @@
4747
class LongValuesSource extends SingleDimensionValuesSource<Long> {
4848
private final CheckedFunction<LeafReaderContext, SortedNumericDocValues, IOException> docValuesFunc;
4949
private final LongUnaryOperator rounding;
50-
// handles "format" for date histogram source
51-
private final DocValueFormat format;
5250

5351
private final LongArray values;
5452
private long currentValue;
5553

5654
LongValuesSource(BigArrays bigArrays, MappedFieldType fieldType,
5755
CheckedFunction<LeafReaderContext, SortedNumericDocValues, IOException> docValuesFunc,
58-
LongUnaryOperator rounding, DocValueFormat format, int size, int reverseMul) {
59-
super(fieldType, size, reverseMul);
56+
LongUnaryOperator rounding, DocValueFormat format, Object missing, int size, int reverseMul) {
57+
super(format, fieldType, missing, size, reverseMul);
6058
this.docValuesFunc = docValuesFunc;
6159
this.rounding = rounding;
62-
this.format = format;
6360
this.values = bigArrays.newLongArray(size, false);
6461
}
6562

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/SingleDimensionValuesSource.java

+17-5
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.elasticsearch.common.Nullable;
2727
import org.elasticsearch.common.lease.Releasable;
2828
import org.elasticsearch.index.mapper.MappedFieldType;
29+
import org.elasticsearch.search.DocValueFormat;
2930
import org.elasticsearch.search.aggregations.LeafBucketCollector;
3031
import org.elasticsearch.search.sort.SortOrder;
3132

@@ -35,21 +36,31 @@
3536
* A source that can record and compare values of similar type.
3637
*/
3738
abstract class SingleDimensionValuesSource<T extends Comparable<T>> implements Releasable {
39+
protected final DocValueFormat format;
40+
@Nullable
41+
protected final MappedFieldType fieldType;
42+
@Nullable
43+
protected final Object missing;
44+
3845
protected final int size;
3946
protected final int reverseMul;
47+
4048
protected T afterValue;
41-
@Nullable
42-
protected MappedFieldType fieldType;
4349

4450
/**
45-
* Ctr
51+
* Creates a new {@link SingleDimensionValuesSource}.
4652
*
47-
* @param fieldType The fieldType associated with the source.
53+
* @param format The format of the source.
54+
* @param fieldType The field type or null if the source is a script.
55+
* @param missing The missing value or null if documents with missing value should be ignored.
4856
* @param size The number of values to record.
4957
* @param reverseMul -1 if the natural order ({@link SortOrder#ASC} should be reversed.
5058
*/
51-
SingleDimensionValuesSource(@Nullable MappedFieldType fieldType, int size, int reverseMul) {
59+
SingleDimensionValuesSource(DocValueFormat format, @Nullable MappedFieldType fieldType, @Nullable Object missing,
60+
int size, int reverseMul) {
61+
this.format = format;
5262
this.fieldType = fieldType;
63+
this.missing = missing;
5364
this.size = size;
5465
this.reverseMul = reverseMul;
5566
this.afterValue = null;
@@ -127,6 +138,7 @@ abstract LeafBucketCollector getLeafCollector(Comparable<?> value,
127138
*/
128139
protected boolean checkIfSortedDocsIsApplicable(IndexReader reader, MappedFieldType fieldType) {
129140
if (fieldType == null ||
141+
missing != null ||
130142
fieldType.indexOptions() == IndexOptions.NONE ||
131143
// inverse of the natural order
132144
reverseMul == -1) {

‎server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java

+10-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424
import org.elasticsearch.common.xcontent.ObjectParser;
2525
import org.elasticsearch.common.xcontent.XContentBuilder;
2626
import org.elasticsearch.common.xcontent.XContentParser;
27+
import org.elasticsearch.index.mapper.DateFieldMapper;
2728
import org.elasticsearch.index.mapper.MappedFieldType;
29+
import org.elasticsearch.search.DocValueFormat;
2830
import org.elasticsearch.search.aggregations.support.ValuesSource;
2931
import org.elasticsearch.search.aggregations.support.ValuesSourceConfig;
3032
import org.elasticsearch.search.internal.SearchContext;
@@ -84,6 +86,13 @@ protected CompositeValuesSourceConfig innerBuild(SearchContext context, ValuesSo
8486
vs = ValuesSource.Numeric.EMPTY;
8587
}
8688
final MappedFieldType fieldType = config.fieldContext() != null ? config.fieldContext().fieldType() : null;
87-
return new CompositeValuesSourceConfig(name, fieldType, vs, config.format(), order());
89+
final DocValueFormat format;
90+
if (format() == null && fieldType instanceof DateFieldMapper.DateFieldType) {
91+
// defaults to the raw format on date fields (preserve timestamp as longs).
92+
format = DocValueFormat.RAW;
93+
} else {
94+
format = config.format();
95+
}
96+
return new CompositeValuesSourceConfig(name, fieldType, vs, format, order(), missing());
8897
}
8998
}

0 commit comments

Comments
 (0)