-
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 1 commit
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,70 @@ | ||
| package connmysql | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/PeerDB-io/peerdb/flow/generated/protos" | ||
| "github.com/PeerDB-io/peerdb/flow/shared/types" | ||
| ) | ||
|
|
||
| func TestBuildSelectedColumns(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| cols []*protos.FieldDescription | ||
| exclude []string | ||
| isBinlogMetadataSupported bool | ||
| expectedSelectedColumns string | ||
| }{ | ||
| { | ||
| 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, | ||
| 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, | ||
| expectedSelectedColumns: "`id`", | ||
| }, | ||
| { | ||
| name: "one enum column, binlog metadata not supported", | ||
| cols: []*protos.FieldDescription{ | ||
| {Name: "id", Type: string(types.QValueKindInt32)}, | ||
| {Name: "status", Type: string(types.QValueKindEnum)}, | ||
| }, | ||
| exclude: []string{}, | ||
| isBinlogMetadataSupported: false, | ||
| expectedSelectedColumns: "`id`, `status` + 0", | ||
| }, | ||
| { | ||
| 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, | ||
| expectedSelectedColumns: "`id`, `status`", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| selectedColumns := buildSelectedColumns(tc.cols, tc.exclude, tc.isBinlogMetadataSupported) | ||
| 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 | ||
|---|---|---|---|---|
|
|
@@ -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 *)