Refactor schemadiff to inject environment #14934

dbussink · 2024-01-11T10:49:36Z

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.

Also includes the changes in #14930

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

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>

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

vitess-bot · 2024-01-11T10:49:38Z

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>

codecov · 2024-01-11T13:20:52Z

Codecov Report

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

Comparison is base (9df71e0) 47.26% compared to head (5241c23) 47.30%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
go/vt/sidecardb/sidecardb.go	64.28%	8 Missing and 2 partials ⚠️
go/vt/schemadiff/table.go	86.48%	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 ⚠️
go/vt/vttest/local_cluster.go	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14934      +/-   ##
==========================================
+ Coverage   47.26%   47.30%   +0.04%     
==========================================
  Files        1136     1137       +1     
  Lines      238484   238647     +163     
==========================================
+ Hits       112721   112898     +177     
+ Misses     117158   117134      -24     
- Partials     8605     8615      +10

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

shlomi-noach

Good one! Looks like this formalizes a lot of code and brings some order.

Re: including #14930, I just thought of something I missed there. I'm gonna add a commit to #14930 which you can then cherry pick (or I can push to your repo).

shlomi-noach · 2024-01-11T13:16:01Z

go/vt/schemadiff/env.go

shlomi-noach · 2024-01-11T13:16:21Z

go/vt/schemadiff/schema.go

shlomi-noach · 2024-01-11T13:16:46Z

go/vt/schemadiff/column.go

👍, copied from #14930

shlomi-noach · 2024-01-11T13:18:22Z

go/vt/schemadiff/env.go

+		DefaultColl:  defaultColl,
+		Parser:       parser,
+	}
+}


I predict it might be useful: let's add a NewMySQL8Env() function that uses utf8mb4 and possibly also creates a MySQL8 compliant parser.

@shlomi-noach I'm hesitant actually about that and I like that this is explicit and the caller is required to pass these in. We also explicitly name the default one as a test specific one.

The main reason for this is also slightly less collations, but also the parser since the parser has MySQL specific version behavior, so you almost always want to pass in a specific version parser from the context you're in.

So I very much prefer not to add this and see that if we seem to have a need, to look at that and do the proper thing at that point.

shlomi-noach · 2024-01-11T13:18:40Z

go/vt/schemadiff/diff.go

shlomi-noach · 2024-01-11T13:26:36Z

go/vt/sidecardb/sidecardb.go

-	showCreateTableQuery = "show create table %s.%s"
+	sidecarDBExistsQuery  = "select 'true' as 'dbexists' from information_schema.SCHEMATA where SCHEMA_NAME = %a"
+	showCreateTableQuery  = "show create table %s.%s"
+	sidecarCollationQuery = "select @@global.collation_server"


shlomi-noach · 2024-01-11T13:27:15Z

go/vt/sidecardb/sidecardb.go

+		return si.collEnv.LookupByName(rs.Rows[0][0].ToString()), nil
+	default:
+		// This should never happen.
+		return collations.Unknown, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "invalid results for SidecarDB query %q as it produced %d rows", sidecarCollationQuery, len(rs.Rows))


shlomi-noach · 2024-01-11T13:29:07Z

go/vt/sidecardb/sidecardb.go

+		return collations.Unknown, err
+	}
+
+	switch len(rs.Rows) {


I know this is a copy+paste from elsewhere, because I recognize the pattern. I think at some later stage we should cleanup this kind of code. A select @@global.collation_server will either fail, or return exactly one row. There is no need to check in the code what happens if less or more different rows are returned. It's only confusing and adding error prone logic.

@shlomi-noach It actually helped me here since with the fake db I had to add stuff since things were missing since no mock queries were set up. So it help uncover that in an easier way than random panics.

shlomi-noach · 2024-01-11T13:29:27Z

go/vt/sidecardb/sidecardb.go

@@ -375,7 +400,8 @@ func (si *schemaInit) findTableSchemaDiff(tableName, current, desired string) (s
 		TableCharsetCollateStrategy: schemadiff.TableCharsetCollateIgnoreAlways,
 		AlterTableAlgorithmStrategy: schemadiff.AlterTableAlgorithmStrategyCopy,
 	}
-	diff, err := schemadiff.DiffCreateTablesQueries(current, desired, hints, si.parser)
+	env := schemadiff.NewEnv(si.collEnv, si.coll, si.parser)


shlomi-noach · 2024-01-11T13:31:04Z

go/vt/sidecardb/sidecardb_test.go

Should we also add a 5.7-collation test? Will it be beneficial?

@shlomi-noach schemadiff has one for 5.7 and implicit the sidecar is already tested on 5.7 (see the failures in #14930), so I think we're covered here?

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

This PR now merged into #14930

shlomi-noach · 2024-01-11T16:20:42Z

Converted to Draft since we now have #14930 as the more updated version.

shlomi-noach added 9 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>

testing change of charset to default

52fd91c

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

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>

Remove unused function argument

261c54a

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

dbussink added Type: Bug Type: Internal Cleanup Component: Query Serving labels Jan 11, 2024

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

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

dbussink removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels Jan 11, 2024

dbussink removed the NeedsBackportReason If backport labels have been applied to a PR, a justification is required label Jan 11, 2024

dbussink force-pushed the schemadiff-charset-columns-table branch from 1886f57 to df1eb4e Compare January 11, 2024 12:27

dbussink force-pushed the schemadiff-charset-columns-table branch from df1eb4e to 5241c23 Compare January 11, 2024 13:08

shlomi-noach approved these changes Jan 11, 2024

View reviewed changes

shlomi-noach mentioned this pull request Jan 11, 2024

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

Merged

5 tasks

shlomi-noach marked this pull request as draft January 11, 2024 16:20

dbussink closed this Jan 11, 2024

dbussink deleted the schemadiff-charset-columns-table branch January 11, 2024 16:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor schemadiff to inject environment #14934

Refactor schemadiff to inject environment #14934

dbussink commented Jan 11, 2024

vitess-bot bot commented Jan 11, 2024

codecov bot commented Jan 11, 2024

shlomi-noach left a comment

shlomi-noach Jan 11, 2024

shlomi-noach Jan 11, 2024

shlomi-noach Jan 11, 2024

shlomi-noach Jan 11, 2024

dbussink Jan 11, 2024

shlomi-noach Jan 11, 2024

shlomi-noach Jan 11, 2024

shlomi-noach Jan 11, 2024

shlomi-noach Jan 11, 2024

dbussink Jan 11, 2024

shlomi-noach Jan 11, 2024

shlomi-noach Jan 11, 2024

dbussink Jan 11, 2024

shlomi-noach commented Jan 11, 2024

shlomi-noach commented Jan 11, 2024

Refactor schemadiff to inject environment #14934

Refactor schemadiff to inject environment #14934

Conversation

dbussink commented Jan 11, 2024

Related Issue(s)

Checklist

vitess-bot bot commented Jan 11, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jan 11, 2024

Codecov Report

shlomi-noach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach commented Jan 11, 2024

shlomi-noach commented Jan 11, 2024