fix(mysql): align JSON representation between snapshot and CDC#4535
Conversation
MySQL JSON columns reached PeerDB through two different renderers that disagreed on representation: - Snapshot (text protocol): MySQL server-rendered JSON, type-faithful (DOUBLE 1.0 stays "1.0", JSONB key order preserved) but with a space after ':' and ','. - CDC (binlog JSONB decoder): historically lossy -- 1.0 collapsed to 1, keys reordered by Go map iteration. Enable RenderJSONAsMySQLText on the binlog syncer so CDC emits type-faithful, key-order-preserving, compact JSON, and json.Compact the snapshot text form so both paths are byte-consistent. MariaDB JSON is LONGTEXT-backed (QValueKindString), so it never hits the compaction path and is unaffected. Adds a unit test for the compaction helper and an e2e test asserting the snapshot and CDC representations of the same JSON value match. DBI-823 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| DisableRetrySync: true, | ||
| UseDecimal: true, | ||
| ParseTime: true, | ||
| RenderJSONAsMySQLText: true, |
There was a problem hiding this comment.
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.
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.
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.
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ Test FailureAnalysis: The PR's own new test Test_MySQL_JSON_SnapshotCDCConsistency deterministically fails with "Not equal: unexpected JSON representation at row 1" on the mysql-pos config (passing on both mysql-gtid configs), indicating the JSON snapshot/CDC mismatch the PR aims to fix is still present on that path — a real, config-dependent bug, not flakiness. |
❌ Test FailureAnalysis: A deterministic value-equality assertion ("unexpected JSON representation at row 1") fails in the very test (Test_MySQL_JSON_SnapshotCDCConsistency) that validates this PR's own fix for JSON snapshot/CDC alignment, indicating the fix is incomplete rather than a flaky failure. |
❌ Test FailureAnalysis: The PR's own new test Test_MySQL_JSON_SnapshotCDCConsistency deterministically fails a fixed JSON value assertion (expected "1.0", got "1") in both standalone and cluster variants, a real snapshot/CDC representation-mismatch bug the PR aims to fix — not a timeout, race, or network flake. |
MySQL normalizes whole-number JSON doubles (1.0 -> 1) on storage, so the previous exact-literal assertion was wrong. Rewrite the e2e test around the invariant that actually matters for DBI-823: the snapshot and CDC representations of the same JSON value must be byte-identical. Center the variants on object key ordering, which is where the old CDC path (Go-map lexicographic order) diverged from MySQL's (length-then-byte) order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ Test FailureAnalysis: A deterministic value mismatch in Test_MySQL_JSON_SnapshotCDCConsistency (JSON float serialized as "1.0000005e+06" instead of "1000000.5") that reproduced identically across multiple matrix jobs and directly reflects the unfixed bug this PR targets — not a flaky failure. |
❌ Test FailureAnalysis: A real, deterministic failure: the PR's own new test Test_MySQL_JSON_SnapshotCDCConsistency fails a JSON float-representation equality assertion identically across both MySQL_CH and MySQL_CH_Cluster variants, indicating the PR's snapshot/CDC JSON-alignment fix is incomplete rather than any flakiness. |
…son-representation-snapshot-cdc-mismatch
For some magnitudes Go's float formatting (CDC decoder) emits scientific notation where MySQL's text protocol emits plain decimal (e.g. 1.0000005e+06 vs 1000000.5) -- a documented go-mysql limitation, values still round-trip. Use small non-whole doubles that render identically in both paths so the snapshot/CDC consistency assertion holds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Companion to Test_MySQL_JSON_SnapshotCDCConsistency that enables PEERDB_CLICKHOUSE_ENABLE_JSON so the JSON column lands in a ClickHouse JSON column. ClickHouse parses and normalizes the text on insert, which absorbs the double text-format divergence the snapshot and CDC paths cannot make byte-identical themselves (e.g. 1000000.5 vs 1.0000005e+06), so the previously-diverging float values read back identically. Compares toString(js) since GetRows does not scan the native JSON type directly. Requires ClickHouse >= 25.3, same as Test_JSON. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| case types.QValueKindJSON: | ||
| return types.QValueJSON{Val: string(v)}, nil | ||
| // keep snapshot and cdc json representation consistent | ||
| return types.QValueJSON{Val: compactMySQLJSON(v)}, nil |
There was a problem hiding this comment.
same here, could gate behind internal mirror version to avoid inconsistency
🔄 Flaky Test DetectedAnalysis: TestGenericPG/Test_Simple_Flow timed out waiting for STATUS_SNAPSHOT (generic_test.go:143) — a timing/timeout failure in a Postgres test unrelated to this MySQL-JSON PR that passed cleanly in the parallel matrix job on the same commit. ✅ Automatically retrying the workflow |
|
|
||
| // Test_MySQL_JSON_SnapshotCDCConsistency_NativeJSON is the native-JSON companion to | ||
| // Test_MySQL_JSON_SnapshotCDCConsistency. | ||
| func (s ClickHouseSuite) Test_MySQL_JSON_SnapshotCDCConsistency_NativeJSON() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
today we technically don't support PEERDB_CLICKHOUSE_ENABLE_JSON for pg/mysql because ClickHouse JSON does not support non-object JSON types (e.g. number, string); but still good to get test coverage here still.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
| js json NOT NULL | ||
| )`, srcFullName))) | ||
|
|
||
| variants := []string{ |
There was a problem hiding this comment.
nit: would be good to add a test case for 1.0 to test out type-faithfulness mentioned in the PR description
There was a problem hiding this comment.
ok.. tried it out
it doesn't work on mysql 5.7 inside non native json test:
https://github.com/PeerDB-io/peerdb/actions/runs/28851748697/job/85568516983?pr=4535
mysql 5.7 collapses 1.0 into 1 on the snapshot path
on the cdc path we get 1.0 as expected with the new flag set to true
sadly, not much we can do about it
mysql 5.7 behavior:
INSERT INTO jtest.t (js) VALUES ('{\"n\": [3, 1, 2], \"obj\": {\"y\": 1.0, \"x\": 2}}');
SELECT js FROM jtest.t;
SELECT JSON_TYPE(JSON_EXTRACT(js,'\$.obj.y')) FROM jtest.t;
----
{"n": [3, 1, 2], "obj": {"x": 2, "y": 1}}
DOUBLE
the only thing we could do is to extract value type using JSON_TYPE+JSON_EXTRACT, but I think it's too expensive (also will quite complex code which doesn't seem to worth it)
There was a problem hiding this comment.
we could also keep RenderJSONAsMySQLText=OFF for mysql 5.7
BUT it means that everything else will stay inconsistent on cdc path
i would rather keep it ON for all versions and maybe describe this snapshot 1.0 double inconsistency in docs if it's worth it
❌ Test FailureAnalysis: Deterministic assertion failure in the PR's own new Test_MySQL_JSON_SnapshotCDCConsistency test (snapshot vs CDC JSON representation differs) on the mysql-pos/7.0 matrix while mysql-gtid/8.0 passed — a real, version-dependent bug the fix doesn't fully cover, not flakiness. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
❌ Test FailureAnalysis: A deterministic assertion failure in Test_MySQL_JSON_SnapshotCDCConsistency (large integer not preserved faithfully in JSON) that reproduces identically across two suite variants and directly tests the exact behavior this PR modifies, indicating a real bug rather than flakiness. |
❌ Test FailureAnalysis: Deterministic assertion failure in Test_MySQL_JSON_SnapshotCDCConsistency across all 3 MySQL matrix jobs: the PR's own snapshot/CDC JSON-alignment fix is incomplete, as the snapshot path still emits a space after the colon ({"big": ...}) while CDC emits compact JSON ({"big":...})—a real bug, not a flake. |
|
Didn't get to this yet, agree the MySQL JSON nuances need a more thorough look |
ilidemi
left a comment
ilidemi
left a comment
There was a problem hiding this comment.
Cool that a recent change makes this easier.
CH supports extracting larger ints from JSON strings as well https://fiddle.clickhouse.com/1db6559b-7fe3-4640-b9e5-29411baf4c57
Ultra long decimals aren't supported and would need a custom decoder/encoder but we'll wait for a feature request, especially that it doesn't break the pipe like PG did
dtunikov
deleted the
fix/dbi-823/json-representation-snapshot-cdc-mismatch
branch
3 participants
Before -> After:
The three divergences (snapshot vs. old CDC, flag OFF)
Comparing {"id": 1, "pi": 3.14, "val": 1.0, "name": "a"} against {"id":1,"name":"a","pi":3.14,"val":1}:
Notes
SNAPSHOT SELECTreturns1instead of1.0, not much we can do about that.