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

Add tagged parameters to encode. #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kzemek
Copy link

@kzemek kzemek commented Aug 24, 2017

Tagged Elixir strings allow pushing BINARY and CHAR fields in a single query. They also can be used in Ecto adapters to leverage Ecto's knowledge of types.

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage decreased (-0.04%) to 65.71% when pulling b697a9e on kzemek:master into c654c88 on xerions:master.

@coveralls
Copy link

coveralls commented Aug 24, 2017

Coverage Status

Coverage increased (+0.004%) to 65.757% when pulling 6df8e8b on kzemek:master into c654c88 on xerions:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 65.757% when pulling 6df8e8b on kzemek:master into c654c88 on xerions:master.

@fishcakez
Copy link
Contributor

Whilst this works for Ecto I do not think it is a nice API for those using Mariaex directly. I think a better approach would be to use the parameter type information sent back by the database when a query is prepared to determine the parameter types. We currently ignore the parameter definitions. This means we can use the type that the database expects. This will allow us to be more efficient with integers (and other some types?) so that we send less data over the wire and MySQL does not do implicit casting.

We should also be checking in the "describe" phase of the prepare that we know how to encode/decode the parameters/columns, and fail if not.

@kzemek
Copy link
Author

kzemek commented Aug 28, 2017

I agree that's probably the best way to solve the issue; however, I currently lack resources to implement it. I've fallen back to using raw SQL queries with inline hex-encoded binaries which is ugly and hardly ideal (especially since I lose some Ecto model goodies) but sidesteps the problem.

Note that even using MariaEx directly I'd vastly prefer to use %Mariaex.TypedValue{type: :binary, value: value} with parametrized queries instead of inlining the hex-encoded value, so I think this is a case of perfect being the enemy of good.

@fishcakez
Copy link
Contributor

@kzemek I have some db_connection/mariaex things to do this week so I can look into this for you if no one picks up. It should give us wins elsewhere, and bring some consistency between adapters.

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

3 participants