Skip to content

Add geometry types #4

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

Closed
wojtekmach opened this issue Oct 23, 2018 · 22 comments
Closed

Add geometry types #4

wojtekmach opened this issue Oct 23, 2018 · 22 comments

Comments

@wojtekmach
Copy link
Member

No description provided.

@wojtekmach wojtekmach added this to the v0.2 milestone Oct 23, 2018
@wojtekmach wojtekmach removed this from the v0.2 milestone Jan 2, 2019
@byronpc
Copy link

byronpc commented Sep 27, 2019

hi! are there any plans to support this? because the mysql driver is now deprecated but we can't upgrade to myxql without geometry support :/

@wojtekmach
Copy link
Member Author

wojtekmach commented Sep 27, 2019 via email

@byronpc
Copy link

byronpc commented Sep 27, 2019

alright this is welcome news! thanks!

@jeroenbourgois
Copy link
Contributor

@wojtekmach @byronpc I started on this to give it a go. If anyone is interested in collaborating, please let me know. My progress will be slow, but hey, yesterday I managed to parse a Point!

@m13m
Copy link

m13m commented Nov 13, 2019

@jeroenbourgois I would love to contribute can you share your fork lets work on that fork :)

@jeroenbourgois
Copy link
Contributor

@m13m great news! I will give you access to the repo. I will also open a ticket to discuss next steps, so we do not get in each others way :)

@m13m
Copy link

m13m commented Nov 14, 2019

@jeroenbourgois Thanks for the invite.

@jeroenbourgois
Copy link
Contributor

jeroenbourgois commented Nov 15, 2019

@wojtekmach @byronpc tiny update from our side: we can already parse something. This is the output from iex running some basis tests:

%MyXQL.Geometry.Polygon{                                                                                                                                              
  coordinates: [                                                                                                                                                       
    [[{0.0, 0.0}, {10.0, 0.0}, {10.0, 10.0}, {0.0, 10.0}, {0.0, 0.0}]]                                                                                                 
  ],                                                                                                                                                                   
  srid: nil                                                                                                                                                            
}                                                                                                                                                                      
%MyXQL.Geometry.MultiPolygon{                                                                                                                                          
  coordinates: [                                                                                                                                                       
    [                                                                                                                                                                  
      [                                                                                                                                                                
        [{0.0, 0.0}, {10.0, 0.0}, {10.0, 10.0}, {0.0, 10.0}, {0.0, 0.0}],                                                                                              
        [{0.0, 0.0}, {10.0, 0.0}, {10.0, 10.0}, {0.0, 10.0}, {0.0, 0.0}]                                                                                               
      ]                                                                                                                                                                
    ]                                                                                                                                                                  
  ],                                                                                                                                                                   
  srid: 0                                                                                                                                                              
} 

But when actually using it inside our app, there is a problem decoding other fields, which is strange and we have not found the root cause yet. Progress is slow because everything is quite new for us.

We think in the decoding part of the geometry structures, we are doing something to disturb the rest bits, which then, of course, causes issues further down the road.

Any help is still very welcome :)

@wojtekmach
Copy link
Member Author

If you have a script (or ideally an integration test) I could run in your fork I'm happy to take a look.

we are doing something to disturb the rest bits,

yeah that might be it that you read too few or too many bits. I believe geometry types are encoded as string<lenenc> so since you know the length of the value make sure you read exactly that many bits.

You probably have something like this already, but what I'd try is create table g (name text, p point); (or whatever geometry type you want to test against), and observe queries like:

  • select p from g
  • select name, p from g
  • select p, name from g

@wojtekmach
Copy link
Member Author

@jeroenbourgois I pushed a branch where I was able to do some basic decoding for POINT and MULTIPOINT for the binary protocol: d0771b8 hope that helps. We can probably learn a lot from https://github.com/xerions/mariaex/pull/191/files too. To really push this forward I think it would be extremely helpful to find the specification (if one exists) how exactly mysql represents geometry types on the wire instead of trying to reverse engineer it. If there's no such specification perhaps we could use some existing implementation as a reference.

@wojtekmach
Copy link
Member Author

Btw, I found out there's a geo [1] package (used by e.g. geo_postgis [2] for Postgrex) which is interesting. It's too early to make a decision on this but something to have in mind is potentially using this package (fortunately it has 0 deps) so that we use the same structs and thus provide interoperability.

[1] https://hex.pm/packages/geo
[2] https://hex.pm/packages/geo_postgis

@wojtekmach
Copy link
Member Author

I've updated the branch with encoding support: https://github.com/elixir-ecto/myxql/compare/wm-geometry

@jeroenbourgois
Copy link
Contributor

jeroenbourgois commented Nov 16, 2019

@wojtekmach thanks! I checked out the branch you created, good work. As for decoding, for point and polygon we are pretty solid, I think we figured out the same things you did about the srid, the endianness and so on. Maybe it's a good idea to work on the fork too? We added the geom structs there already, and some decode functions, it is rather confusing having multiple implementations to look at.

