Skip to content

Remove "extractVariables" phase from Painless user tree#51690

Merged
jdconrad merged 1 commit intoelastic:masterfrom
jdconrad:trees4
Jan 31, 2020
Merged

Remove "extractVariables" phase from Painless user tree#51690
jdconrad merged 1 commit intoelastic:masterfrom
jdconrad:trees4

Conversation

@jdconrad
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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

allEscape = statement.allEscape;
}

scriptRoot.setUsedVariables(functionScope.getUsedVariables());
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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