Implement ST_IsValidReason using geos library

[Abeeujah]

This pull request references the discussions had in #224

[petern48]

Suggested change

	Some("LINESTRING (0 0, 1 1)"),
	Some("LINESTRING (0 0, 1 1)"),
	Some("Polygon((0 0, 2 0, 1 1, 2 2, 0 2, 1 1, 0 0))"),

Let's add this case, since it's a type of reason we haven't included yet. On PostGIS, it returns Ring Self-intersection

[Abeeujah]

Yeah, true!

[petern48]

Suggested change

	ArrayScalar(Polygon(10), Polygon(10)),
	Polygon(10),

These are like arguments to the function, since is_valid only takes one geometry, the value here is just a single Polygon. You can see st_centroid above as an example. This should fix the CI failure.

[Abeeujah]

Correctly So, I've adjusted accordingly!

[petern48]

Suggested change

	ArrayScalar(Polygon(10), Polygon(500)),
	Polygon(500),

[Abeeujah]

Thank you once again!

[petern48]

@paleolimbot I noticed PostGIS returns Self-intersection[0.5 0.5] while geos only gives us the Self-intersection part currently. Do we want to implement the [ ] part? I personally don't mind leaving the [ ] part for a separate later issue if we want it, since I'm not sure how simple it would be to implement that off the top of my head.

Also, I realize we should probably be implementing function stubs too, right? (for my one curiosity) Do they serve any extra purpose other than documentation?

[paleolimbot]

Thank you!

Do we want to implement the [ ] part? I personally don't mind leaving the [ ] part for a separate later issue if we want it, since I'm not sure how simple it would be to implement that off the top of my head.

I agree we can't do this for now because is_valid_detail() is not exposed in the Rust GEOS crate. For what it's worth, the relevant GEOS function is:

https://github.com/libgeos/geos/blob/32c23d1fe658c575e187579ae49dc821df2651b6/capi/geos_c.h.in#L3422-L3440

...and where it would go in the geos crate is:

https://github.com/georust/geos/blob/47afbad2483e489911ddb456417808340e9342c3/src/geometry.rs#L106-L126

Also, I realize we should probably be implementing function stubs too, right? (for my one curiosity) Do they serve any extra purpose other than documentation?

The main purpose is documentation and possibly to help interfaces generate logical plans (since stubs can compute an output type from input types). We haven't quite set that up yet and so I don't think function implementation PRs need to handle it just yet 🙂

[petern48]

Sorry @Abeeujah, looks like we have merge conflicts again, could you resolve them? While doing so, could you also re-order the code in the following files, so they are all in alphabetical order? I didn't notice you've been putting them all at the bottom.

• geos-functions.rs (the benches)
• register.rs

Make sure you merge with main, there was a recent PR that fixed the ordering for the existing ones. Enforcing this order will also help with reducing merge conflicts a bit (helping PRs get merged faster), since not everything is being placed at the bottom of the list. Generally, we try to follow alphabetical order in everything (or at least everything I can think of), when there's a list of functions.

[petern48]

I should've been clearer: I just meant reorder the functions you implemented recently (ie. what you did for register.rs), not the entire file. Hopefully you didn't spend too much time manually sorting (sorry 😓).

I didn't mean to come off as the ordering is super strict (it's not). I'd guess none of the files are in the exact same order. As an FYI, they do typically tend to be in a sort of grouped alphabetical order (see this register.rs to see this visually). My main point is 1) you don't need to put them all at the bottom but also 2) don't stress about the ordering. Just place it where ever in the file that makes sense to you. It's unrealistic for reviewers to check and enforce a strict ordering.

My main intention was to give you a heads-up for any future PRs that there is some (rough) ordering in these files, and following it can help avoid merge conflicts, which helps Dewey merge them faster.

[paleolimbot]

Thank you! (And apologies I lost track of the approval to merge)

Peter is correct...I do usually alphabetize things because I've been doing this for a while and it can help prevent merge conflicts, but also I usually don't make people do this when I review except as a side note.

[Abeeujah]

@petern48 It's fine actually, I should have paid more attention to details, I definitely would in future PRs 👍
@paleolimbot It's cool actually, maintaining is not the easiest thing to do in the world, and you guys have been great all through 👍