Skip to content

Inline redundant SingleBucketAggregation interface #127093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Inline redundant SingleBucketAggregation interface #127093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
74f6e0e
Inline redundant SingleBucketAggregation interface
original-brownbear Apr 19, 2025
0349803
fix tests
original-brownbear Apr 19, 2025
997dc84
Merge remote-tracking branch 'elastic/main' into drop-redundant-singl…
original-brownbear Apr 19, 2025
b6bbb1b
fix more tests
original-brownbear Apr 19, 2025
2d54515
fix more tests
original-brownbear Apr 19, 2025
bc65672
Merge branch 'main' into drop-redundant-single-b-interface
original-brownbear Apr 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/InternalChildren.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@

import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.bucket.InternalSingleBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;

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

/**
* Results of the {@link ParentToChildrenAggregator}.
*/
public class InternalChildren extends InternalSingleBucketAggregation {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation in cleaning this up shoud become visible here, all but one implementation of SingleBucketAggregation are noop overrides. We can (with a bit of BwC code for the transport layer) fold all of these into a single class and save both a lot of code but also make serialization a lot more compact (and faster!).

public class InternalChildren extends SingleBucketAggregation {
public InternalChildren(String name, long docCount, InternalAggregations aggregations, Map<String, Object> metadata) {
super(name, docCount, aggregations, metadata);
}
Expand All @@ -37,7 +37,7 @@ public String getWriteableName() {
}

@Override
protected InternalSingleBucketAggregation newAggregation(String name, long docCount, InternalAggregations subAggregations) {
protected SingleBucketAggregation newAggregation(String name, long docCount, InternalAggregations subAggregations) {
return new InternalChildren(name, docCount, subAggregations, getMetadata());
}
}
6 changes: 3 additions & 3 deletions modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/InternalParent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@

import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.bucket.InternalSingleBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;

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

/**
* Results of the {@link ChildrenToParentAggregator}.
*/
public class InternalParent extends InternalSingleBucketAggregation {
public class InternalParent extends SingleBucketAggregation {
public InternalParent(String name, long docCount, InternalAggregations aggregations, Map<String, Object> metadata) {
super(name, docCount, aggregations, metadata);
}
Expand All @@ -37,7 +37,7 @@ public String getWriteableName() {
}

@Override
protected InternalSingleBucketAggregation newAggregation(String name, long docCount, InternalAggregations subAggregations) {
protected SingleBucketAggregation newAggregation(String name, long docCount, InternalAggregations subAggregations) {
return new InternalParent(name, docCount, subAggregations, getMetadata());
}
}
4 changes: 3 additions & 1 deletion ...in/src/main/java/org/elasticsearch/join/aggregations/JoinAggregationInspectionHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
*/
package org.elasticsearch.join.aggregations;

import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;

/**
* Counterpart to {@link org.elasticsearch.search.aggregations.support.AggregationInspectionHelper}, providing
* helpers for some aggs in the Join package
Expand All @@ -18,7 +20,7 @@ public static boolean hasValue(InternalParent agg) {
return agg.getDocCount() > 0;
}

public static boolean hasValue(InternalChildren agg) {
public static boolean hasValue(SingleBucketAggregation agg) {
return agg.getDocCount() > 0;
}
}
9 changes: 5 additions & 4 deletions .../parent-join/src/test/java/org/elasticsearch/join/aggregations/InternalChildrenTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,21 @@
import org.elasticsearch.join.ParentJoinPlugin;
import org.elasticsearch.plugins.SearchPlugin;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.InternalSingleBucketAggregationTestCase;
import org.elasticsearch.search.aggregations.SingleBucketAggregationTestCase;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;

import java.util.List;
import java.util.Map;

public class InternalChildrenTests extends InternalSingleBucketAggregationTestCase<InternalChildren> {
public class InternalChildrenTests extends SingleBucketAggregationTestCase<SingleBucketAggregation> {

@Override
protected SearchPlugin registerPlugin() {
return new ParentJoinPlugin();
}

@Override
protected InternalChildren createTestInstance(
protected SingleBucketAggregation createTestInstance(
String name,
long docCount,
InternalAggregations aggregations,
Expand All @@ -35,7 +36,7 @@ protected InternalChildren createTestInstance(
}

@Override
protected void extraAssertReduced(InternalChildren reduced, List<InternalChildren> inputs) {
protected void extraAssertReduced(SingleBucketAggregation reduced, List<SingleBucketAggregation> inputs) {
// Nothing extra to assert
}
}
4 changes: 2 additions & 2 deletions ...es/parent-join/src/test/java/org/elasticsearch/join/aggregations/InternalParentTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@
import org.elasticsearch.join.ParentJoinPlugin;
import org.elasticsearch.plugins.SearchPlugin;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.InternalSingleBucketAggregationTestCase;
import org.elasticsearch.search.aggregations.SingleBucketAggregationTestCase;

import java.util.List;
import java.util.Map;

public class InternalParentTests extends InternalSingleBucketAggregationTestCase<InternalParent> {
public class InternalParentTests extends SingleBucketAggregationTestCase<InternalParent> {

@Override
protected SearchPlugin registerPlugin() {
Expand Down
11 changes: 6 additions & 5 deletions ...in/src/test/java/org/elasticsearch/join/aggregations/ParentToChildrenAggregatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.elasticsearch.search.aggregations.AggregationBuilder;
import org.elasticsearch.search.aggregations.Aggregator;
import org.elasticsearch.search.aggregations.AggregatorTestCase;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.terms.StringTerms;
import org.elasticsearch.search.aggregations.bucket.terms.Terms;
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
Expand Down Expand Up @@ -154,14 +155,14 @@ public void testParentChildAsSubAgg() throws IOException {
StringTerms result = searchAndReduce(indexReader, new AggTestConfig(request, withJoinFields(longField("number"), kwd)));

StringTerms.Bucket evenBucket = result.getBucketByKey("even");
InternalChildren evenChildren = evenBucket.getAggregations().get("children");
SingleBucketAggregation evenChildren = evenBucket.getAggregations().get("children");
Min evenMin = evenChildren.getAggregations().get("min");
assertThat(evenChildren.getDocCount(), equalTo(expectedEvenChildCount));
assertThat(evenMin.value(), equalTo(expectedEvenMin));

if (expectedOddChildCount > 0) {
StringTerms.Bucket oddBucket = result.getBucketByKey("odd");
InternalChildren oddChildren = oddBucket.getAggregations().get("children");
SingleBucketAggregation oddChildren = oddBucket.getAggregations().get("children");
Min oddMin = oddChildren.getAggregations().get("min");
assertThat(oddChildren.getDocCount(), equalTo(expectedOddChildCount));
assertThat(oddMin.value(), equalTo(expectedOddMin));
Expand Down Expand Up @@ -224,7 +225,7 @@ public void testBestDeferringCollectorWithSubAggOfChildrenAggNeedingScores() thr

var fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);
var fieldType2 = new KeywordFieldMapper.KeywordFieldType("string_field", false, true, Map.of());
InternalChildren result = searchAndReduce(
SingleBucketAggregation result = searchAndReduce(
indexReader,
new AggTestConfig(aggregationBuilder, withJoinFields(fieldType, fieldType2)).withQuery(
new TermQuery(new Term("join_field", "parent_type"))
Expand Down Expand Up @@ -291,13 +292,13 @@ private static SortedDocValuesField createJoinField(String parentType, String id
return new SortedDocValuesField("join_field#" + parentType, new BytesRef(id));
}

private void testCase(Query query, IndexReader indexReader, Consumer<InternalChildren> verify) throws IOException {
private void testCase(Query query, IndexReader indexReader, Consumer<SingleBucketAggregation> verify) throws IOException {

ChildrenAggregationBuilder aggregationBuilder = new ChildrenAggregationBuilder("_name", CHILD_TYPE);
aggregationBuilder.subAggregation(new MinAggregationBuilder("in_child").field("number"));

MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number", NumberFieldMapper.NumberType.LONG);
InternalChildren result = searchAndReduce(
SingleBucketAggregation result = searchAndReduce(
indexReader,
new AggTestConfig(aggregationBuilder, withJoinFields(fieldType)).withQuery(query)
);
Expand Down
2 changes: 1 addition & 1 deletion ...nalClusterTest/java/org/elasticsearch/search/aggregations/bucket/terms/StringTermsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public static class CustomScriptPlugin extends AggregationTestScriptsPlugin {
protected Map<String, Function<Map<String, Object>, Object>> pluginScripts() {
Map<String, Function<Map<String, Object>, Object>> scripts = super.pluginScripts();

scripts.put("'foo_' + _value", vars -> "foo_" + (String) vars.get("_value"));
scripts.put("'foo_' + _value", vars -> "foo_" + vars.get("_value"));
scripts.put("_value.substring(0,3)", vars -> ((String) vars.get("_value")).substring(0, 3));

scripts.put("doc['" + MULTI_VALUED_FIELD_NAME + "']", vars -> {
Expand Down
39 changes: 13 additions & 26 deletions ...ternalClusterTest/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.aggregations.AggregationTestScriptsPlugin;
import org.elasticsearch.search.aggregations.BucketOrder;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
import org.elasticsearch.search.aggregations.bucket.terms.Terms;
Expand Down Expand Up @@ -293,57 +292,45 @@ public void testSingleValuedFieldGetProperty() throws Exception {
ExtendedStats stats = global.getAggregations().get("stats");
assertThat(stats, notNullValue());
assertThat(stats.getName(), equalTo("stats"));
ExtendedStats statsFromProperty = (ExtendedStats) ((InternalAggregation) global).getProperty("stats");
ExtendedStats statsFromProperty = (ExtendedStats) global.getProperty("stats");
assertThat(statsFromProperty, notNullValue());
assertThat(statsFromProperty, sameInstance(stats));
double expectedAvgValue = (double) (1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10) / 10;
assertThat(stats.getAvg(), equalTo(expectedAvgValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.avg"), equalTo(expectedAvgValue));
assertThat((double) global.getProperty("stats.avg"), equalTo(expectedAvgValue));
double expectedMinValue = 1.0;
assertThat(stats.getMin(), equalTo(expectedMinValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.min"), equalTo(expectedMinValue));
assertThat((double) global.getProperty("stats.min"), equalTo(expectedMinValue));
double expectedMaxValue = 10.0;
assertThat(stats.getMax(), equalTo(expectedMaxValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.max"), equalTo(expectedMaxValue));
assertThat((double) global.getProperty("stats.max"), equalTo(expectedMaxValue));
double expectedSumValue = 1 + 2 + 3 + 4 + 5 + 6 + 7 + 8 + 9 + 10;
assertThat(stats.getSum(), equalTo(expectedSumValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.sum"), equalTo(expectedSumValue));
assertThat((double) global.getProperty("stats.sum"), equalTo(expectedSumValue));
long expectedCountValue = 10;
assertThat(stats.getCount(), equalTo(expectedCountValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.count"), equalTo((double) expectedCountValue));
assertThat((double) global.getProperty("stats.count"), equalTo((double) expectedCountValue));
double expectedSumOfSquaresValue = (double) 1 + 4 + 9 + 16 + 25 + 36 + 49 + 64 + 81 + 100;
assertThat(stats.getSumOfSquares(), equalTo(expectedSumOfSquaresValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.sum_of_squares"), equalTo(expectedSumOfSquaresValue));
assertThat((double) global.getProperty("stats.sum_of_squares"), equalTo(expectedSumOfSquaresValue));
double expectedVarianceValue = variance(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
assertThat(stats.getVariance(), equalTo(expectedVarianceValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.variance"), equalTo(expectedVarianceValue));
assertThat((double) global.getProperty("stats.variance"), equalTo(expectedVarianceValue));
double expectedVariancePopulationValue = variancePopulation(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
assertThat(stats.getVariancePopulation(), equalTo(expectedVariancePopulationValue));
assertThat(
(double) ((InternalAggregation) global).getProperty("stats.variance_population"),
equalTo(expectedVariancePopulationValue)
);
assertThat((double) global.getProperty("stats.variance_population"), equalTo(expectedVariancePopulationValue));
double expectedVarianceSamplingValue = varianceSampling(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
assertThat(stats.getVarianceSampling(), equalTo(expectedVarianceSamplingValue));
assertThat(
(double) ((InternalAggregation) global).getProperty("stats.variance_sampling"),
equalTo(expectedVarianceSamplingValue)
);
assertThat((double) global.getProperty("stats.variance_sampling"), equalTo(expectedVarianceSamplingValue));
double expectedStdDevValue = stdDev(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
assertThat(stats.getStdDeviation(), equalTo(expectedStdDevValue));
assertThat((double) ((InternalAggregation) global).getProperty("stats.std_deviation"), equalTo(expectedStdDevValue));
assertThat((double) global.getProperty("stats.std_deviation"), equalTo(expectedStdDevValue));
double expectedStdDevPopulationValue = stdDevPopulation(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
assertThat(stats.getStdDeviationPopulation(), equalTo(expectedStdDevValue));
assertThat(
(double) ((InternalAggregation) global).getProperty("stats.std_deviation_population"),
equalTo(expectedStdDevPopulationValue)
);
assertThat((double) global.getProperty("stats.std_deviation_population"), equalTo(expectedStdDevPopulationValue));
double expectedStdDevSamplingValue = stdDevSampling(1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
assertThat(stats.getStdDeviationSampling(), equalTo(expectedStdDevSamplingValue));
assertThat(
(double) ((InternalAggregation) global).getProperty("stats.std_deviation_sampling"),
equalTo(expectedStdDevSamplingValue)
);
assertThat((double) global.getProperty("stats.std_deviation_sampling"), equalTo(expectedStdDevSamplingValue));
}
);
}
Expand Down
3 changes: 1 addition & 2 deletions ...lClusterTest/java/org/elasticsearch/search/aggregations/metrics/HDRPercentileRanksIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.aggregations.AggregationTestScriptsPlugin;
import org.elasticsearch.search.aggregations.BucketOrder;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
import org.elasticsearch.search.aggregations.bucket.terms.Terms;
Expand Down Expand Up @@ -226,7 +225,7 @@ public void testSingleValuedFieldGetProperty() throws Exception {
PercentileRanks values = global.getAggregations().get("percentile_ranks");
assertThat(values, notNullValue());
assertThat(values.getName(), equalTo("percentile_ranks"));
assertThat(((InternalAggregation) global).getProperty("percentile_ranks"), sameInstance(values));
assertThat(global.getProperty("percentile_ranks"), sameInstance(values));
}
);
}
Expand Down
3 changes: 1 addition & 2 deletions ...ernalClusterTest/java/org/elasticsearch/search/aggregations/metrics/HDRPercentilesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.aggregations.AggregationTestScriptsPlugin;
import org.elasticsearch.search.aggregations.BucketOrder;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
import org.elasticsearch.search.aggregations.bucket.terms.Terms;
Expand Down Expand Up @@ -203,7 +202,7 @@ public void testSingleValuedFieldGetProperty() throws Exception {
Percentiles percentiles = global.getAggregations().get("percentiles");
assertThat(percentiles, notNullValue());
assertThat(percentiles.getName(), equalTo("percentiles"));
assertThat(((InternalAggregation) global).getProperty("percentiles"), sameInstance(percentiles));
assertThat(global.getProperty("percentiles"), sameInstance(percentiles));
}
);
}
Expand Down
3 changes: 1 addition & 2 deletions ...terTest/java/org/elasticsearch/search/aggregations/metrics/MedianAbsoluteDeviationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.aggregations.AggregationTestScriptsPlugin;
import org.elasticsearch.search.aggregations.BucketOrder;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.bucket.SingleBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.histogram.Histogram;
import org.elasticsearch.search.aggregations.bucket.range.Range;
Expand Down Expand Up @@ -189,7 +188,7 @@ public void testSingleValuedFieldGetProperty() throws Exception {
final MedianAbsoluteDeviation mad = global.getAggregations().get("mad");
assertThat(mad, notNullValue());
assertThat(mad.getName(), is("mad"));
assertThat(((InternalAggregation) global).getProperty("mad"), sameInstance(mad));
assertThat(global.getProperty("mad"), sameInstance(mad));
}
);
}
Expand Down
4 changes: 2 additions & 2 deletions ...ernalClusterTest/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,8 +731,8 @@ public void testInitMapCombineReduceGetProperty() throws Exception {
assertThat(object, notNullValue());
assertThat(object, instanceOf(Number.class));
assertThat(((Number) object).longValue(), equalTo(numDocs * 3));
assertThat(((InternalAggregation) global).getProperty("scripted"), sameInstance(scriptedMetricAggregation));
assertThat((List) ((InternalAggregation) global).getProperty("scripted.value"), sameInstance(aggregationList));
assertThat(global.getProperty("scripted"), sameInstance(scriptedMetricAggregation));
assertThat((List) global.getProperty("scripted.value"), sameInstance(aggregationList));
assertThat((List) ((InternalAggregation) scriptedMetricAggregation).getProperty("value"), sameInstance(aggregationList));
}
);
Expand Down
Loading