Skip to content
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.

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

Add Fields API to aggregation scripts and field scripts #76325

Merged
merged 9 commits into from
Aug 11, 2021

Conversation

jdconrad
Copy link
Contributor

This change updates the aggregation script, map script for aggregations, and field scripts to extend DocBasedScript to give them access to the new fields api.

@jdconrad jdconrad added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 auto-backport Automatically create backport pull requests when merged v7.15.0 labels Aug 10, 2021
@jdconrad jdconrad requested a review from stu-elastic August 10, 2021 21:30
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Aug 10, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@@ -140,6 +143,24 @@
filter.add(filterWhitelist);
map.put(FilterScript.CONTEXT, filter);

List<Whitelist> aggregation = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point we should have something like getRuntimeFieldWhitelist for this API since a lot of this config code is the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I would prefer to do this as a follow up PR. I even wonder if this should maybe be moved elsewhere. The entirety of including these contexts as part of the module is a bit of an afterthought, though I understand doing them through SPI doesn't make sense here given the dependency hierarchy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well we should cleanup the whitelisting code. Next PR is fine, but if we wanna do a more fundamental change then I'd prefer the wl code cleanup happen first.

Copy link
Contributor

@stu-elastic stu-elastic left a comment

Choose a reason for hiding this comment

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

very nice, a couple of requests.

@jdconrad
Copy link
Contributor Author

@stu-elastic Thanks for the review! I believe I've addressed your comment and will commit once CI passes.

@jdconrad
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-1

@jdconrad jdconrad merged commit a48c736 into elastic:master Aug 11, 2021
elasticsearchmachine pushed a commit to elasticsearchmachine/elasticsearch that referenced this pull request Aug 11, 2021
This change updates the aggregation script, map script for aggregations, and field scripts to extend 
DocBasedScript to give them access to the new fields api.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.x

jdconrad pushed a commit that referenced this pull request Aug 11, 2021
)

This change updates the aggregation script, map script for aggregations, and field scripts to extend 
DocBasedScript to give them access to the new fields api.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement Team:Core/Infra Meta label for core/infra team v7.15.0 v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants