Skip to content
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

Patch go-mysql to keep trailing zero in floats in json #368

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mtaner
Copy link
Contributor

@mtaner mtaner commented Nov 19, 2024

Patched json binary decoder from go-mysql library to add a custom marshal json method for the floats to keep the trailing 0.

This patch only works with mysql version 8, as mysql version 5.7 gets rid of the trailing 0 on cast...as json
Screenshot 2024-11-21 at 12 36 53.

This means that the tests added for checking that the trailing 0 is not dropped will fail for the 5.7 ruby tests so I have updated the test to ignore trailing 0 data for mysql version 5.7

Next steps are the open a PR with go-mysql highlighting that this will only work with mysql version 8 onwards (and add settings option similar to useDecimal to BinlogSyncerConfig and see what they think.

Copy link
Contributor

@driv3r driv3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets add a test case with something after decimal, just for completes, i.e. 52.19

@mtaner mtaner force-pushed the mt/patch-trailing-zero-json-serialisation branch from 579d72c to 41f382c Compare November 21, 2024 12:56
@mtaner mtaner marked this pull request as ready for review November 21, 2024 13:00
@mtaner mtaner changed the title Add config to keep trailing zero & patch Patch go-mysql to keep trailing zero in floats in json Nov 21, 2024
@@ -103,28 +106,33 @@ def test_json_data_insert
# with a JSON column is broken on 5.7.
# See: https://bugs.mysql.com/bug.php?id=87847
res = target_db.query("SELECT COUNT(*) AS cnt FROM #{DEFAULT_FULL_TABLE_NAME}")
assert_equal 16, res.first["cnt"]
assert_equal ENV["MYSQL_VERSION"] == "8.0" ? 22 : 18, res.first["cnt"] # 22 rows with trailing zero data, 18 without trailing zero

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any other better way? checking and counting manually sounds not like a sustainable/maintainable solution here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is always a better way :D Let me have a look at it again now I have slept on it. I wanted to make it really obvious that system behaves differently for different mysql versions.

The other approach I considered is getting rid of the trailing 0 for 5.7 instead of this seperation - I will have a look at it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

98d4b92 - changes here I think its better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the plan to upstream this change? If we make patches to the vendored libraries, we have to make sure that they're not overwritten. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is overwritten - the integration tests will start failing but the plan is to propose the upstream change to go-mysql and see what they say

Copy link
Contributor

@Scalvando Scalvando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it!

@mtaner mtaner force-pushed the mt/patch-trailing-zero-json-serialisation branch from 8086a46 to bda3785 Compare December 12, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants