Conversation
0f09ae3 to
92f2a56
Compare
…nto `store/prolly/tree` The z encoding logic could probably be pushed further down the dependency tree into `store/val` but there's no need at the moment.
ef17d46 to
735bca1
Compare
…Bytes` for use in `noms show`
735bca1 to
8134e85
Compare
fulghum
approved these changes
Jan 2, 2024
Contributor
fulghum
left a comment
There was a problem hiding this comment.
Looks great! Just a few minor comments. Overall, very clean.
7e33634 to
2da3e23
Compare
…be able to toggle expensive JSON merging with a session variable.
fulghum
approved these changes
Jan 8, 2024
Contributor
fulghum
left a comment
There was a problem hiding this comment.
Flag/var to opt-out of json merging looks good!
This file contains hidden or 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
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.
I recommend looking at one commit at a time.
This PR implemented the "Primary Goal" outlined in this doc.
Basically, when two branches in a merge both modify the same JSON value, but touch different keys, dolt should be able to automatically resolve the merge by doing a Three Way Merge that compares the value on both branches to the value at a common ancestor.
This implementation deviates from the strategy outlined in the doc in one major way: when a branch contains an edit to a nested object, we don't run the diff or merge algorithms recursively. Instead, we generate a diff that contains a JSON path to the modified object. Doing this allows us to generate all the diffs in a single linear pass, and is still easy to reason about what changes are conflicts.
We discussed only performing this merge on JSON objects below a certain size for performance reasons, but the size of the object isn't actually visible to the merge algorithm. Given that this algorithm only runs for rows that would otherwise have a merge conflict, all dolt merges that succeed before this PR should see zero performance impact from this change, so I'm not worried about performance.