-
Notifications
You must be signed in to change notification settings - Fork 198
fix(mysql): align JSON representation between snapshot and CDC #4535
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 all commits
9d314a7
060e561
4edd4f6
4fa783d
564fb88
7e9c5f8
fdfc0bd
1c4dad8
b7ded2f
99423a1
0dc24d2
120cb48
87a9e06
6f4aee2
eedb55c
fc8d9bc
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 |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| package connmysql | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/binary" | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "log/slog" | ||
|
|
@@ -28,6 +30,14 @@ import ( | |
| "github.com/PeerDB-io/peerdb/flow/shared/types" | ||
| ) | ||
|
|
||
| func compactMySQLJSON(v []byte) string { | ||
| var buf bytes.Buffer | ||
| if err := json.Compact(&buf, v); err != nil { | ||
| return string(v) | ||
| } | ||
| return buf.String() | ||
| } | ||
|
|
||
| func qkindFromMysqlType(mytype byte, unsigned bool, charset uint16, version uint32) (types.QValueKind, error) { | ||
| switch mytype { | ||
| case mysql.MYSQL_TYPE_TINY: | ||
|
|
@@ -332,7 +342,8 @@ func QValueFromMysqlFieldValue(qkind types.QValueKind, mytype byte, fv mysql.Fie | |
| case types.QValueKindBytes: | ||
| return types.QValueBytes{Val: slices.Clone(v)}, nil | ||
| case types.QValueKindJSON: | ||
| return types.QValueJSON{Val: string(v)}, nil | ||
| // keep snapshot and cdc json representation consistent | ||
| return types.QValueJSON{Val: compactMySQLJSON(v)}, nil | ||
|
Collaborator
Author
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. same here, could gate behind internal mirror version to avoid inconsistency |
||
| case types.QValueKindUUID: | ||
| // snapshot reads via the text protocol, so MariaDB sends the canonical string | ||
| u, err := uuid.Parse(unsafeString) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -277,6 +277,164 @@ func (s ClickHouseSuite) Test_MySQL_Blobs() { | |
| RequireEnvCanceled(s.t, env) | ||
| } | ||
|
|
||
| func (s ClickHouseSuite) Test_MySQL_JSON_SnapshotCDCConsistency() { | ||
| mySource, ok := s.source.(*MySqlSource) | ||
| if !ok { | ||
| s.t.Skip("only applies to mysql") | ||
| } | ||
| // MySQL stores JSON in a native binary form and re-serializes it to a compact, | ||
| // whitespace-stripped text; MariaDB's JSON type is plain LONGTEXT stored verbatim, so | ||
| // the exact-representation checks below only hold on MySQL. | ||
| nativeJSON := mySource.Config.Flavor == protos.MySqlFlavor_MYSQL_MYSQL | ||
|
|
||
| srcTableName := "test_json_repr" | ||
| srcFullName := s.attachSchemaSuffix(srcTableName) | ||
| dstTableName := "test_json_repr_dst" | ||
|
|
||
| require.NoError(s.t, s.source.Exec(s.t.Context(), fmt.Sprintf(` | ||
| CREATE TABLE IF NOT EXISTS %s ( | ||
| id serial PRIMARY KEY, | ||
| js json NOT NULL | ||
| )`, srcFullName))) | ||
|
|
||
| variants := []struct { | ||
| val string | ||
| expectExact string | ||
| }{ | ||
| // key order: shorter key sorts after longer one | ||
| {val: `{"z": 1, "aa": 2}`}, | ||
| {val: `{"b": 2, "a": 1}`}, // key order: reversed on storage | ||
| // nested object key order + array order preserved | ||
| {val: `{"n": [3, 1, 2], "obj": {"y": 1, "x": 2}}`}, | ||
| {val: `{"s": "a, b: c"}`}, // whitespace inside strings is preserved | ||
| // non-whole doubles that render identically in both paths | ||
| {val: `{"pi": 3.14, "half": 0.5, "neg": -2.5}`}, | ||
| {val: `{"big": 1234567890123456789}`, expectExact: `{"big":1234567890123456789}`}, // 19-digit int64: float64 rounds to ...6800 | ||
| {val: `{"maxsafe": 9007199254740993}`, expectExact: `{"maxsafe":9007199254740993}`}, // 2^53+1: float64 drops the low bit (...992) | ||
| {val: `{"negbig": -1234567890123456789}`, expectExact: `{"negbig":-1234567890123456789}`}, // sign preserved at full precision | ||
| { | ||
| val: `{"mix": [1234567890123456789, -9007199254740993, 9007199254740993]}`, // same hazards inside an array | ||
| expectExact: `{"mix":[1234567890123456789,-9007199254740993,9007199254740993]}`, | ||
| }, | ||
| } | ||
|
|
||
| insertVariants := func() { | ||
| for _, v := range variants { | ||
| require.NoError(s.t, s.source.Exec(s.t.Context(), | ||
| fmt.Sprintf(`INSERT INTO %s (js) VALUES ('%s')`, srcFullName, v.val))) | ||
| } | ||
| } | ||
|
|
||
| insertVariants() | ||
|
|
||
| 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) | ||
|
|
||
| EnvWaitForCount(env, s, "waiting for initial snapshot", dstTableName, "id", len(variants)) | ||
|
|
||
| insertVariants() | ||
|
|
||
| EnvWaitForCount(env, s, "waiting for cdc", dstTableName, "id", 2*len(variants)) | ||
|
|
||
| // GetRows orders by id, so snapshot rows (ids 1..N) come first, then CDC rows (N+1..2N); | ||
| // both batches hold the same values in the same order, so row i and row i+N are the same | ||
| // logical value read via the snapshot and CDC paths respectively. | ||
| rows, err := s.GetRows(dstTableName, "id, js") | ||
| require.NoError(s.t, err) | ||
| require.Len(s.t, rows.Records, 2*len(variants)) | ||
|
|
||
| for i := range variants { | ||
| snapshotGot := fmt.Sprint(rows.Records[i][1].Value()) | ||
| cdcGot := fmt.Sprint(rows.Records[i+len(variants)][1].Value()) | ||
| require.Equal(s.t, snapshotGot, cdcGot, "snapshot/cdc JSON representation differs for %q", variants[i].val) | ||
| if nativeJSON && variants[i].expectExact != "" { | ||
| require.Equal(s.t, variants[i].expectExact, snapshotGot, "snapshot JSON not preserved faithfully for %q", variants[i].val) | ||
| require.Equal(s.t, variants[i].expectExact, cdcGot, "cdc JSON not preserved faithfully for %q", variants[i].val) | ||
| } | ||
| } | ||
|
|
||
| env.Cancel(s.t.Context()) | ||
| RequireEnvCanceled(s.t, env) | ||
| } | ||
|
|
||
| // Test_MySQL_JSON_SnapshotCDCConsistency_NativeJSON is the native-JSON companion to | ||
| // Test_MySQL_JSON_SnapshotCDCConsistency. | ||
| func (s ClickHouseSuite) Test_MySQL_JSON_SnapshotCDCConsistency_NativeJSON() { | ||
|
Collaborator
Author
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. Without PEERDB_CLICKHOUSE_ENABLE_JSON: true (i.e. JSON stored as String), snapshot and CDC can still disagree textually for doubles in magnitude ranges where the two paths format differently. The snapshot path uses MySQL's my_gcvt, which stays in plain decimal (e.g. 1000000.5), while the CDC path uses go-mysql, which switches to scientific notation (1.0000005e+06). The float64 value is identical — only the text differs — so it's harmless for native-JSON destinations (ClickHouse normalizes on parse) but visible for String-typed columns.
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. today we technically don't support |
||
| if _, ok := s.source.(*MySqlSource); !ok { | ||
| s.t.Skip("only applies to mysql") | ||
| } | ||
|
|
||
| srcTableName := "test_json_repr_native" | ||
| srcFullName := s.attachSchemaSuffix(srcTableName) | ||
| dstTableName := "test_json_repr_native_dst" | ||
|
|
||
| require.NoError(s.t, s.source.Exec(s.t.Context(), fmt.Sprintf(` | ||
| CREATE TABLE IF NOT EXISTS %s ( | ||
| id serial PRIMARY KEY, | ||
| js json NOT NULL | ||
| )`, srcFullName))) | ||
|
|
||
| // These deliberately include the doubles that diverge in text form between the snapshot | ||
| // and CDC paths; native JSON normalization is what makes them consistent at the destination. | ||
| variants := []string{ | ||
| `{"pi": 3.14, "big": 1000000.5, "neg": -2.5}`, // magnitude where snapshot/CDC text forms differ | ||
| `{"tiny": 0.000015, "huge": 150000000000.5}`, // more exponent-crossover magnitudes | ||
| `{"z": 1.0, "aa": 2}`, // key order, normalized consistently | ||
| } | ||
|
|
||
| insertVariants := func() { | ||
| for _, v := range variants { | ||
| require.NoError(s.t, s.source.Exec(s.t.Context(), | ||
| fmt.Sprintf(`INSERT INTO %s (js) VALUES ('%s')`, srcFullName, v))) | ||
| } | ||
| } | ||
|
|
||
| insertVariants() | ||
|
|
||
| connectionGen := FlowConnectionGenerationConfig{ | ||
| FlowJobName: s.attachSuffix(srcTableName), | ||
| TableNameMapping: map[string]string{srcFullName: dstTableName}, | ||
| Destination: s.Peer().Name, | ||
| } | ||
| flowConnConfig := connectionGen.GenerateFlowConnectionConfigs(s) | ||
| flowConnConfig.DoInitialSnapshot = true | ||
| flowConnConfig.Env = map[string]string{"PEERDB_CLICKHOUSE_ENABLE_JSON": "true"} | ||
|
|
||
| tc := NewTemporalClient(s.t) | ||
| env := ExecutePeerflow(s.t, tc, flowConnConfig) | ||
| SetupCDCFlowStatusQuery(s.t, env, flowConnConfig) | ||
|
|
||
| EnvWaitForCount(env, s, "waiting for initial snapshot", dstTableName, "id", len(variants)) | ||
|
|
||
| insertVariants() | ||
|
|
||
| EnvWaitForCount(env, s, "waiting for cdc", dstTableName, "id", 2*len(variants)) | ||
|
|
||
| // Read ClickHouse's own normalized text of the JSON column so the comparison is over a | ||
| // plain String scan target (GetRows does not scan the native JSON type directly). | ||
| rows, err := s.GetRows(dstTableName, "id, toString(js)") | ||
| require.NoError(s.t, err) | ||
| require.Len(s.t, rows.Records, 2*len(variants)) | ||
|
|
||
| for i := range variants { | ||
| snapshotGot := fmt.Sprint(rows.Records[i][1].Value()) | ||
| cdcGot := fmt.Sprint(rows.Records[i+len(variants)][1].Value()) | ||
| require.Equal(s.t, snapshotGot, cdcGot, "snapshot/cdc JSON representation differs for %q", variants[i]) | ||
| } | ||
|
|
||
| env.Cancel(s.t.Context()) | ||
| RequireEnvCanceled(s.t, env) | ||
| } | ||
|
|
||
| func (s ClickHouseSuite) Test_MySQL_TransactionPayloadCompression() { | ||
| mySource, ok := s.source.(*MySqlSource) | ||
| if !ok { | ||
|
|
||
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 one was added (really weird git diff because of go fmt)
we can also put it behind mirror version gate to ensure that the old mirrors continue working the same way (in case someone relies on default parsing logic)
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.
link to documentation for this flag if you want to read it:
https://github.com/go-mysql-org/go-mysql/blob/a57884f25cf0008867559df9012a6498bb17df70/replication/binlogsyncer.go#L111
the main reason we need it is to ensure that DOUBLEs are parsed properly
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 a reasonably safe change to introduce it without putting it behind a flag, given this was an inconsistency between initial snapshot and CDC.