Prevent panic when dropping columns in schema merge #7821
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
…e and modified in the other: if the only change is the column tag, we allow the merge to proceed. Tags are useful for detecting column renames, but should not otherwise affect the merge result.
|
@nicktobey DOLT
|
zachmu
approved these changes
May 8, 2024
| @@ -223,14 +223,14 @@ func OutputProllyNode(ctx context.Context, w io.Writer, node Node, ns NodeStore, | |||
| w.Write([]byte(", ")) | |||
| } | |||
|
|
|||
| isAddr := val.IsAddrEncoding(kd.Types[i].Enc) | |||
There was a problem hiding this comment.
This is kind of alarming, good catch.
There was a problem hiding this comment.
I think this code only gets hit in tests, and only when tests fail. So we weren't seeing it.
| @@ -717,13 +707,11 @@ var columnDefaultTests = []schemaMergeTest{ | |||
| // one side changes columns, the other inserts rows | |||
| { | |||
| // TODO: this test silently does the wrong thing without erroring | |||
|
@nicktobey DOLT
|
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 #7762
In certain cases, performing a schema merge when the merged schema had fewer columns than the base schema would cause a panic.
We actually had a test for this, but the test was disabled because a limitation in how the test harness generated column tags was causing incorrect detection of merge conflicts.
To re-enable these tests, this PR slightly relaxes the logic for merge conflicts wrt column tags. This is safe to do because column tags shouldn't influence the result of merges outside of helping to identify renamed columns, so long as the merge behaves the same in both directions.