fix(plan): fix logic bug in planner helper method #5115
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
scbrickley
reviewed
Aug 19, 2022
Comment on lines
+268
to
+269
| for _, pred := range merged.Predecessors() { | ||
| for i, succ := range pred.Successors() { |
There was a problem hiding this comment.
So the bug was caused by this code block using the wrong index? Do I have that right?
There was a problem hiding this comment.
Yes, that's right. We are looking at the predecessors of the node being replaced, and updating them to point at the new node. But we were using the index from the list of predecessors (which we are not changing) instead of the index from the predecessors' successors.
scbrickley
approved these changes
Aug 19, 2022
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.
Fixes #5114.
This PR addresses an issue that has existed for a long time, but did not manifest until #5047 was merged.
The issue was in a
planhelper method that merges two nodes. If the predecessor to one of the nodes had a predecessor with multiple successors, we were not updating edges correctly and we put the plan graph into an inconsistent state.Previously this issue could not be hit because we did not allow many rules to match in multi-successor scenarios, and this bug is particular to that situation.
Checklist
Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.
experimental/(N/A)docs/Spec.mdhas been updated (N/A)Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.