Skip to content

Fix handling of JSON data #182

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

Merged

Conversation

baloo
Copy link
Collaborator

@baloo baloo commented Dec 14, 2016

Introduced in 5.7.8

I chose not to handle charset parsing, all dict returned are in binary form. User is expected to parse it.

Fixes #181

@baloo baloo force-pushed the baloo/features/field-type-json branch 3 times, most recently from 1afb5e7 to cafcb60 Compare December 15, 2016 00:49
@baloo baloo changed the title Fix handling of JSON data WIP: Fix handling of JSON data Dec 15, 2016
@baloo
Copy link
Collaborator Author

baloo commented Dec 15, 2016

This is a very rough implementation that misses a LOT of types but this gives a good starting point. Do not merge it yet.

@baloo
Copy link
Collaborator Author

baloo commented Dec 15, 2016

https://github.com/percona/percona-server/blob/5.7/sql/json_binary.cc Here is the official "documentation" for mysql json type

@baloo baloo force-pushed the baloo/features/field-type-json branch from cafcb60 to 7e52344 Compare December 15, 2016 06:28
@baloo baloo changed the title WIP: Fix handling of JSON data Fix handling of JSON data Dec 15, 2016
@baloo
Copy link
Collaborator Author

baloo commented Dec 15, 2016

@namabile I'd love if you could give this a shot before I merge it.

@baloo baloo force-pushed the baloo/features/field-type-json branch from 7e52344 to e760cb8 Compare December 15, 2016 06:34
@namabile
Copy link

Awesome. I'll pull this later today and give it a shot.

@namabile
Copy link

Gave this a try and looks really good.

I hit an error on json types 4 and 5. Here are the cases I tested:

insert into test (id, value) values (1, 'true');
insert into test (id, value) values (1, '9');

Let me know if I can provide any info to help debug.

@baloo
Copy link
Collaborator Author

baloo commented Dec 16, 2016

Well, those are easy to fix though. I intentionally left those one out. I though json was spec'ed as either an object or an array. No other structure. I just re-read the spec. I was wrong.
I'll fix those.

@baloo baloo force-pushed the baloo/features/field-type-json branch from e760cb8 to 882a5a2 Compare December 16, 2016 15:49
self.tearDown()
self.setUp()

def test_json_basic(self):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is a test for your last usecase @namabile

Introduced in 5.7.8

Fixes julien-duponchelle#181

Signed-off-by: Arthur Gautier <[email protected]>
@baloo baloo force-pushed the baloo/features/field-type-json branch from 882a5a2 to 0b011e0 Compare December 17, 2016 09:44
@baloo
Copy link
Collaborator Author

baloo commented Dec 17, 2016

Hey @noplay, let me know what you think about this one.

Copy link
Owner

@julien-duponchelle julien-duponchelle left a comment

Choose a reason for hiding this comment

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

Sound good to me, thanks a lot for taking care of this one

@baloo baloo merged commit fe827f7 into julien-duponchelle:master Dec 18, 2016
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.

4 participants