The thing you noticed about some header being prepended by MySQL internally is also something we saw, just before the srid bits. We think it is an integer value and depending on the value itself, it is spread over one, two or three bytes. We found this header also in the Geo package you mentioned and some other erlang libs. We solved it this way:

 def decode_geometry_head(<<0xFC, _::uint2, r::bits>>), do: r
  def decode_geometry_head(<<0xFD, _::uint3, r::bits>>), do: r
  def decode_geometry_head(<<0xFE, _::uint8, r::bits>>), do: r
  def decode_geometry_head(<<_lt251::uint1, r::bits>>), do: r

I think this works, because if it would be incorrect, I suppose we could never decode the remaining bits into a polygon. The thing is, the header changes as the polygon changes: more rings and suddenly it is a three byte value.

UPDATE: sorry, I see you also saw that header thing, you handled it with the decode_string_lenenc. I will try to merge all you code into our fork, so we are on the same path again. What does the decode_string_lenenc actually do?

Or otherwise, if you prefer to work on this branch, on this repo, maybe you can add me as contributor? I just think it might be favorable to both work on the same code, since I have a lot to learn and it is not that easy to mix and match and understand everything that is going on :P

More work will be for next week. Thanks already.

@wojtekmach
Copy link
Member Author

You mentioned you saw the header in geo and other libs - could you send links to relevant pieces of code? If your fork is public can you send a link too?

the <<0xFC, _::uint2>>, <<0xFD, _::uint3>> looks exactly how MySQL writes length-encoded integers (or strings) on the wire.

Yup, feel free to continue working on this on the fork but I’d appreciate a PR upstream when you’re done :)

@jeroenbourgois
Copy link
Contributor

https://github.com/jackjoe/myxql

I found out how the length-encoded integers work thanks to the MySQL docs, I never heard of it. Did not know it was for strings as wel. But what really interests me is what the value inside that ‘header’ represents. Now we just shave it off.

I will look up the libs and mention them here. On the road now.

@wojtekmach
Copy link
Member Author

wojtekmach commented Nov 16, 2019

@jeroenbourgois I think I got it, still don't know where the <<0, 0, 0, 0>> in front comes from (maybe srid?) but the Geo library handles decoding of the rest of the packet very well. I've updated the branch. You should be able to easily handle polygons and multi polygons there, let me know!

@jeroenbourgois
Copy link
Contributor

jeroenbourgois commented Nov 18, 2019

@wojtekmach pure awesome 😎
In a strange way a bit dissapointed in myself for not finding this elegant solution as fast as you did, but it seems to work great. And still I learned a big deal about bit syntax, so that's not lost.

I pulled you branch in our fork, I will add a couple of more tests and update the docs regarding the geo dep. In an actual app (using Phoenix/Ecto) it does require to have some setup with also the geo dep, and a custom Ecto type. Not too hard, but where could I put that information? Maybe this readme is not the best place, but there could be a page in the docs?

I will submit the PR from the updated wm-geometry branch on our side for you to review. Thanks again for your help!

@wojtekmach
Copy link
Member Author

Thanks @jeroenbourgois sounds good!

In an actual app (using Phoenix/Ecto) it does require to have some setup with also the geo dep, and a custom Ecto type. Not too hard, but where could I put that information? Maybe this readme is not the best place, but there could be a page in the docs?

We could make :geo an optional dependency so it won't be pulled in by people that don't need it, but honestly given it has 0 transitive dependency I'm inclined to just depend on it. Thus, there's no setup step.

Regarding a custom Ecto type, I'm a bit torn because on one hand myxql is completely independent from Ecto, but on the other hand it seems practical to put this information here. The only other place I could think would be this: https://hexdocs.pm/ecto_sql/Ecto.Adapters.MyXQL.html.

Btw, custom ecto type isn't required, this should work out of the box:

# migration.exs
add :point, :point

# schema.ex
field :point, :map

# code
Repo.insert!(%User{point: %Geo.Point{coordinates: {1, 1}}})

but with this we can't really handle user input. In your application, how are you getting the data into the system by the users?

@jeroenbourgois
Copy link
Contributor

@wojtekmach well, users are able to create polygons and put points on the map client side, with javascript, and we then send a comma separated string of {x,y} coords to the server. There it is parsed to a struct (using the custom ecto type, the cast/1 method). We also use the Phoenix.HTML.Safe behaviour in orde to be able to output the struct.

But it is good to know it can be done without too.

@wojtekmach
Copy link
Member Author

wojtekmach commented Nov 18, 2019 via email

@jeroenbourgois
Copy link
Contributor

@wojtekmach ok, I just submitted the PR (#88) If more work is needed, let us know.

@wojtekmach
Copy link
Member Author

Closing in favour of the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants