-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
Correctly identify and report data conflicts when deleting rows and/or columns during a three-way merge. #6980
Conversation
…ere one row was deleted and the other was modified.
…ved from the result. That way we can still detect conflicts in those columns.
…hree way merge. This ensures we don't try to compute a merged row that would violate constraints in the final schema.
…vert address columns to a type that can be used in a primary key.
a37c86f
to
9edbe39
Compare
…ate these tests to include an actual merge conflict.
…and clarify that simultanious row and column deletes are no longer a conflict.
85d9b91
to
b064516
Compare
…behavior that was in the test (row deletion + column deletion) is no longer a conflict. This commit changes the behavior to something else that is a conflict.
…preemptively migrate the left side of the merge.
b064516
to
32fd66d
Compare
…left-side row unless we actually need to (because the schema changed.)
Regression test is not included because it led to discovery of #7014
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.
Nice work on this one Nick. I think you found a good approach here and had clean changes. Only a few very minor comments from me.
@@ -130,7 +109,7 @@ func mergeProllyTable(ctx context.Context, tm *TableMerger, mergedSch schema.Sch | |||
// if not, they are recorded as conflicts in the table's artifacts. If |rebuildIndexes| is set to |
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.
(minor) Would be good to update the godocs that reference the renamed rebuildIndexes
parameter and mention anything important about rebuildPrimaryIndex
that isn't obvious from the name.
@@ -420,17 +424,17 @@ func (si *schemaImpl) GetKeyDescriptor() val.TupleDesc { | |||
sqlType := col.TypeInfo.ToSqlType() | |||
queryType := sqlType.Type() | |||
var t val.Type | |||
if queryType == query.Type_BLOB { | |||
if convertAddressColumns && queryType == query.Type_BLOB { |
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.
Not related to your changes... but... JSON
is also an address type (as of very recently at least!). If it's possible to add that in cleanly here, that would be awesome, if you aren't comfortable rolling that into this PR, could you plz open an issue to track adding it here so we don't forget to update this?
Fixes #6747
Also fixes #6766
#6747 is important to fix because ignoring data conflicts means that a merge could put a user in a state that wouldn't be possible from a linear sequence of transactions. Issues like this (especially ones that don't display any warning to the user) need to be addressed before they cause problems.
Most of this PR is refactoring how we do three way merging in a way that makes these fixes possible.
Prior to this PR, we would handle schema merges by converting the left side of the merge to match the result schema, and then processing each diff. However, this causes ambiguity when the right side drops a column: it is no longer possible to distinguish between merges where the left side modified the dropped column and merges where it didn't. It is no longer possible to reliably determine whether or not there was a data conflict.
This PR changes the approach so that, in the event of a schema merge, each row gets a Diff object which is processed during three way merging. This requires every part of the merge process to handle
DiffOpLeftAdd
andDiffOpLeftModify
, diff types that were previously handled before the merge and thus could be safely ignored.This shouldn't cause an impact in runtime: running the three way diff in the event of a schema merge is now O(N) on the size of the left table, but prior to this PR, preparing to run the three way diff in the event of a schema merge was O(N) on the size of the left table: the total runtime should be unaffected.
I added several more tests to gain confidence in the correctness of these changes. It's possible that, if there's an oversight, diffs where there isn't a schema change may also result in O(Left Rows) runtime. If this is the case it should be easy to fix, but it's something to keep an eye out for.