X Tutup
Skip to content

Add support for --hex-blob option in mysqldump#71

Open
vdechef wants to merge 26 commits intomysql2sqlite:masterfrom
vdechef:master
Open

Add support for --hex-blob option in mysqldump#71
vdechef wants to merge 26 commits intomysql2sqlite:masterfrom
vdechef:master

Conversation

@vdechef
Copy link

@vdechef vdechef commented Mar 22, 2020

Copy link
Collaborator

@dumblob dumblob left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It look good to me. Please try to address my comments or discuss them and then I'll merge it 😉. Don't forget also about the following I mentioned in the #69 discussion:

In that case we'd need some "unit" tests (I still couldn't find the time to consolidate and make them work - as commit policy), then I'd add a comment like # as of 2020-03-21 mysqldump --hex-blob is the sole way to get hexadecimal syntax in the whole INSERT/REPLACE output except for substrings of e.g. base64 etc. and last we also need to test it with more awk versions (e.g. macOS awk, FreeBSD awk, GNU awk).

@vdechef
Copy link
Author

vdechef commented Mar 23, 2020

Thanks for the PR. It look good to me. Please try to address my comments or discuss them and then I'll merge it 😉.

OK, I will fix this later this week. I will also try to improve unit tests.

@dumblob
Copy link
Collaborator

dumblob commented Mar 23, 2020

@vdechef take your time (mine is also a "scarcity").

@vdechef
Copy link
Author

vdechef commented Mar 24, 2020

I think I'm done.

  • I updated the code with your remarks, and added the comments you asked for
  • I replaced BIT with BLOB (but I did not remove the function used to handle conversion, just in case ... )
  • I also fixed a bug on ENUM regex, and cleaned some \ escaping as you told me to
  • I added some unit tests, in a way that it should be easy to extend (this can be discussed actually)

@vdechef vdechef requested a review from dumblob March 26, 2020 19:14
Copy link
Collaborator

@dumblob dumblob left a comment

Choose a reason for hiding this comment

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

Thanks! That's a lot of work. Please address my comments (I didn't finish the review for lack of time, but I'll finish it later - hopefully during the next 10 days or so).

@vdechef
Copy link
Author

vdechef commented Mar 30, 2020

So I just changed the unit tests to match what you wanted.

I also took one of the examples you had written, just to be sure that it would work with the helpers functions I added. And thanks to this I realized that the BIT type default values needed hexstring conversion. So I wrote a function for this and it seems like it is fixed now.

@vdechef vdechef requested a review from dumblob April 8, 2020 19:45
@vdechef
Copy link
Author

vdechef commented Apr 22, 2020

While testing a Node.js replacement for mysqldump, I realized that the regex used to handle VALUES(...) statement could easily be improved to be compatible with this library. So I made a small patch.

@dumblob I know you don't have much time, but do you think this PR has a chance to be accepted someday ?

@dumblob
Copy link
Collaborator

dumblob commented Apr 22, 2020

While testing a Node.js replacement for mysqldump, I realized that the regex used to handle VALUES(...) statement could easily be improved to be compatible with this library. So I made a small patch.

It's fine, the difference is negligible.

@dumblob I know you don't have much time, but do you think this PR has a chance to be accepted someday ?

I'm sorry I'm really out of time now, but skimming the PR it looks very good - it's a lot of hard work and I'm glad you devoted so much time and effort to this. To answer your question - yes, I definitely want to merge this PR!

One minor nit (I repeat I didn't read the changes thoroughly, so there might be other things once I'll find some more time) would be my preference to not "rename" utilities (with "rename" I'm referring to storing their names in variables and subsequently using these variables). Any good reasons for that? If we really needed to "shadow" a real utility (e.g. to increase cross-platform support or speed or whatever), then I'd definitely rather introduce a differently named function as a wrapper with an easy interface (e.g. fixed number of args) to make the need explicit.

@vdechef
Copy link
Author

vdechef commented Apr 23, 2020

To answer your question - yes, I definitely want to merge this PR!

Nice, I will continue to improve it when needed.

Any good reasons for that? If we really needed to "shadow" a real utility (e.g. to increase cross-platform support or speed or whatever), then I'd definitely rather introduce a differently named function as a wrapper with an easy interface (e.g. fixed number of args) to make the need explicit.

I totally agree. I just tried to mimic the style of existing code, but I didn't feel comfortable with this because the args of those utilities prevent their replacement without code refactoring.
I just updated the code to remove those declarations.

@vdechef
Copy link
Author

vdechef commented Sep 30, 2023

Hi @dumblob,
More than 3 years later, I rebased my PR and updated it with a CI file that will run the unit tests on Pull-Requests.
Feel free to review/merge this.

Vincent de Chefdebien and others added 2 commits October 1, 2023 23:21
@vdechef
Copy link
Author

vdechef commented Oct 1, 2023

I added a few more test cases.
Thanks to this I found that:

  1. hex integers longer than 16 characters are not truncated anymore (an error is raised when creating the database)
  2. bit fields set with integer values are not correctly parsed (we get ASCII instead of int)

Honestly I don't have enough energy to investigate this

Vincent de Chefdebien and others added 2 commits October 2, 2023 15:58
@vdechef
Copy link
Author

vdechef commented Oct 2, 2023

I just realized that the previous points were invalid:

  1. by digging in our old discussions, it appear that we stopped truncating hex integers, because this causes dataloss
  2. bit-fields are parsed correctly as long as one does not omit the --hex-blob option during mysqldump operation, as stated in the README

So I added a few more tests and pushed the code.

For the remaining test cases, I don't understand what they are supposed to test so I cannot implement them.

@dumblob
Copy link
Collaborator

dumblob commented Mar 30, 2024

@vdechef this is wonderful! Many thanks! I am indeed much less active during the last years but I really appreciate all the effort you have put into this.

Now we are considering moving everything under the organization https://github.com/mysql2sqlite - feel free to express yourself if you find this idea valuable and whether you would wish to become a member (for reviews etc.).

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.

2 participants

X Tutup