Skip to content

Conversation

@ivancea
Copy link
Contributor

@ivancea ivancea commented Aug 1, 2025

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.

I checked for the validate() method, but I couldn't find a sane way to check that the agg builder is a MultiBucket agg. There's an InternalMultiBucketAgg interface, but not one for the builders. I could add it, but I'm not sure it would be worth the time.

@ivancea ivancea requested a review from nik9000 August 1, 2025 10:34
@ivancea ivancea added >bug :Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged v9.2.0 v9.1.1 v8.19.1 v9.0.5 v8.17.10 v8.18.5 labels Aug 1, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @ivancea, I've created a changelog YAML for you.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I'm ok with it. I'd ask @not-napoleon if he can think of a better way, but this is A way and that's better than a weird exception.

@ivancea ivancea marked this pull request as ready for review August 1, 2025 14:51
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@ivancea ivancea requested a review from not-napoleon August 1, 2025 14:52
Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

If we don't want to support this (and I assume we don't, but that's more your call at this point), then I think this is a fine solution. As @nik9000 noted, it's better than the class cast exception.

I would like a yaml test with the reproducing query from the original issue. I see there's an Aggregator Test for it, which is good, but I would like both.

@ivancea ivancea enabled auto-merge (squash) August 4, 2025 11:04
@ivancea ivancea merged commit ec82abd into elastic:main Aug 4, 2025
33 checks passed
@ivancea ivancea deleted the aggs-fix-bucket-script-validation branch August 4, 2025 13:16
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.1 Commit could not be cherrypicked due to conflicts
8.19 Commit could not be cherrypicked due to conflicts
9.0 Commit could not be cherrypicked due to conflicts
8.17 Commit could not be cherrypicked due to conflicts
8.18 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 132320

ivancea added a commit to ivancea/elasticsearch that referenced this pull request Aug 5, 2025
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.
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Aug 5, 2025
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.
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Aug 5, 2025
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.
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Aug 5, 2025
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.
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Aug 5, 2025
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.
szybia added a commit to szybia/elasticsearch that referenced this pull request Aug 5, 2025
…cking * upstream/main: (26 commits) [Fleet] add privileges to `kibana_system` to read integrations data (elastic#132400) Add `TestEntitlementsRule` with support for dynamic entitled node paths for testing (elastic#132077) Reduce logging frequency for GCS per project clients (elastic#132429) Skip update/100_synthetic_source tests in yamlRestCompatTests (elastic#132296) Correct exception for missing nested path (elastic#132408) Fixing esql release tests elastic#132369 (elastic#132406) Adjust date docvalue formatting to return 4xx instead of 5xx (elastic#132414) Handle nested fields with the termvectors REST API in artificial docs (elastic#92568) Only collect bulk scored vectors when exceeding min competitive (elastic#132293) Fix release tests diskbbq update (elastic#132405) ESQL: Fix skipping of generative tests (elastic#132390) Short circuit failure handling in OIDC flow (elastic#130618) Small optimization in OptimizedScalarQuantizer by using mul instead of div (elastic#132397) Aggs: Add validation to Bucket script pipeline agg (elastic#132320) ESQL: Multiple parameters in ungrouped aggs (elastic#132375) ESQL: Explain test operators (elastic#132374) EQL: Deal with internally created IN in a different way for EQL (elastic#132167) Speed up hierarchical k-means by computing distances in bulk (elastic#132384) Reduce the number of fields per document (elastic#132322) Assert current thread in ESQL (elastic#132324) ...
elasticsearchmachine pushed a commit that referenced this pull request Aug 5, 2025
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.
elasticsearchmachine pushed a commit that referenced this pull request Aug 5, 2025
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.
elasticsearchmachine pushed a commit that referenced this pull request Aug 5, 2025
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.
elasticsearchmachine pushed a commit that referenced this pull request Aug 5, 2025
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.
elasticsearchmachine pushed a commit that referenced this pull request Aug 5, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations auto-backport Automatically create backport pull requests when merged >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.17.10 v8.18.5 v8.19.1 v9.0.5 v9.1.1 v9.2.0

4 participants