Skip to content

Remove "extractVariables" phase from Painless user tree #51690

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

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

jdconrad
Copy link
Contributor

Removes the "extractVariables" phase from the Painless user tree which is no longer necessary. The information to retrieve used variables is now collected during the semantic ("analysis") phase and passed back through ScriptRoot to generate the appropriate needs methods in the factories.

@jdconrad jdconrad added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >refactoring v8.0.0 labels Jan 30, 2020
@elasticmachine
Copy link
Collaborator

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

@@ -159,6 +141,8 @@ public ScriptRoot analyze(PainlessLookup painlessLookup, CompilerSettings settin
allEscape = statement.allEscape;
}

scriptRoot.setUsedVariables(functionScope.getUsedVariables());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is functionScope the container for these? They used to be spread all over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question as this is in transition. It was easier to move it a single place for this PR since we only care about which function scope variables are used, but it turns out with removal of some of the custom code for the execute method in both trees it will be required throughout all scopes. This will be updated in future PRs, so I would prefer to not change it here.

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.

nice step

@jdconrad
Copy link
Contributor Author

@stu-elastic Thanks for the review!

@jdconrad jdconrad merged commit c3767b1 into elastic:master Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >refactoring v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants