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

Handle text protocol COM_QUERY response #138

Merged
merged 4 commits into from
Dec 9, 2016

Conversation

jdangerx
Copy link
Contributor

The MySQL text protocol COM_QUERY response is not supported. I needed to communicated with a MySQL-like implementation that only supported the text protocol, so I was running into #132.

This is my implementation of the text query response protocol. The difference between the binary and text protocols is that the text protocol converts all the row data to length encoded strings. I've made Query.decode dispatch on the query type, added more function clauses to handle_text_query, and added a bunch of string parsing for the decode_text_rows function.

I also have a few questions/concerns about my code:

  • It seems like the handle_text_query function should be able to have more overlap with the handle_binary_query functions, but I'm not sure how that would work. It sure would be nice to DRY it up a bit.
  • It would be nice for my use case to pass query type (and whether or not to prepare the query) into Mariaex.query - does that seem like something that we could or should insert into the API?

@jdangerx jdangerx changed the title Text protocol Handle text protocol COM_QUERY response Sep 26, 2016
@coveralls
Copy link

coveralls commented Sep 26, 2016

Coverage Status

Coverage decreased (-9.2%) to 53.318% when pulling 31b34de on TeachersPayTeachers:text-protocol into 566ece4 on xerions:master.

@jdangerx
Copy link
Contributor Author

jdangerx commented Sep 26, 2016

Hm. Not sure why my changes messed with the existing unit tests. I'll fix them and update.

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage increased (+1.9%) to 64.411% when pulling 0a56b59 on TeachersPayTeachers:text-protocol into 566ece4 on xerions:master.

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage increased (+1.7%) to 64.286% when pulling b325c05 on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage increased (+2.1%) to 64.651% when pulling 39887ff on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage increased (+2.6%) to 65.146% when pulling ba54e00 on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 61.154% when pulling 94e95d5 on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

@jdangerx jdangerx force-pushed the text-protocol branch 2 times, most recently from 31c9ff4 to 9b17906 Compare September 27, 2016 16:52
@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage increased (+3.7%) to 66.202% when pulling 9b17906 on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage increased (+3.7%) to 66.202% when pulling 9b17906 on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

johnxia added 4 commits September 27, 2016 13:29
Adding tests and more data types
* Fix tablename overlap between 'test' and 'test'
* Also, slight refactoring of decode function dispatch

Elixir 1.1 does not support guards in else
@coveralls
Copy link

Coverage Status

Coverage increased (+3.7%) to 66.202% when pulling 04cacbd on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+3.7%) to 66.202% when pulling 04cacbd on TeachersPayTeachers:text-protocol into a0aa98b on xerions:master.

@jdangerx
Copy link
Contributor Author

Sorry about the coveralls/travis noise - I was having issues getting the Elixir 1.1 build to work. Tests pass everywhere now :)

@jdangerx
Copy link
Contributor Author

Ping @liveforeverx - tests pass, so the question is, is this functionality something we want to have around?

@liveforeverx
Copy link
Member

Pong, sorry for late answer.

@liveforeverx
Copy link
Member

What use case, do you need it for?

@liveforeverx
Copy link
Member

Is there some query or database, which didn't support prepared statements?

@jdangerx
Copy link
Contributor Author

Yes, as far as I can tell the Sphinx search MySQL protocol does not support prepared statements.

@jdangerx
Copy link
Contributor Author

I guess an alternative could be that we can add optional client side preparation of statements :\

@jdangerx
Copy link
Contributor Author

jdangerx commented Nov 1, 2016

@liveforeverx I forgot about this PR for a couple weeks. Revisiting this - what do you think of adding this functionality?

@fishcakez
Copy link
Contributor

I would like to implement cursor support so results of queries can be streamed with memory efficiency inside a transaction but this is prohibitively difficult if the text protocol is supported. Is it possible to drop text query support? I realise this is the opposite of the request in this PR.

@shantiii
Copy link

shantiii commented Nov 7, 2016

@fishcakez Hi there, I'm a coworker of @jdangerx, and I think dropping text protocol support is infeasible given the large number of mysql commands that are unable to be executed through the prepared statement/binary protocol. Currently, this is impacting us when we attempt to connect to Sphinx, which doesn't seem to have support for the binary protocol.

@fishcakez
Copy link
Contributor

@shantiii thanks for letting me. In that case I won't implement cursors but just do a standard query. I'll let someone else pick it up if they need it.

@liveforeverx
Copy link
Member

@fishcakez : Is it possible to support cursor only for prepared queries?

@fishcakez
Copy link
Contributor

@liveforeverx yeah I managed it here: #145.

@liveforeverx
Copy link
Member

@shantiii @jdangerx So, I would rebase your work and merge this week.

Do you need it working with elixir < 1.2?

@jdangerx
Copy link
Contributor Author

That's great news! We don't need it to work with elixir < 1.2. Are you planning on doing the rebasing or should I?

@liveforeverx liveforeverx merged commit 04cacbd into xerions:master Dec 9, 2016
@liveforeverx
Copy link
Member

liveforeverx commented Dec 9, 2016

@jdangerx Thanks for the work! Merged now. If you have further enhancements for text protocol, welcome.

@jdangerx
Copy link
Contributor Author

Thanks @liveforeverx! There are likely a few more enhancements in the pipeline, courtesy of @shantiii.

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.

None yet

5 participants