Skip to content

Commit 72d741e

Browse files
author
Hendrik Muhs
committed
Fix AggregationFactories.Builder equality and hash regarding order (#34005)
Fixes the equals and hash function to ignore the order of aggregations to ensure equality after serialization and deserialization. This ensures storing configs with aggregation works properly. This also addresses a potential issue in caching when the same query contains aggregations but in different order. 1st it will not hit in the cache, 2nd cache objects which shall be equal might end up twice in the cache.
1 parent da71d66 commit 72d741e

File tree

49 files changed

+401
-146
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

49 files changed

+401
-146
lines changed

‎server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilder.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
import org.elasticsearch.search.internal.SearchContext;
2929

3030
import java.io.IOException;
31-
import java.util.List;
31+
import java.util.Collection;
3232
import java.util.Map;
3333

3434
/**
@@ -79,12 +79,12 @@ public String getName() {
7979
public abstract AggregationBuilder subAggregation(PipelineAggregationBuilder aggregation);
8080

8181
/** Return the configured set of subaggregations **/
82-
public List<AggregationBuilder> getSubAggregations() {
82+
public Collection<AggregationBuilder> getSubAggregations() {
8383
return factoriesBuilder.getAggregatorFactories();
8484
}
8585

8686
/** Return the configured set of pipeline aggregations **/
87-
public List<PipelineAggregationBuilder> getPipelineAggregations() {
87+
public Collection<PipelineAggregationBuilder> getPipelineAggregations() {
8888
return factoriesBuilder.getPipelineAggregatorFactories();
8989
}
9090

‎server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,11 @@
3838

3939
import java.io.IOException;
4040
import java.util.ArrayList;
41+
import java.util.Collection;
4142
import java.util.Collections;
4243
import java.util.HashMap;
4344
import java.util.HashSet;
45+
import java.util.LinkedHashSet;
4446
import java.util.LinkedList;
4547
import java.util.List;
4648
import java.util.Map;
@@ -240,8 +242,11 @@ public int countPipelineAggregators() {
240242

241243
public static class Builder implements Writeable, ToXContentObject {
242244
private final Set<String> names = new HashSet<>();
243-
private final List<AggregationBuilder> aggregationBuilders = new ArrayList<>();
244-
private final List<PipelineAggregationBuilder> pipelineAggregatorBuilders = new ArrayList<>();
245+
246+
// Using LinkedHashSets to preserve the order of insertion, that makes the results
247+
// ordered nicely, although technically order does not matter
248+
private final Collection<AggregationBuilder> aggregationBuilders = new LinkedHashSet<>();
249+
private final Collection<PipelineAggregationBuilder> pipelineAggregatorBuilders = new LinkedHashSet<>();
245250
private boolean skipResolveOrder;
246251

247252
/**
@@ -325,29 +330,32 @@ public AggregatorFactories build(SearchContext context, AggregatorFactory<?> par
325330
parent);
326331
}
327332
AggregatorFactory<?>[] aggFactories = new AggregatorFactory<?>[aggregationBuilders.size()];
328-
for (int i = 0; i < aggregationBuilders.size(); i++) {
329-
aggFactories[i] = aggregationBuilders.get(i).build(context, parent);
333+
334+
int i = 0;
335+
for (AggregationBuilder agg : aggregationBuilders) {
336+
aggFactories[i] = agg.build(context, parent);
337+
++i;
330338
}
331339
return new AggregatorFactories(parent, aggFactories, orderedpipelineAggregators);
332340
}
333341

334342
private List<PipelineAggregationBuilder> resolvePipelineAggregatorOrder(
335-
List<PipelineAggregationBuilder> pipelineAggregatorBuilders, List<AggregationBuilder> aggBuilders,
343+
Collection<PipelineAggregationBuilder> pipelineAggregatorBuilders, Collection<AggregationBuilder> aggregationBuilders,
336344
AggregatorFactory<?> parent) {
337345
Map<String, PipelineAggregationBuilder> pipelineAggregatorBuildersMap = new HashMap<>();
338346
for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders) {
339347
pipelineAggregatorBuildersMap.put(builder.getName(), builder);
340348
}
341349
Map<String, AggregationBuilder> aggBuildersMap = new HashMap<>();
342-
for (AggregationBuilder aggBuilder : aggBuilders) {
350+
for (AggregationBuilder aggBuilder : aggregationBuilders) {
343351
aggBuildersMap.put(aggBuilder.name, aggBuilder);
344352
}
345353
List<PipelineAggregationBuilder> orderedPipelineAggregatorrs = new LinkedList<>();
346354
List<PipelineAggregationBuilder> unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders);
347-
Set<PipelineAggregationBuilder> temporarilyMarked = new HashSet<>();
355+
Collection<PipelineAggregationBuilder> temporarilyMarked = new HashSet<>();
348356
while (!unmarkedBuilders.isEmpty()) {
349357
PipelineAggregationBuilder builder = unmarkedBuilders.get(0);
350-
builder.validate(parent, aggBuilders, pipelineAggregatorBuilders);
358+
builder.validate(parent, aggregationBuilders, pipelineAggregatorBuilders);
351359
resolvePipelineAggregatorOrder(aggBuildersMap, pipelineAggregatorBuildersMap, orderedPipelineAggregatorrs, unmarkedBuilders,
352360
temporarilyMarked, builder);
353361
}
@@ -357,7 +365,7 @@ private List<PipelineAggregationBuilder> resolvePipelineAggregatorOrder(
357365
private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggBuildersMap,
358366
Map<String, PipelineAggregationBuilder> pipelineAggregatorBuildersMap,
359367
List<PipelineAggregationBuilder> orderedPipelineAggregators, List<PipelineAggregationBuilder> unmarkedBuilders,
360-
Set<PipelineAggregationBuilder> temporarilyMarked, PipelineAggregationBuilder builder) {
368+
Collection<PipelineAggregationBuilder> temporarilyMarked, PipelineAggregationBuilder builder) {
361369
if (temporarilyMarked.contains(builder)) {
362370
throw new IllegalArgumentException("Cyclical dependency found with pipeline aggregator [" + builder.getName() + "]");
363371
} else if (unmarkedBuilders.contains(builder)) {
@@ -378,7 +386,7 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
378386
} else {
379387
// Check the non-pipeline sub-aggregator
380388
// factories
381-
List<AggregationBuilder> subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders;
389+
Collection<AggregationBuilder> subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders;
382390
boolean foundSubBuilder = false;
383391
for (AggregationBuilder subBuilder : subBuilders) {
384392
if (aggName.equals(subBuilder.name)) {
@@ -389,7 +397,7 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
389397
}
390398
// Check the pipeline sub-aggregator factories
391399
if (!foundSubBuilder && (i == bucketsPathElements.size() - 1)) {
392-
List<PipelineAggregationBuilder> subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders;
400+
Collection<PipelineAggregationBuilder> subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders;
393401
for (PipelineAggregationBuilder subFactory : subPipelineBuilders) {
394402
if (aggName.equals(subFactory.getName())) {
395403
foundSubBuilder = true;
@@ -420,12 +428,12 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
420428
}
421429
}
422430

423-
public List<AggregationBuilder> getAggregatorFactories() {
424-
return Collections.unmodifiableList(aggregationBuilders);
431+
public Collection<AggregationBuilder> getAggregatorFactories() {
432+
return Collections.unmodifiableCollection(aggregationBuilders);
425433
}
426434

427-
public List<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
428-
return Collections.unmodifiableList(pipelineAggregatorBuilders);
435+
public Collection<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
436+
return Collections.unmodifiableCollection(pipelineAggregatorBuilders);
429437
}
430438

431439
public int count() {
@@ -466,6 +474,7 @@ public boolean equals(Object obj) {
466474
if (getClass() != obj.getClass())
467475
return false;
468476
Builder other = (Builder) obj;
477+
469478
if (!Objects.equals(aggregationBuilders, other.aggregationBuilders))
470479
return false;
471480
if (!Objects.equals(pipelineAggregatorBuilders, other.pipelineAggregatorBuilders))

‎server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
2626

2727
import java.io.IOException;
28-
import java.util.List;
28+
import java.util.Collection;
2929
import java.util.Map;
3030

3131
/**
@@ -68,8 +68,8 @@ public final String[] getBucketsPaths() {
6868
* Internal: Validates the state of this factory (makes sure the factory is properly
6969
* configured)
7070
*/
71-
protected abstract void validate(AggregatorFactory<?> parent, List<AggregationBuilder> factories,
72-
List<PipelineAggregationBuilder> pipelineAggregatorFactories);
71+
protected abstract void validate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggregationBuilders,
72+
Collection<PipelineAggregationBuilder> pipelineAggregatorBuilders);
7373

7474
/**
7575
* Creates the pipeline aggregator

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828

2929
import java.io.IOException;
3030
import java.util.Arrays;
31-
import java.util.List;
31+
import java.util.Collection;
3232
import java.util.Map;
3333
import java.util.Objects;
3434

@@ -81,8 +81,8 @@ public String type() {
8181
* configured)
8282
*/
8383
@Override
84-
public final void validate(AggregatorFactory<?> parent, List<AggregationBuilder> factories,
85-
List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
84+
public final void validate(AggregatorFactory<?> parent, Collection<AggregationBuilder> factories,
85+
Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
8686
doValidate(parent, factories, pipelineAggregatorFactories);
8787
}
8888

@@ -99,8 +99,8 @@ public final PipelineAggregator create() throws IOException {
9999
return aggregator;
100100
}
101101

102-
public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> factories,
103-
List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
102+
public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> factories,
103+
Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
104104
}
105105

106106
@SuppressWarnings("unchecked")

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/BucketMetricsPipelineAggregationBuilder.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
3333

3434
import java.io.IOException;
35-
import java.util.List;
35+
import java.util.Collection;
3636
import java.util.Map;
3737
import java.util.Objects;
3838
import java.util.Optional;
@@ -109,8 +109,8 @@ public GapPolicy gapPolicy() {
109109
protected abstract PipelineAggregator createInternal(Map<String, Object> metaData) throws IOException;
110110

111111
@Override
112-
public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggBuilders,
113-
List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
112+
public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggBuilders,
113+
Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
114114
if (bucketsPaths.length != 1) {
115115
throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName()
116116
+ " must contain a single entry for aggregation [" + name + "]");

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/percentile/PercentilesBucketPipelineAggregationBuilder.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@
3535

3636
import java.io.IOException;
3737
import java.util.Arrays;
38-
import java.util.List;
38+
import java.util.Collection;
3939
import java.util.Map;
4040
import java.util.Objects;
4141

@@ -95,8 +95,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
9595
}
9696

9797
@Override
98-
public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
99-
List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
98+
public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
99+
Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
100100
super.doValidate(parent, aggFactories, pipelineAggregatorFactories);
101101

102102
for (Double p : percents) {

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketmetrics/stats/extended/ExtendedStatsBucketPipelineAggregationBuilder.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
import org.elasticsearch.search.aggregations.pipeline.bucketmetrics.BucketMetricsPipelineAggregationBuilder;
3030

3131
import java.io.IOException;
32-
import java.util.List;
32+
import java.util.Collection;
3333
import java.util.Map;
3434
import java.util.Objects;
3535

@@ -82,8 +82,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
8282
}
8383

8484
@Override
85-
public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggBuilders,
86-
List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
85+
public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggBuilders,
86+
Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
8787
super.doValidate(parent, aggBuilders, pipelineAggregatorFactories);
8888

8989
if (sigma < 0.0 ) {

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/bucketsort/BucketSortPipelineAggregationBuilder.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.io.IOException;
3939
import java.util.ArrayList;
4040
import java.util.Arrays;
41+
import java.util.Collection;
4142
import java.util.Collections;
4243
import java.util.List;
4344
import java.util.Locale;
@@ -145,8 +146,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
145146
}
146147

147148
@Override
148-
public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
149-
List<PipelineAggregationBuilder> pipelineAggregatoractories) {
149+
public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
150+
Collection<PipelineAggregationBuilder> pipelineAggregatoractories) {
150151
if (sorts.isEmpty() && size == null && from == 0) {
151152
throw new IllegalStateException("[" + name + "] is configured to perform nothing. Please set either of "
152153
+ Arrays.asList(SearchSourceBuilder.SORT_FIELD.getPreferredName(), SIZE.getPreferredName(), FROM.getPreferredName())

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/cumulativesum/CumulativeSumPipelineAggregationBuilder.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636

3737
import java.io.IOException;
3838
import java.util.ArrayList;
39+
import java.util.Collection;
3940
import java.util.List;
4041
import java.util.Map;
4142
import java.util.Objects;
@@ -97,8 +98,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
9798
}
9899

99100
@Override
100-
public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
101-
List<PipelineAggregationBuilder> pipelineAggregatorFactories) {
101+
public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
102+
Collection<PipelineAggregationBuilder> pipelineAggregatorFactories) {
102103
if (bucketsPaths.length != 1) {
103104
throw new IllegalStateException(BUCKETS_PATH.getPreferredName()
104105
+ " must contain a single entry for aggregation [" + name + "]");

‎server/src/main/java/org/elasticsearch/search/aggregations/pipeline/derivative/DerivativePipelineAggregationBuilder.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242

4343
import java.io.IOException;
4444
import java.util.ArrayList;
45+
import java.util.Collection;
4546
import java.util.List;
4647
import java.util.Map;
4748
import java.util.Objects;
@@ -156,8 +157,8 @@ protected PipelineAggregator createInternal(Map<String, Object> metaData) throws
156157
}
157158

158159
@Override
159-
public void doValidate(AggregatorFactory<?> parent, List<AggregationBuilder> aggFactories,
160-
List<PipelineAggregationBuilder> pipelineAggregatoractories) {
160+
public void doValidate(AggregatorFactory<?> parent, Collection<AggregationBuilder> aggFactories,
161+
Collection<PipelineAggregationBuilder> pipelineAggregatoractories) {
161162
if (bucketsPaths.length != 1) {
162163
throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName()
163164
+ " must contain a single entry for aggregation [" + name + "]");

0 commit comments

Comments
 (0)