Use MariaEx typed binary and string if supported #2183
Conversation
|
Hello, @kzemek! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
lib/ecto/adapters/mysql.ex
Outdated
|
|
||
| ## Custom MySQL types | ||
|
|
||
|
|
There was a problem hiding this comment.
There should be no more than 1 consecutive blank lines.
| def loaders(:binary_id, type), do: [Ecto.UUID, type] | ||
| def loaders({:embed, _} = type, _), do: [&json_decode/1, &Ecto.Adapters.SQL.load_embed(type, &1)] | ||
| def loaders(_, type), do: [type] | ||
| def loaders(type_a, type_b), do: additional_loaders(type_a, type_b) |
There was a problem hiding this comment.
All the loaders are inlined for the reasons of efficiency. This code is called very often, even a function call is a big overhead here - we should avoid it, if we can.
| @behaviour Ecto.Adapter.Storage | ||
| @behaviour Ecto.Adapter.Structure | ||
|
|
||
| # Add dumpers functions if tagged values are supported by the driver |
There was a problem hiding this comment.
We don't need the conditional here, instead we should change "mix.exs" to the minimum Mariaex version that supports this feature.
|
We have added some comments, thank you! |
josevalim
changed the title
|
Please see my comment in the Mariaex issue, we should be using the type information sent back by the database to encode parameters instead of doing implicit casting. This should mean we can remove the |
4 participants
By default, MariaEx doesn't distinguish between raw binaries and text strings. MariaEx
queryfeatures an optionbinary_asthat makes the library either treat all Elixir strings as raw binaries or as text. This option, however, is not easily accessible from Ecto and is not a solution when a query features both a raw binary and text string.I've created a pull request xerions/mariaex#198 that allows passing Elixir string parameters wrapped in
%Mariaex.TypedValuewith an explicittypefield (with a value of either:binaryor:string) to remove ambiguity.This PR leverages Ecto's knowledge of the model to inform MariaEx of the exact types of Elixir string parameters. The change is made in a backward-compatible way; Travis tests using updated MariaEx can be found here.