`schemadiff`: fix diffing of textual columns with implicit charsets #14930

shlomi-noach · 2024-01-10T13:06:41Z

Description

See story in #14929. Also, see test cases added in diff_test.go to understand the nuances of TableCharsetCollateStrategy.

In this PR:

Main change is in column.go, ColumnDefinitionEntity.ColumnDiff() investigates not only the from/to columns, but also the from/to tables' charset/collation, and temporarily populates column with any missing data so as to have a true comparison.
To that effect, a new helper charsetCollate struct was introduced, and used throughout table.go as the diff result of two tables' charset/collations.

Update: merged another PR

In a game of pass-the-ball, this PR now includes #14934 (which originally started off this PR). Please see description on #14934

Related Issue(s)

Fixes #14929

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…eIgnoreAlways behavior Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2024-01-10T13:06:44Z

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

codecov · 2024-01-10T13:24:22Z

Codecov Report

Attention: 55 lines in your changes are missing coverage. Please review.

Comparison is base (9df71e0) 47.26% compared to head (26acffc) 47.31%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
go/vt/schemadiff/column.go	59.64%	20 Missing and 3 partials ⚠️
go/vt/sidecardb/sidecardb.go	64.28%	8 Missing and 2 partials ⚠️
go/vt/schemadiff/table.go	88.63%	4 Missing and 1 partial ⚠️
go/vt/schemadiff/diff.go	87.50%	3 Missing ⚠️
go/vt/vtctl/grpcvtctldserver/server.go	40.00%	3 Missing ⚠️
go/vt/vttablet/onlineddl/executor.go	0.00%	3 Missing ⚠️
go/vt/vtctl/workflow/materializer.go	0.00%	2 Missing ⚠️
go/vt/wrangler/materializer.go	33.33%	2 Missing ⚠️
go/vt/vttablet/onlineddl/vrepl.go	0.00%	1 Missing ⚠️
go/vt/vttablet/tabletserver/schema/engine.go	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14930      +/-   ##
==========================================
+ Coverage   47.26%   47.31%   +0.04%     
==========================================
  Files        1136     1137       +1     
  Lines      238484   238684     +200     
==========================================
+ Hits       112721   112923     +202     
+ Misses     117158   117152       -6     
- Partials     8605     8609       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbussink · 2024-01-10T13:26:30Z

go/vt/schemadiff/column.go

