Skip to content

Commit

Permalink
Drop deprecated aggregator wrapper (backport of #58367) (#58448)
Browse files Browse the repository at this point in the history 
This drops the deprecated and now unused `asMultiBucketAggregator`. It
was too easy to use it to make inefficient `Aggregators`.

Relates to #56487
  • Loading branch information
@nik9000
nik9000 committed Jun 25, 2020
1 parent ebe1d9c commit 335505c
Show file tree
Hide file tree
Showing 7 changed files with 4 additions and 369 deletions.
179 changes: 0 additions & 179 deletions server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,180 +19,15 @@

package org.elasticsearch.search.aggregations;

import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.Scorable;
import org.apache.lucene.search.ScoreMode;
import org.elasticsearch.common.lease.Releasables;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.common.util.ObjectArray;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.search.internal.SearchContext;
import org.elasticsearch.search.internal.SearchContext.Lifetime;

import java.io.IOException;
import java.util.Map;
import java.util.function.BiConsumer;

import static org.elasticsearch.search.aggregations.support.AggregationUsageService.OTHER_SUBTYPE;

public abstract class AggregatorFactory {

public static final class MultiBucketAggregatorWrapper extends Aggregator {
private final BigArrays bigArrays;
private final Aggregator parent;
private final AggregatorFactory factory;
private final Aggregator first;
ObjectArray<Aggregator> aggregators;
ObjectArray<LeafBucketCollector> collectors;

MultiBucketAggregatorWrapper(BigArrays bigArrays, SearchContext context,
Aggregator parent, AggregatorFactory factory, Aggregator first) {
this.bigArrays = bigArrays;
this.parent = parent;
this.factory = factory;
this.first = first;
context.addReleasable(this, Lifetime.PHASE);
aggregators = bigArrays.newObjectArray(1);
aggregators.set(0, first);
collectors = bigArrays.newObjectArray(1);
}

public Class<?> getWrappedClass() {
return first.getClass();
}

@Override
public String name() {
return first.name();
}

@Override
public SearchContext context() {
return first.context();
}

@Override
public Aggregator parent() {
return first.parent();
}

@Override
public ScoreMode scoreMode() {
return first.scoreMode();
}

@Override
public Aggregator subAggregator(String name) {
throw new UnsupportedOperationException();
}

@Override
public void preCollection() throws IOException {
for (long i = 0; i < aggregators.size(); ++i) {
final Aggregator aggregator = aggregators.get(i);
if (aggregator != null) {
aggregator.preCollection();
}
}
}

@Override
public void postCollection() throws IOException {
for (long i = 0; i < aggregators.size(); ++i) {
final Aggregator aggregator = aggregators.get(i);
if (aggregator != null) {
aggregator.postCollection();
}
}
}

@Override
public LeafBucketCollector getLeafCollector(final LeafReaderContext ctx) {
for (long i = 0; i < collectors.size(); ++i) {
collectors.set(i, null);
}
return new LeafBucketCollector() {
Scorable scorer;

@Override
public void setScorer(Scorable scorer) throws IOException {
this.scorer = scorer;
}

@Override
public void collect(int doc, long bucket) throws IOException {
collectors = bigArrays.grow(collectors, bucket + 1);

LeafBucketCollector collector = collectors.get(bucket);
if (collector == null) {
aggregators = bigArrays.grow(aggregators, bucket + 1);
Aggregator aggregator = aggregators.get(bucket);
if (aggregator == null) {
aggregator = factory.create(context(), parent, true);
aggregator.preCollection();
aggregators.set(bucket, aggregator);
}
collector = aggregator.getLeafCollector(ctx);
if (scorer != null) {
// Passing a null scorer can cause unexpected NPE at a later time,
// which can't not be directly linked to the fact that a null scorer has been supplied.
collector.setScorer(scorer);
}
collectors.set(bucket, collector);
}
collector.collect(doc, 0);
}

};
}

@Override
public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws IOException {
InternalAggregation[] results = new InternalAggregation[owningBucketOrds.length];
for (int ordIdx = 0; ordIdx < owningBucketOrds.length; ordIdx++) {
if (owningBucketOrds[ordIdx] < aggregators.size()) {
Aggregator aggregator = aggregators.get(owningBucketOrds[ordIdx]);
if (aggregator != null) {
/*
* This is the same call as buildTopLevel but since
* this aggregator may not be the top level we don't
* call that method here. It'd be weird sounding. And
* it'd trip assertions. Both bad.
*/
results[ordIdx] = aggregator.buildAggregations(new long [] {0})[0];
} else {
results[ordIdx] = buildEmptyAggregation();
}
} else {
results[ordIdx] = buildEmptyAggregation();
}
}
return results;
}

@Override
public InternalAggregation buildEmptyAggregation() {
return first.buildEmptyAggregation();
}

@Override
public void close() {
Releasables.close(aggregators, collectors);
}

@Override
public void collectDebugInfo(BiConsumer<String, Object> add) {
/*
* There isn't really a sane way to give our delegates a way to
* add entries because we'd have to merge them. So we just *don't*
* and leave a marker of our own. This ain't great, but we plan
* to cut down on usage of this wrapper in the future.
*/
add.accept("wrapped_in_multi_bucket_aggregator", true);
super.collectDebugInfo(add);
}
}

protected final String name;
protected final AggregatorFactory parent;
protected final AggregatorFactories factories;
Expand Down Expand Up @@ -254,20 +89,6 @@ public AggregatorFactory getParent() {
return parent;
}

/**
* Utility method. Given an {@link AggregatorFactory} that creates
* {@link Aggregator}s that only know how to collect bucket {@code 0}, this
* returns an aggregator that can collect any bucket.
* @deprecated implement the aggregator to handle many owning buckets
*/
@Deprecated
protected static Aggregator asMultiBucketAggregator(final AggregatorFactory factory, final SearchContext searchContext,
final Aggregator parent) throws IOException {
final Aggregator first = factory.create(searchContext, parent, true);
final BigArrays bigArrays = searchContext.bigArrays();
return new MultiBucketAggregatorWrapper(bigArrays, searchContext, parent, factory, first);
}

/**
* Returns the aggregation subtype for nodes usage stats.
* <p>
Expand Down
31 changes: 2 additions & 29 deletions server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,10 @@
import org.elasticsearch.common.lease.Releasable;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.common.util.IntArray;
import org.elasticsearch.common.util.LongHash;
import org.elasticsearch.search.aggregations.AggregationExecutionException;
import org.elasticsearch.search.aggregations.Aggregator;
import org.elasticsearch.search.aggregations.AggregatorBase;
import org.elasticsearch.search.aggregations.AggregatorFactories;
import org.elasticsearch.search.aggregations.AggregatorFactory;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.LeafBucketCollector;
Expand Down Expand Up @@ -154,7 +152,7 @@ protected void beforeBuildingBuckets(long[] ordsToCollect) throws IOException {}
* <p>
* Most aggregations should probably use something like
* {@link #buildSubAggsForAllBuckets(Object[][], ToLongFunction, BiConsumer)}
* or {@link #buildAggregationsForVariableBuckets(long[], LongHash, BucketBuilderForVariable, Function)}
* or {@link #buildAggregationsForVariableBuckets(long[], LongKeyedBucketOrds, BucketBuilderForVariable, ResultBuilderForVariable)}
* or {@link #buildAggregationsForFixedBucketCount(long[], int, BucketBuilderForFixedCount, Function)}
* or {@link #buildAggregationsForSingleBucket(long[], SingleBucketResultBuilder)}
* instead of calling this directly.
Expand Down Expand Up @@ -300,32 +298,7 @@ protected interface SingleBucketResultBuilder {

/**
* Build aggregation results for an aggregator with a varying number of
* {@code long} keyed buckets that is at the top level or wrapped in
* {@link AggregatorFactory#asMultiBucketAggregator}.
* @param owningBucketOrds owning bucket ordinals for which to build the results
* @param bucketOrds hash of values to the bucket ordinal
*/
protected final <B> InternalAggregation[] buildAggregationsForVariableBuckets(long[] owningBucketOrds, LongHash bucketOrds,
BucketBuilderForVariable<B> bucketBuilder, Function<List<B>, InternalAggregation> resultBuilder) throws IOException {
assert owningBucketOrds.length == 1 && owningBucketOrds[0] == 0;
long[] bucketOrdsToCollect = new long[(int) bucketOrds.size()];
for (int bucketOrd = 0; bucketOrd < bucketOrds.size(); bucketOrd++) {
bucketOrdsToCollect[bucketOrd] = bucketOrd;
}

InternalAggregations[] subAggregationResults = buildSubAggsForBuckets(bucketOrdsToCollect);
List<B> buckets = new ArrayList<>((int) bucketOrds.size());
for (int bucketOrd = 0; bucketOrd < bucketOrds.size(); bucketOrd++) {
buckets.add(bucketBuilder.build(bucketOrds.get(bucketOrd), bucketDocCount(bucketOrd), subAggregationResults[bucketOrd]));
}

return new InternalAggregation[] { resultBuilder.apply(buckets) };
}

/**
* Build aggregation results for an aggregator with a varying number of
* {@code long} keyed buckets that is at the top level or wrapped in
* {@link AggregatorFactory#asMultiBucketAggregator}.
* {@code long} keyed buckets.
* @param owningBucketOrds owning bucket ordinals for which to build the results
* @param bucketOrds hash of values to the bucket ordinal
*/
Expand Down
4 changes: 0 additions & 4 deletions ...ain/java/org/elasticsearch/search/profile/aggregation/InternalAggregationProfileTree.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
package org.elasticsearch.search.profile.aggregation;

import org.elasticsearch.search.aggregations.Aggregator;
import org.elasticsearch.search.aggregations.AggregatorFactory.MultiBucketAggregatorWrapper;
import org.elasticsearch.search.profile.AbstractInternalProfileTree;

public class InternalAggregationProfileTree extends AbstractInternalProfileTree<AggregationProfileBreakdown, Aggregator> {
Expand All @@ -38,9 +37,6 @@ protected String getTypeFromElement(Aggregator element) {
if (element.getClass().getSimpleName().isEmpty()) {
return element.getClass().getSuperclass().getSimpleName();
}
if (element instanceof MultiBucketAggregatorWrapper) {
return ((MultiBucketAggregatorWrapper) element).getWrappedClass().getSimpleName();
}
Class<?> enclosing = element.getClass().getEnclosingClass();
if (enclosing != null) {
return enclosing.getSimpleName() + "." + element.getClass().getSimpleName();
Expand Down
77 changes: 0 additions & 77 deletions ...rc/test/java/org/elasticsearch/search/aggregations/MultiBucketAggregatorWrapperTests.java
View file

This file was deleted.

0 comments on commit 335505c

Please sign in to comment.