Skip to content

Commit 1fe6500

Browse files
authored
Aggs: Add validation to Bucket script pipeline agg (elastic#132320) (elastic#132439)
Closes elastic#132272 Docs are explicit on what the bucket_script agg requires: > A parent pipeline aggregation which executes a script which can perform per bucket computations on specified metrics in the parent **_multi-bucket aggregation_** But it's missing a validation.
1 parent 1df6ac2 commit 1fe6500

File tree

5 files changed

+91
-4
lines changed

5 files changed

+91
-4
lines changed

docs/changelog/132320.yaml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 132320
2+
summary: "Aggs: Add validation to Bucket script pipeline agg"
3+
area: Aggregations
4+
type: bug
5+
issues:
6+
- 132272

modules/aggregations/src/yamlRestTest/resources/rest-api-spec/test/aggregations/bucket_script.yml

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,3 +340,31 @@ top level fails:
340340
buckets_path:
341341
b: b
342342
script: params.b + 12
343+
344+
---
345+
invalid parent aggregation:
346+
- requires:
347+
capabilities:
348+
- method: POST
349+
path: /_search
350+
capabilities: [ bucket_script_parent_multi_bucket_error ]
351+
test_runner_features: [capabilities]
352+
reason: "changed error 500 to 400"
353+
- do:
354+
catch: /Expected a multi bucket aggregation but got \[InternalFilter\] for aggregation \[d\]/
355+
search:
356+
body:
357+
aggs:
358+
a:
359+
filter:
360+
term:
361+
a: 1
362+
aggs:
363+
b:
364+
sum:
365+
field: b
366+
d:
367+
bucket_script:
368+
buckets_path:
369+
b: b
370+
script: params.b + 12

server/src/main/java/org/elasticsearch/rest/action/search/SearchCapabilities.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ private SearchCapabilities() {}
4848
private static final String SIGNIFICANT_TERMS_BACKGROUND_FILTER_AS_SUB = "significant_terms_background_filter_as_sub";
4949

5050
private static final String SIGNIFICANT_TERMS_ON_NESTED_FIELDS = "significant_terms_on_nested_fields";
51+
private static final String BUCKET_SCRIPT_PARENT_MULTI_BUCKET_ERROR = "bucket_script_parent_multi_bucket_error";
5152

5253
public static final Set<String> CAPABILITIES;
5354
static {
@@ -67,6 +68,7 @@ private SearchCapabilities() {}
6768
capabilities.add(HIGHLIGHT_MAX_ANALYZED_OFFSET_DEFAULT);
6869
capabilities.add(SIGNIFICANT_TERMS_BACKGROUND_FILTER_AS_SUB);
6970
capabilities.add(SIGNIFICANT_TERMS_ON_NESTED_FIELDS);
71+
capabilities.add(BUCKET_SCRIPT_PARENT_MULTI_BUCKET_ERROR);
7072
CAPABILITIES = Set.copyOf(capabilities);
7173
}
7274
}

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregator.java

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.ArrayList;
2222
import java.util.HashMap;
2323
import java.util.List;
24+
import java.util.Locale;
2425
import java.util.Map;
2526
import java.util.stream.Collectors;
2627
import java.util.stream.StreamSupport;
@@ -49,10 +50,24 @@ public class BucketScriptPipelineAggregator extends PipelineAggregator {
4950
}
5051

5152
@Override
53+
@SuppressWarnings({ "rawtypes", "unchecked" })
5254
public InternalAggregation reduce(InternalAggregation aggregation, AggregationReduceContext reduceContext) {
53-
@SuppressWarnings({ "rawtypes", "unchecked" })
54-
InternalMultiBucketAggregation<InternalMultiBucketAggregation, InternalMultiBucketAggregation.InternalBucket> originalAgg =
55-
(InternalMultiBucketAggregation<InternalMultiBucketAggregation, InternalMultiBucketAggregation.InternalBucket>) aggregation;
55+
56+
InternalMultiBucketAggregation<InternalMultiBucketAggregation, InternalMultiBucketAggregation.InternalBucket> originalAgg;
57+
58+
if (aggregation instanceof InternalMultiBucketAggregation multiBucketAggregation) {
59+
originalAgg = multiBucketAggregation;
60+
} else {
61+
throw new IllegalArgumentException(
62+
String.format(
63+
Locale.ROOT,
64+
"Expected a multi bucket aggregation but got [%s] for aggregation [%s]",
65+
aggregation.getClass().getSimpleName(),
66+
name()
67+
)
68+
);
69+
}
70+
5671
List<? extends InternalMultiBucketAggregation.InternalBucket> buckets = originalAgg.getBuckets();
5772

5873
BucketAggregationScript.Factory factory = reduceContext.scriptService().compile(script, BucketAggregationScript.CONTEXT);

server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptAggregatorTests.java

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,9 @@
3030
import org.elasticsearch.script.ScriptModule;
3131
import org.elasticsearch.script.ScriptService;
3232
import org.elasticsearch.script.ScriptType;
33+
import org.elasticsearch.search.aggregations.AggregationBuilder;
3334
import org.elasticsearch.search.aggregations.AggregatorTestCase;
35+
import org.elasticsearch.search.aggregations.bucket.filter.FilterAggregationBuilder;
3436
import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregationBuilder;
3537
import org.elasticsearch.search.aggregations.bucket.filter.InternalFilters;
3638
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
@@ -95,8 +97,42 @@ public void testScript() throws IOException {
9597
);
9698
}
9799

100+
public void testNonMultiBucketParent() {
101+
MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType("number_field", NumberFieldMapper.NumberType.INTEGER);
102+
MappedFieldType fieldType1 = new KeywordFieldMapper.KeywordFieldType("the_field");
103+
104+
FilterAggregationBuilder filter = new FilterAggregationBuilder("placeholder", new MatchAllQueryBuilder()).subAggregation(
105+
new TermsAggregationBuilder("the_terms").userValueTypeHint(ValueType.STRING)
106+
.field("the_field")
107+
.subAggregation(new AvgAggregationBuilder("the_avg").field("number_field"))
108+
)
109+
.subAggregation(
110+
new BucketScriptPipelineAggregationBuilder(
111+
"bucket_script",
112+
Collections.singletonMap("the_avg", "the_terms['test1']>the_avg.value"),
113+
new Script(ScriptType.INLINE, MockScriptEngine.NAME, SCRIPT_NAME, Collections.emptyMap())
114+
)
115+
);
116+
117+
assertThrows(
118+
"Expected a multi bucket aggregation but got [InternalFilter] for aggregation [bucket_script]",
119+
IllegalArgumentException.class,
120+
() -> testCase(filter, new MatchAllDocsQuery(), iw -> {
121+
Document doc = new Document();
122+
doc.add(new SortedSetDocValuesField("the_field", new BytesRef("test1")));
123+
doc.add(new SortedNumericDocValuesField("number_field", 19));
124+
iw.addDocument(doc);
125+
126+
doc = new Document();
127+
doc.add(new SortedSetDocValuesField("the_field", new BytesRef("test2")));
128+
doc.add(new SortedNumericDocValuesField("number_field", 55));
129+
iw.addDocument(doc);
130+
}, f -> fail("This shouldn't be called"), fieldType, fieldType1)
131+
);
132+
}
133+
98134
private void testCase(
99-
FiltersAggregationBuilder aggregationBuilder,
135+
AggregationBuilder aggregationBuilder,
100136
Query query,
101137
CheckedConsumer<RandomIndexWriter, IOException> buildIndex,
102138
Consumer<InternalFilters> verify,

0 commit comments

Comments
 (0)