Skip to content

Commit ec82abd

Browse files
authored
Aggs: Add validation to Bucket script pipeline agg (#132320)
Closes #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 3fea67d commit ec82abd

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
@@ -58,6 +58,7 @@ private SearchCapabilities() {}
5858
private static final String SYNTHETIC_VECTORS_SETTING = "synthetic_vectors_setting";
5959
private static final String UPDATE_FIELD_TO_BBQ_DISK = "update_field_to_bbq_disk";
6060
private static final String KNN_FILTER_ON_NESTED_FIELDS_CAPABILITY = "knn_filter_on_nested_fields";
61+
private static final String BUCKET_SCRIPT_PARENT_MULTI_BUCKET_ERROR = "bucket_script_parent_multi_bucket_error";
6162

6263
public static final Set<String> CAPABILITIES;
6364
static {
@@ -84,6 +85,7 @@ private SearchCapabilities() {}
8485
capabilities.add(FIELD_EXISTS_QUERY_FOR_TEXT_FIELDS_NO_INDEX_OR_DV);
8586
capabilities.add(UPDATE_FIELD_TO_BBQ_DISK);
8687
capabilities.add(KNN_FILTER_ON_NESTED_FIELDS_CAPABILITY);
88+
capabilities.add(BUCKET_SCRIPT_PARENT_MULTI_BUCKET_ERROR);
8789
if (SYNTHETIC_VECTORS) {
8890
capabilities.add(SYNTHETIC_VECTORS_SETTING);
8991
}

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

2627
import static org.elasticsearch.search.aggregations.pipeline.BucketHelpers.resolveBucketValue;
@@ -47,10 +48,24 @@ public class BucketScriptPipelineAggregator extends PipelineAggregator {
4748
}
4849

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

5671
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
@@ -31,7 +31,9 @@
3131
import org.elasticsearch.script.ScriptModule;
3232
import org.elasticsearch.script.ScriptService;
3333
import org.elasticsearch.script.ScriptType;
34+
import org.elasticsearch.search.aggregations.AggregationBuilder;
3435
import org.elasticsearch.search.aggregations.AggregatorTestCase;
36+
import org.elasticsearch.search.aggregations.bucket.filter.FilterAggregationBuilder;
3537
import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregationBuilder;
3638
import org.elasticsearch.search.aggregations.bucket.filter.InternalFilters;
3739
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
@@ -102,8 +104,42 @@ public void testScript() throws IOException {
102104
);
103105
}
104106

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

0 commit comments

Comments
 (0)