-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
schemadiff
: supporting textual diff
#15388
schemadiff
: supporting textual diff
#15388
Conversation
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
schemadiff
: supporting textual diff
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15388 +/- ##
==========================================
+ Coverage 65.64% 65.69% +0.05%
==========================================
Files 1563 1564 +1
Lines 194389 194558 +169
==========================================
+ Hits 127602 127816 +214
+ Misses 66787 66742 -45 ☔ View full report in Codecov by Sentry. |
Will probably revert to |
b828d51
to
b23bb2d
Compare
HTML change reverted via force push. |
For this PR, I suggest not implementing HTML diff yet, until we have more clarity on how we might want to generate it (with/out syntax highlighting, full/per-line HTML, etc.). |
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! Only a few very minor comments about potential additional unit test cases. If they're relatively easy to add, probably worth it. I'll leave that up to you though.
case t1Partitions == nil: | ||
// add partitioning | ||
alterTable.PartitionOption = t2Partitions | ||
annotations.MarkAdded(sqlparser.CanonicalString(t2Partitions)) |
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.
Any reason not to add a test case for this in the unit test?
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.
Added.
} | ||
case AutoIncrementIgnore: | ||
// do not apply | ||
} | ||
default: | ||
// Apply the new options | ||
alterTableOptions = append(alterTableOptions, t2Option) | ||
modifyTableOption(t1Option, t2Option) |
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.
Seems like it's probably relatively easy to cover the default in the unit test 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.
Added.
case TableCharsetCollateIgnoreEmpty: | ||
if t1Option.String != "" && t2Option.String != "" { | ||
alterTableOptions = append(alterTableOptions, t2Option) | ||
modifyTableOption(t1Option, t2Option) |
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.
Looks like this isn't covered by the unit test.
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.
Added,
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Signed-off-by: Shlomi Noach <[email protected]>
Description
In this PR
schemadiff
is able to generate textual diffs, nuanced to diff hints and specific logic ofschemadiff
.Consider the following example. Two schemas are diffed, and the logical diff is:
On top of this logical diff,
schemadiff
will now also be able to generate this textual diff:The above is a simple example. What sets
schemadiff
apart is that the textual diff only shows whatschemadiff
thinks are ntoeworthy changes. For example,schemadiff
ignores index ordering: it considers two tables to be identical if all but order of indexes is the same. The standard textual diff of such two tables will show the indexes as+
and-
annotated entries, but theschemdiff
textual diff will show no changes. Likewise, based on diff hints, constraint names can be ignored, etc.It also takes care of trailing last comma issues, so common with e.g.
git diff
. For example, in the below:A standard textual diff will capture the
PRIMARY KEY
line, due to the addition of the trailing comma.schemadiff
, however, outputs:CREATE TABLE `t4` ( `id` int, PRIMARY KEY (`id`), + KEY `another_idx` (`id`) )
Implementation-wise, a
DiffEntity
now has anAnnotated()
function, which returns 3 annotated diff texts:1+2: The split from & to annotated forms.
3: The unified form (as shown in the above example).
Related Issue(s)
Fixes #15387
Checklist
Deployment Notes