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

Array encoding/decoding #14

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

greg-rychlewski
Copy link

@greg-rychlewski greg-rychlewski commented Dec 17, 2022

Closes #7

ltree introduced a binary protocol a couple years back. If you take advantage of it then you can get arrays for free.

since binary encoding/decoding is only available in pg 13+ i changed this to text encoding

@greg-rychlewski
Copy link
Author

cc @warmwaffles in case you're still interested

@warmwaffles
Copy link
Contributor

I am still VERY interested in that.

@greg-rychlewski
Copy link
Author

@warmwaffles there's something else I can do, if the maintainer of this is busy with other things

i can make a package just for the binary protocol and then you'd just have to change [EctoLtree.Postgrex.Lquery, EctoLtree.Postgrex.Ltree] below to whatever my module names are

Postgrex.Types.define(
  MyApp.PostgresTypes,
  [EctoLtree.Postgrex.Lquery, EctoLtree.Postgrex.Ltree]
)

then it should just work™. if you're interested i can make a package quickly

@warmwaffles
Copy link
Contributor

Would be nice if this was just upstreamed into the postgres ecto adapter 😄

@greg-rychlewski
Copy link
Author

I'll give it a shot :). I think the stance in the past was to use packages for non-builtin data types. But this one is really simple so hopefully it's ok.

@warmwaffles
Copy link
Contributor

@greg-rychlewski the funny part is that the postgres ltree extension is in the documentation for custom adapters.

@josemrb josemrb self-assigned this Dec 19, 2022
@greg-rychlewski
Copy link
Author

Here goes nothing: elixir-ecto/postgrex#636

@greg-rychlewski
Copy link
Author

greg-rychlewski commented Dec 19, 2022

I just found out this only works for pg 13+. The text protocol is the only one prior to that.

@greg-rychlewski
Copy link
Author

greg-rychlewski commented Dec 19, 2022

You could do arrays for the text protocol too but you'd have to create another extension for ltree/lquery arrays. i.e. type: _ltree and type: _lquery. And then this would have to convert the Elixir list to the postgres array text protocol, i.e. {'this.is.path1, 'this.is.path2'}

If that's preferable for you all, I could submit a PR for that instead.

@warmwaffles
Copy link
Contributor

Ooof, I can't use this in our production use case at the moment. We are on PG12, but are working towards migrating to PG14.

@greg-rychlewski
Copy link
Author

Ok I'll change this PR to use text protocol and then Postgrex can handle the binary protocol.

@greg-rychlewski greg-rychlewski changed the title Use binary format Array encoding/decoding Dec 20, 2022
@greg-rychlewski
Copy link
Author

greg-rychlewski commented Dec 20, 2022

@warmwaffles I got the text protocol working, you just have to add these 2 new extensions:

EctoLtree.Postgrex.LqueryArray, EctoLtree.Postgrex.LtreeArray

I'm not sure if the decoding will fail in certain situations though. It assumes that commas aren't allowed in any of the individual elements. I'm not familiar enough with either data types to know if that's possible though.

I tried some queries like this and it failed in psql:

select '{*.path1.*,path2.*{2, 5}}'::lquery[];

If there are some cases where commas are allowed we just have to make the decoding smarter. If someone can find a query that breaks I can fix it.

@greg-rychlewski
Copy link
Author

greg-rychlewski commented Dec 20, 2022

BTW, the reason you need a completely different extension for the text protocol is because arrays are still just strings but formatted a certain way. It's not like the binary protocol where you can say "I have a collection of these other things that already exist".

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.

Array support
3 participants