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

Handling of UUIDs #51

Closed
LostKobrakai opened this issue Aug 27, 2021 · 5 comments · Fixed by #52
Closed

Handling of UUIDs #51

LostKobrakai opened this issue Aug 27, 2021 · 5 comments · Fixed by #52

Comments

@LostKobrakai
Copy link
Contributor

While making sure we don't run into compatibility issues migrating from the sqlite_ecto2 adapter to this one I hit another difference: UUIDs.

Currently this adapter seems to store uuids as strings and the old adapter did opt for the smaller option of raw binary data. I found this one in the testsuite:

# TODO: We need to determine what format to store the UUID. Is it Text or binary 16?
# Are we going for readability or for compactness?

I'm wondering if you already decided for one way or the other? It would be really great if this adapter could be switched to using the binary-16 storage. In the embedded world storing data in a more than twice as big format doesn't sound appealing.

I've added the appropriate loaders/2 and dumpers/2 callback locally to use Ecto.UUID and it seems nothing in the testsuite broke and it made my compatibility check between the adapters work.

@kevinlang
Copy link
Member

My only concern with using the binary is that it is much harder to work with when using sqlite3 cli. AFAIK there are no built in utilities to query a UUID by its text representation within the CLI, unlike Postgres and MySQL. It is only possible by using the UUID extension, but most 99.9% of users will be using the distro-provided sqlite3 binary which will not have this.

I think it is reasonable to make it configurable - Postgres does this with the :map type.

    defp ecto_to_db(:map),                 do: Application.fetch_env!(:ecto_sql, :postgres_map_type)
    defp ecto_to_db({:map, _}),            do: Application.fetch_env!(:ecto_sql, :postgres_map_type)

We can do this by doing:

  def column_type(:uuid, _opts), do: Application.get_env(:ecto_sql, :uuid_type, "TEXT")
  def column_type(:binary_id, _opts), do: Application.get_env(:ecto_sql, :binary_id_type, "TEXT")

What do you think, @LostKobrakai ?

@LostKobrakai
Copy link
Contributor Author

I'm happy to leave it configurable for as long as we can somehow use the raw binary representation, though the column type is only part of the picture, the encoding/decoding is the bigger one :)

@kevinlang
Copy link
Member

Yeah, I think the loader/dumper can just look at that same config to determine which way to process it.

@warmwaffles
Copy link
Member

Yea I'd be okay with a configurable option with how to handle that.

We could also do the same thing for TEXT_DATETIME to be just DATETIME if people wanted.

@warmwaffles
Copy link
Member

It is only possible by using the UUID extension, but most 99.9% of users will be using the distro-provided sqlite3 binary which will not have this.

@kevinlang looking at the code in that extension, it looks like it is fallback safe and stores the values as bytes. But again, it won't be human friendly for those using a client without the extension. It would also make it incredibly difficult to insert a new record through the CLI for maintenance and what not.

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 a pull request may close this issue.

3 participants