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
Apply optimizations for handling of condition checks #32123
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b61c02d
don't lose checks from refresh-only plan
jbardin fa4c652
changes are mutated during apply
jbardin eae246c
normalize empty CheckResults fields in stateV4
jbardin 19152e7
fix log mesage
jbardin eb88ccb
only add NoOp nodes with conditions
jbardin ffe2e39
avoid re-writing state for noop applies
jbardin efd7715
use key data from plan method for apply
jbardin File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically anything which has access to the
EvalContext
can get the "repetition data" like this:In the original design for this "instance expander" thing it was intended to act as the source of record for all of this expansion-related information specifically so that we wouldn't need to explicitly pass all of this contextual data around.
I do feel in two minds about it because it feels like a violation of Law of Demeter, but we already widely use
ctx
as a big bag of shared "global" context, including in this very function where it usesctx
to do various other things that aren't strictly this function's business to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I left this as close to the current code as possible. I did forget that the rep data was directly available though, because the existing apply code already re-evaluated it anyway. Switching out that code however runs into a no
expansion has been registered
panic, so I think there's something else going on here too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes I suppose the fact that we've been passing it around through arguments meant that there was not previously any need to actually register it at the source of record. Unfortunate but not unexpected.
Hopefully in a future change we can find a suitable central point to evaluate this just once and register it in the expander, so it's clearer exactly whose responsibility it is to evaluate the
for_each
orcount
expressions.