+	if c.IsTextual() || other.IsTextual() {
+		switch hints.TableCharsetCollateStrategy {
+		case TableCharsetCollateStrict:
+			// No need to do anything


I don't think this is correct. Also in this case we can get an empty / column charset due to normalization if you change the table level charset and we shouldn't then generate a diff.

This is actually visible in a broken test that was added.

dbussink · 2024-01-10T13:27:40Z

go/vt/schemadiff/diff_test.go

+			from:     "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin) default charset=latin1",
+			to:       "create table t (a varchar(64) CHARACTER SET latin1 COLLATE latin1_bin)",
+			diff:     "alter table t modify column a varchar(64) character set latin1 collate latin1_bin, charset utf8mb4, algorithm = COPY",
+			cdiff:    "ALTER TABLE `t` MODIFY COLUMN `a` varchar(64) CHARACTER SET latin1 COLLATE latin1_bin, CHARSET utf8mb4, ALGORITHM = COPY",


This test expectation is wrong. In this case there should not be a MODIFY COLUMN, since the charset / collation of the column doesn't actually change. This should only be alter table t charset utf8mb4 instead.

Agreed. Looking into it.

I've fixed the expected output, so that now the PR should be failing until the logic is adapted.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2024-01-10T13:57:59Z

@dbussink please review the new and improved tests in:

The tests now all pass. If you agree that the tests are correct, then we're good.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

We already had the parser that needed to be injected, but in order to properly handle MySQL 5.7 we also need to inject collations configuration. Instead of adding more arguments, we introduce the schemadiff environment struct and use that to inject the things like the current parser and current collation environment and default collation. Also need to refactor the rest that ends up using schemadiff to inject it similarly. The core goal here is that the sidecar can properly inject the right collation as well so we can unblock fixing the case sensitivity there. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

…hout a charset, we use Envronment to autopopulate the charset, or return an error what that is not possible Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2024-01-11T16:19:07Z

Update: merged another PR

In a game of pass-the-ball, this PR now includes #14934 (which originally started off this PR). Please see description on #14934

GuptaManan100

LGTM! Thankyou for fixing this!

…olumns with implicit charsets Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added 5 commits January 10, 2024 12:01

testing: more details upon failure

0db6904

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

extract getTableCharsetAndCollation() function

d6f5b6d

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

minor rewrite

c26dea1

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

use TableOptions

25a070b

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

schemadiff: fix TableCharsetCollateIgnoreEmpty and TableCharsetCollat…

f2010a3

…eIgnoreAlways behavior Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Bug Component: Query Serving labels Jan 10, 2024

shlomi-noach requested review from dbussink and a team January 10, 2024 13:06

shlomi-noach requested a review from deepthi as a code owner January 10, 2024 13:06

github-actions bot added this to the v19.0.0 milestone Jan 10, 2024

testing change of charset to default

52fd91c

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dbussink reviewed Jan 10, 2024

View reviewed changes

shlomi-noach added 2 commits January 10, 2024 15:36

fix expected output

029ddef

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

fixed logic, added multiple tests, now also covering collation changes

6d88ce2

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach changed the title ~~scheadiff: handle TableCharsetCollateIgnoreAlways and TableCharsetCollateIgnoreEmpty correctly~~ scheadiff: fix diffing of textual columns with implicit charsets Jan 10, 2024

Remove unused function argument

261c54a

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

dbussink mentioned this pull request Jan 11, 2024

Refactor schemadiff to inject environment #14934

Closed

5 tasks

shlomi-noach added 2 commits January 11, 2024 17:56

ColumnDiff takes Environment. When a column specifies a collation wit…

5784dc3

…hout a charset, we use Envronment to autopopulate the charset, or return an error what that is not possible Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

ColumnDiff takes Environment. When a column specifies a collation wit…

26acffc

…hout a charset, we use Envronment to autopopulate the charset, or return an error what that is not possible Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach force-pushed the schemadiff-charset-columns-table branch from ad0fe03 to 26acffc Compare January 11, 2024 15:56

shlomi-noach requested review from mattlord, rohit-nayak-ps, harshit-gangal, systay, GuptaManan100, ajm188 and notfelineit as code owners January 11, 2024 15:56

dbussink approved these changes Jan 11, 2024

View reviewed changes

dbussink changed the title ~~scheadiff: fix diffing of textual columns with implicit charsets~~ schemadiff: fix diffing of textual columns with implicit charsets Jan 11, 2024

GuptaManan100 approved these changes Jan 12, 2024

View reviewed changes

GuptaManan100 merged commit eddb39e into vitessio:main Jan 12, 2024
102 checks passed

GuptaManan100 deleted the schemadiff-charset-columns-table branch January 12, 2024 06:18

GuptaManan100 mentioned this pull request Jan 12, 2024

Make Schema Tracking case-sensitive #14904

Merged

5 tasks

dbussink mentioned this pull request Jan 12, 2024

Refactor out more usage of servenv for mysql version #14938

Merged

5 tasks

shlomi-noach added a commit to planetscale/vitess that referenced this pull request May 8, 2024

manual import of vitessio#14930: schemadiff: fix diffing of textual c…

1928dbe

…olumns with implicit charsets Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach mentioned this pull request May 8, 2024

v18 backport: schemadiff: fix diffing of textual columns with implicit charsets #15873

Closed

5 tasks

GuptaManan100 mentioned this pull request Oct 24, 2024

[Direct PR] [release-18.0] Remove sql changes from schemacopy #17050

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`schemadiff`: fix diffing of textual columns with implicit charsets #14930

`schemadiff`: fix diffing of textual columns with implicit charsets #14930

shlomi-noach commented Jan 10, 2024 •

edited

Loading

vitess-bot bot commented Jan 10, 2024

codecov bot commented Jan 10, 2024 •

edited

Loading

dbussink Jan 10, 2024

dbussink Jan 10, 2024

shlomi-noach Jan 10, 2024

shlomi-noach Jan 10, 2024

shlomi-noach commented Jan 10, 2024

shlomi-noach commented Jan 11, 2024

GuptaManan100 left a comment

schemadiff: fix diffing of textual columns with implicit charsets #14930

schemadiff: fix diffing of textual columns with implicit charsets #14930

Conversation

shlomi-noach commented Jan 10, 2024 • edited Loading

Description

Update: merged another PR

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 10, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

dbussink Jan 10, 2024

Choose a reason for hiding this comment

dbussink Jan 10, 2024

Choose a reason for hiding this comment

shlomi-noach Jan 10, 2024

Choose a reason for hiding this comment

shlomi-noach Jan 10, 2024

Choose a reason for hiding this comment

shlomi-noach commented Jan 10, 2024

shlomi-noach commented Jan 11, 2024

GuptaManan100 left a comment

Choose a reason for hiding this comment

`schemadiff`: fix diffing of textual columns with implicit charsets #14930

`schemadiff`: fix diffing of textual columns with implicit charsets #14930

shlomi-noach commented Jan 10, 2024 •

edited

Loading

codecov bot commented Jan 10, 2024 •

edited

Loading