Skip to content

Commit e2f310b

Browse files
author
Hendrik Muhs
authored
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 c4b8316 commit e2f310b

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;
@@ -237,8 +239,11 @@ public int countPipelineAggregators() {
237239

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

244249
/**
@@ -322,29 +327,32 @@ public AggregatorFactories build(SearchContext context, AggregatorFactory<?> par
322327
parent);
323328
}
324329
AggregatorFactory<?>[] aggFactories = new AggregatorFactory<?>[aggregationBuilders.size()];
325-
for (int i = 0; i < aggregationBuilders.size(); i++) {
326-
aggFactories[i] = aggregationBuilders.get(i).build(context, parent);
330+
331+
int i = 0;
332+
for (AggregationBuilder agg : aggregationBuilders) {
333+
aggFactories[i] = agg.build(context, parent);
334+
++i;
327335
}
328336
return new AggregatorFactories(aggFactories, orderedpipelineAggregators);
329337
}
330338

331339
private List<PipelineAggregationBuilder> resolvePipelineAggregatorOrder(
332-
List<PipelineAggregationBuilder> pipelineAggregatorBuilders, List<AggregationBuilder> aggBuilders,
340+
Collection<PipelineAggregationBuilder> pipelineAggregatorBuilders, Collection<AggregationBuilder> aggregationBuilders,
333341
AggregatorFactory<?> parent) {
334342
Map<String, PipelineAggregationBuilder> pipelineAggregatorBuildersMap = new HashMap<>();
335343
for (PipelineAggregationBuilder builder : pipelineAggregatorBuilders) {
336344
pipelineAggregatorBuildersMap.put(builder.getName(), builder);
337345
}
338346
Map<String, AggregationBuilder> aggBuildersMap = new HashMap<>();
339-
for (AggregationBuilder aggBuilder : aggBuilders) {
347+
for (AggregationBuilder aggBuilder : aggregationBuilders) {
340348
aggBuildersMap.put(aggBuilder.name, aggBuilder);
341349
}
342350
List<PipelineAggregationBuilder> orderedPipelineAggregatorrs = new LinkedList<>();
343351
List<PipelineAggregationBuilder> unmarkedBuilders = new ArrayList<>(pipelineAggregatorBuilders);
344-
Set<PipelineAggregationBuilder> temporarilyMarked = new HashSet<>();
352+
Collection<PipelineAggregationBuilder> temporarilyMarked = new HashSet<>();
345353
while (!unmarkedBuilders.isEmpty()) {
346354
PipelineAggregationBuilder builder = unmarkedBuilders.get(0);
347-
builder.validate(parent, aggBuilders, pipelineAggregatorBuilders);
355+
builder.validate(parent, aggregationBuilders, pipelineAggregatorBuilders);
348356
resolvePipelineAggregatorOrder(aggBuildersMap, pipelineAggregatorBuildersMap, orderedPipelineAggregatorrs, unmarkedBuilders,
349357
temporarilyMarked, builder);
350358
}
@@ -354,7 +362,7 @@ private List<PipelineAggregationBuilder> resolvePipelineAggregatorOrder(
354362
private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggBuildersMap,
355363
Map<String, PipelineAggregationBuilder> pipelineAggregatorBuildersMap,
356364
List<PipelineAggregationBuilder> orderedPipelineAggregators, List<PipelineAggregationBuilder> unmarkedBuilders,
357-
Set<PipelineAggregationBuilder> temporarilyMarked, PipelineAggregationBuilder builder) {
365+
Collection<PipelineAggregationBuilder> temporarilyMarked, PipelineAggregationBuilder builder) {
358366
if (temporarilyMarked.contains(builder)) {
359367
throw new IllegalArgumentException("Cyclical dependency found with pipeline aggregator [" + builder.getName() + "]");
360368
} else if (unmarkedBuilders.contains(builder)) {
@@ -375,7 +383,7 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
375383
} else {
376384
// Check the non-pipeline sub-aggregator
377385
// factories
378-
List<AggregationBuilder> subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders;
386+
Collection<AggregationBuilder> subBuilders = aggBuilder.factoriesBuilder.aggregationBuilders;
379387
boolean foundSubBuilder = false;
380388
for (AggregationBuilder subBuilder : subBuilders) {
381389
if (aggName.equals(subBuilder.name)) {
@@ -386,7 +394,7 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
386394
}
387395
// Check the pipeline sub-aggregator factories
388396
if (!foundSubBuilder && (i == bucketsPathElements.size() - 1)) {
389-
List<PipelineAggregationBuilder> subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders;
397+
Collection<PipelineAggregationBuilder> subPipelineBuilders = aggBuilder.factoriesBuilder.pipelineAggregatorBuilders;
390398
for (PipelineAggregationBuilder subFactory : subPipelineBuilders) {
391399
if (aggName.equals(subFactory.getName())) {
392400
foundSubBuilder = true;
@@ -417,12 +425,12 @@ private void resolvePipelineAggregatorOrder(Map<String, AggregationBuilder> aggB
417425
}
418426
}
419427

420-
public List<AggregationBuilder> getAggregatorFactories() {
421-
return Collections.unmodifiableList(aggregationBuilders);
428+
public Collection<AggregationBuilder> getAggregatorFactories() {
429+
return Collections.unmodifiableCollection(aggregationBuilders);
422430
}
423431

424-
public List<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
425-
return Collections.unmodifiableList(pipelineAggregatorBuilders);
432+
public Collection<PipelineAggregationBuilder> getPipelineAggregatorFactories() {
433+
return Collections.unmodifiableCollection(pipelineAggregatorBuilders);
426434
}
427435

428436
public int count() {
@@ -463,6 +471,7 @@ public boolean equals(Object obj) {
463471
if (getClass() != obj.getClass())
464472
return false;
465473
Builder other = (Builder) obj;
474+
466475
if (!Objects.equals(aggregationBuilders, other.aggregationBuilders))
467476
return false;
468477
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)