Merge analyze and write phases in Painless "user" tree #53685
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
|
@elasticmachine test elasticsearch-ci/default-distro |
stu-elastic
reviewed
Mar 19, 2020
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/EListInit.java
Outdated
Show resolved
Hide resolved
stu-elastic
reviewed
Mar 19, 2020
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ENewArray.java
Outdated
Show resolved
Hide resolved
stu-elastic
reviewed
Mar 19, 2020
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ENewArrayFunctionRef.java
Outdated
Show resolved
Hide resolved
stu-elastic
reviewed
Mar 19, 2020
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/ENewArrayFunctionRef.java
Outdated
Show resolved
Hide resolved
stu-elastic
reviewed
Mar 19, 2020
| private int radix; | ||
|
|
||
| protected Object constant; | ||
| protected final String value; |
There was a problem hiding this comment.
If these aren't going to be the extension point, no need to change visibility.
There was a problem hiding this comment.
If it's all right I would like to leave this for now as extending the nodes is currently the only way to do an extension point in the user tree. As other design ideas are explored, it seems more appropriate to modify them then.
Edit: especially, as a better design is not a guarantee right now
stu-elastic
reviewed
Mar 19, 2020
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/PBrace.java
Outdated
Show resolved
Hide resolved
stu-elastic
reviewed
Mar 19, 2020
modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SFunction.java
Outdated
Show resolved
Hide resolved
|
@stu-elastic Thanks for the review. I've responded to all PR comments, so I think this is ready for the next round. |
|
@stu-elastic Thanks again. Will merge as soon as CI passes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the next step in producing immutable state in the Painless "user" tree. This combines the analyze phase and the write phase so that both semantic checking and the ir tree are generated in the same phase. This allows the removal of the Input/Output objects created in the previous PRs from being mutable state on the nodes. Instead they are now local state for the analyze phase.
Relates: #53561
Relates: #49869