Skip to content

Commit 32bdf85

Browse files
authored
Fail variable_width_histogram that collects from many (#58619)
Adds an explicit check to `variable_width_histogram` to stop it from trying to collect from many buckets because it can't. I tried to make it do so but that is more than an afternoon's project, sadly. So for now we just disallow it. Relates to #42035
1 parent 01a07d4 commit 32bdf85

File tree

3 files changed

+54
-1
lines changed

3 files changed

+54
-1
lines changed

docs/reference/aggregations/bucket/variablewidthhistogram-aggregation.asciidoc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,8 @@ Response:
5757
--------------------------------------------------
5858
// TESTRESPONSE[s/\.\.\./"took": $body.took,"timed_out": false,"_shards": $body._shards,"hits": $body.hits,/]
5959

60+
IMPORTANT: This aggregation cannot currently be nested under any aggregation that collects from more than a single bucket.
61+
6062
==== Clustering Algorithm
6163
Each shard fetches the first `initial_buffer` documents and stores them in memory. Once the buffer is full, these documents
6264
are sorted and linearly separated into `3/4 * shard_size buckets`.

server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/VariableWidthHistogramAggregatorFactory.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,13 @@ protected Aggregator doCreateInternal(SearchContext searchContext,
6565
Aggregator parent,
6666
boolean collectsFromSingleBucket,
6767
Map<String, Object> metadata) throws IOException{
68+
if (collectsFromSingleBucket == false) {
69+
throw new IllegalArgumentException(
70+
"["
71+
+ VariableWidthHistogramAggregationBuilder.NAME
72+
+ "] cannot be nested inside an aggregation that collects more than a single bucket."
73+
);
74+
}
6875
AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config,
6976
VariableWidthHistogramAggregationBuilder.NAME);
7077
if (aggregatorSupplier instanceof VariableWidthHistogramAggregatorSupplier == false) {

server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/VariableWidthHistogramAggregatorTests.java

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,16 @@
3131
import org.apache.lucene.util.NumericUtils;
3232
import org.elasticsearch.Version;
3333
import org.elasticsearch.cluster.metadata.IndexMetadata;
34+
import org.elasticsearch.common.CheckedConsumer;
3435
import org.elasticsearch.common.settings.Settings;
3536
import org.elasticsearch.index.IndexSettings;
3637
import org.elasticsearch.index.mapper.MappedFieldType;
3738
import org.elasticsearch.index.mapper.NumberFieldMapper;
39+
import org.elasticsearch.search.aggregations.AggregationBuilder;
3840
import org.elasticsearch.search.aggregations.AggregationBuilders;
3941
import org.elasticsearch.search.aggregations.AggregatorTestCase;
4042
import org.elasticsearch.search.aggregations.bucket.terms.InternalTerms;
43+
import org.elasticsearch.search.aggregations.bucket.terms.LongTerms;
4144
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
4245
import org.elasticsearch.search.aggregations.metrics.InternalStats;
4346
import org.elasticsearch.search.aggregations.metrics.StatsAggregationBuilder;
@@ -51,12 +54,15 @@
5154
import java.util.Map;
5255
import java.util.function.Consumer;
5356

57+
import static java.util.stream.Collectors.toList;
58+
import static org.hamcrest.Matchers.containsString;
59+
import static org.hamcrest.Matchers.equalTo;
60+
5461
public class VariableWidthHistogramAggregatorTests extends AggregatorTestCase {
5562

5663
private static final String NUMERIC_FIELD = "numeric";
5764

5865
private static final Query DEFAULT_QUERY = new MatchAllDocsQuery();
59-
private VariableWidthHistogramAggregationBuilder aggregationBuilder;
6066

6167
public void testNoDocs() throws Exception{
6268
final List<Number> dataset = Arrays.asList();
@@ -424,6 +430,44 @@ public void testMultipleSegments() throws IOException{
424430

425431
}
426432

433+
public void testAsSubAggregation() throws IOException {
434+
AggregationBuilder builder = new TermsAggregationBuilder("t").field("t")
435+
.subAggregation(new VariableWidthHistogramAggregationBuilder("v").field("v").setNumBuckets(2));
436+
CheckedConsumer<RandomIndexWriter, IOException> buildIndex = iw -> {
437+
iw.addDocument(List.of(new SortedNumericDocValuesField("t", 1), new SortedNumericDocValuesField("v", 1)));
438+
iw.addDocument(List.of(new SortedNumericDocValuesField("t", 1), new SortedNumericDocValuesField("v", 10)));
439+
iw.addDocument(List.of(new SortedNumericDocValuesField("t", 1), new SortedNumericDocValuesField("v", 11)));
440+
441+
iw.addDocument(List.of(new SortedNumericDocValuesField("t", 2), new SortedNumericDocValuesField("v", 20)));
442+
iw.addDocument(List.of(new SortedNumericDocValuesField("t", 2), new SortedNumericDocValuesField("v", 30)));
443+
};
444+
Consumer<LongTerms> verify = terms -> {
445+
/*
446+
* This is what the result should be but it never gets called because of
447+
* the explicit check. We do expect to remove the check in the future,
448+
* thus, this stays.
449+
*/
450+
LongTerms.Bucket t1 = terms.getBucketByKey("1");
451+
InternalVariableWidthHistogram v1 = t1.getAggregations().get("v");
452+
assertThat(
453+
v1.getBuckets().stream().map(InternalVariableWidthHistogram.Bucket::centroid).collect(toList()),
454+
equalTo(List.of(1.0, 10.5))
455+
);
456+
457+
LongTerms.Bucket t2 = terms.getBucketByKey("1");
458+
InternalVariableWidthHistogram v2 = t2.getAggregations().get("v");
459+
assertThat(
460+
v2.getBuckets().stream().map(InternalVariableWidthHistogram.Bucket::centroid).collect(toList()),
461+
equalTo(List.of(20.0, 30))
462+
);
463+
};
464+
Exception e = expectThrows(
465+
IllegalArgumentException.class,
466+
() -> testCase(builder, DEFAULT_QUERY, buildIndex, verify, longField("t"), longField("v"))
467+
);
468+
assertThat(e.getMessage(), containsString("cannot be nested"));
469+
}
470+
427471

428472
private void testSearchCase(final Query query, final List<Number> dataset, boolean multipleSegments,
429473
final Consumer<VariableWidthHistogramAggregationBuilder> configure,

0 commit comments

Comments
 (0)