Skip to content

Aggs: Add validation to Bucket script pipeline agg#132320

Merged
ivancea merged 6 commits intoelastic:mainfrom
ivancea:aggs-fix-bucket-script-validation
Aug 4, 2025
Merged

Aggs: Add validation to Bucket script pipeline agg#132320
ivancea merged 6 commits intoelastic:mainfrom
ivancea:aggs-fix-bucket-script-validation

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.
ivancea added a commit that referenced this pull request Jan 16, 2026
Continuation of #132320

Fixes #136173
Fixes #137624

Added cast checks to all pipeline aggs doing casts.

Some aggs had a more specific check (Only histograms as parents). I still used the checked cast there, and added a test with the original error message
spinscale pushed a commit to spinscale/elasticsearch that referenced this pull request Jan 21, 2026
Continuation of elastic#132320

Fixes elastic#136173
Fixes elastic#137624

Added cast checks to all pipeline aggs doing casts.

Some aggs had a more specific check (Only histograms as parents). I still used the checked cast there, and added a test with the original error message
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Aggs: InternalFilter cannot be cast to class InternalMultiBucketAggregation, missing validation

4 participants