-
Notifications
You must be signed in to change notification settings - Fork 198
fix: cast enums to integers during qrep for mysql without binlog metadata … #4057
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
Changes from 5 commits
3447599
d2e3382
bbd884b
4e5c07c
ba10c5d
3186e7f
00ca9d7
647af68
257cc9a
eac0f39
364a75a
6aedfbf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| package connmysql | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/PeerDB-io/peerdb/flow/generated/protos" | ||
| "github.com/PeerDB-io/peerdb/flow/shared" | ||
| "github.com/PeerDB-io/peerdb/flow/shared/types" | ||
| ) | ||
|
|
||
| func TestBuildSelectedColumns(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| expectedSelectedColumns string | ||
| cols []*protos.FieldDescription | ||
| exclude []string | ||
| isBinlogMetadataSupported bool | ||
| mirrorVersion uint32 | ||
| }{ | ||
| { | ||
| name: "no excluded columns, binlog metadata supported", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "name", Type: string(types.QValueKindString)}, | ||
| {Name: "status", Type: string(types.QValueKindEnum)}, | ||
| }, | ||
| exclude: []string{}, | ||
| isBinlogMetadataSupported: true, | ||
| mirrorVersion: shared.InternalVersion_MySQLConvertEnumsToInts, | ||
| expectedSelectedColumns: "*", | ||
| }, | ||
| { | ||
| name: "one excluded column, binlog metadata supported", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "name", Type: string(types.QValueKindString)}, | ||
| }, | ||
| exclude: []string{"name"}, | ||
| isBinlogMetadataSupported: true, | ||
| mirrorVersion: shared.InternalVersion_MySQLConvertEnumsToInts, | ||
| expectedSelectedColumns: "`id`", | ||
| }, | ||
| { | ||
| name: "one enum column, binlog metadata not supported, new version", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "status", Type: string(types.QValueKindEnum)}, | ||
| }, | ||
| exclude: []string{}, | ||
| isBinlogMetadataSupported: false, | ||
| mirrorVersion: shared.InternalVersion_MySQLConvertEnumsToInts, | ||
| expectedSelectedColumns: "`id`, CAST(`status` AS UNSIGNED) AS `status`", | ||
| }, | ||
| { | ||
| name: "one enum column, binlog metadata not supported, old version", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "status", Type: string(types.QValueKindEnum)}, | ||
| }, | ||
| exclude: []string{}, | ||
| isBinlogMetadataSupported: false, | ||
| mirrorVersion: shared.InternalVersion_MySQLConvertEnumsToInts - 1, | ||
| expectedSelectedColumns: "*", | ||
| }, | ||
| { | ||
| name: "one enum column, binlog metadata not supported, version zero", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "status", Type: string(types.QValueKindEnum)}, | ||
| }, | ||
| exclude: []string{}, | ||
| isBinlogMetadataSupported: false, | ||
| mirrorVersion: 0, | ||
| expectedSelectedColumns: "*", | ||
| }, | ||
| { | ||
| name: "one enum column, one excluded non enum column, binlog metadata supported", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "status", Type: string(types.QValueKindEnum)}, | ||
| {Name: "created_at", Type: string(types.QValueKindTimestamp)}, | ||
| }, | ||
| exclude: []string{"created_at"}, | ||
| isBinlogMetadataSupported: true, | ||
| mirrorVersion: shared.InternalVersion_MySQLConvertEnumsToInts, | ||
| expectedSelectedColumns: "`id`, `status`", | ||
| }, | ||
| { | ||
| name: "enum with exclude, binlog metadata not supported, old version", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "status", Type: string(types.QValueKindEnum)}, | ||
| {Name: "created_at", Type: string(types.QValueKindTimestamp)}, | ||
| }, | ||
| exclude: []string{"created_at"}, | ||
| isBinlogMetadataSupported: false, | ||
| mirrorVersion: shared.InternalVersion_MySQLConvertEnumsToInts - 1, | ||
| expectedSelectedColumns: "`id`, `status`", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| selectedColumns := buildSelectedColumns(tc.cols, tc.exclude, tc.isBinlogMetadataSupported, tc.mirrorVersion) | ||
| if selectedColumns != tc.expectedSelectedColumns { | ||
| t.Errorf("expected selected columns to be %s, but got %s", tc.expectedSelectedColumns, selectedColumns) | ||
| } | ||
| }) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,6 +229,8 @@ func QValueFromMysqlFieldValue(qkind types.QValueKind, mytype byte, fv mysql.Fie | |
| case mysql.FieldValueTypeUnsigned: | ||
| v := fv.AsUint64() | ||
| switch qkind { | ||
| case types.QValueKindEnum: | ||
| return types.QValueEnum{Val: strconv.FormatUint(v, 10)}, nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now the values would be converted to strings and we're really making them integers on purpose. Let's introduce a QValueKindUint16Enum and return it from QkindFromMysqlColumnType in type_conversion.go, propagating all the necessary information there. If something's confusing, I gave it a shot on uint16enum branch, that should be complete (but you don't have to look there :) |
||
| case types.QValueKindBoolean: | ||
| return types.QValueBoolean{Val: v != 0}, nil | ||
| case types.QValueKindInt8: | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -274,6 +274,59 @@ func (s ClickHouseSuite) Test_MySQL_Enum() { | |||
| RequireEnvCanceled(s.t, env) | ||||
| } | ||||
|
|
||||
| func (s ClickHouseSuite) Test_MySQL_Enum_Consistency() { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a test for the previous version as well (existing mirrors/pipes). Example: peerdb/flow/e2e/clickhouse_test.go Line 1804 in 257cc9a
Test itself would be basically the same except for asserting the |
||||
| if _, ok := s.source.(*MySqlSource); !ok { | ||||
| s.t.Skip("only applies to mysql") | ||||
| } | ||||
|
|
||||
| srcTableName := "test_my_enum_consistency" | ||||
| srcFullName := s.attachSchemaSuffix(srcTableName) | ||||
| dstTableName := "test_my_enum_consistency_dst" | ||||
|
|
||||
| require.NoError(s.t, s.source.Exec(s.t.Context(), fmt.Sprintf(` | ||||
| CREATE TABLE IF NOT EXISTS %s ( | ||||
| id SERIAL PRIMARY KEY, | ||||
| status ENUM('active', 'inactive', 'pending') NOT NULL | ||||
| ) | ||||
| `, srcFullName))) | ||||
|
|
||||
| // Insert row before snapshot | ||||
| require.NoError(s.t, s.source.Exec(s.t.Context(), fmt.Sprintf( | ||||
| `INSERT INTO %s (status) VALUES ('active')`, srcFullName))) | ||||
|
|
||||
| connectionGen := FlowConnectionGenerationConfig{ | ||||
| FlowJobName: s.attachSuffix(srcTableName), | ||||
| TableNameMapping: map[string]string{srcFullName: dstTableName}, | ||||
| Destination: s.Peer().Name, | ||||
| } | ||||
| flowConnConfig := connectionGen.GenerateFlowConnectionConfigs(s) | ||||
| flowConnConfig.DoInitialSnapshot = true | ||||
|
|
||||
| tc := NewTemporalClient(s.t) | ||||
| env := ExecutePeerflow(s.t, tc, flowConnConfig) | ||||
| SetupCDCFlowStatusQuery(s.t, env, flowConnConfig) | ||||
|
|
||||
| // Wait for snapshot row to appear in destination | ||||
| EnvWaitForCount(env, s, "waiting on snapshot", dstTableName, "id,status", 1) | ||||
|
|
||||
| // Insert row via CDC — on old MySQL this comes as integer from binlog | ||||
| require.NoError(s.t, s.source.Exec(s.t.Context(), fmt.Sprintf( | ||||
| `INSERT INTO %s (status) VALUES ('active')`, srcFullName))) | ||||
|
|
||||
| // Wait for CDC row | ||||
| EnvWaitForCount(env, s, "waiting on cdc", dstTableName, "id,status", 2) | ||||
|
|
||||
| // Verify both rows have the same status value (consistency between snapshot and CDC) | ||||
| rows, err := s.GetRows(dstTableName, "status") | ||||
| require.NoError(s.t, err) | ||||
| require.Len(s.t, rows.Records, 2) | ||||
| require.Equal(s.t, rows.Records[0][0].Value(), rows.Records[1][0].Value(), | ||||
| "snapshot and CDC enum values should be consistent") | ||||
|
dtunikov marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an assert for the actual value. Can do if os.Getenv("CI_MYSQL_VERSION") == "mysql-pos" {
require.Equal(s.t, 1, rows.Records[0][0].Value())
} else {
require.Equal(s.t, 'active', rows.Records[0][0].Value())
} |
||||
|
|
||||
| env.Cancel(s.t.Context()) | ||||
| RequireEnvCanceled(s.t, env) | ||||
| } | ||||
|
|
||||
| func (s ClickHouseSuite) Test_MySQL_Vector() { | ||||
| mysource, ok := s.source.(*MySqlSource) | ||||
| if !ok || mysource.Config.Flavor != protos.MySqlFlavor_MYSQL_MYSQL { | ||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.
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.
This is not really necessary (we could always query columns explicitly).
But I decided that this way it will be consistent with the current behaviour (no transformations -> select *)