fix(pg): handle SRID 4326 bbox tiles spanning over the antimeridian

[todtb]

Improves tile query bounding box calculation in table_to_query by applying buffer margins correctly for SRID 3857 (already supported) and 4326 (new) .

For SRID 4326, this uses ST_Expand with degree-based margins to fix antimeridian edge cases.

Resolves #982

[todtb]

Reading the SRID projection and unit from spatial_ref_sys seems like a workable approach to handling many (though not all) reference systems.

That said, I think a much simpler conditional for bbox_search could handle the vast majority of use cases by just looking at SRID 3857 or SRID 4326

[CommanderStorm]

Seems like a reasonable approach to me.
I am a but out of my depth in how these projections work though. 😰

I would like a bit more docs on how this can be used for our users.
At lease this (and the parts under /docs) should explain how srid, proj and proj_unit interact

For us, testing that things work is quite important.
To ensure that this works, I would like:

• one unit test testing proj=Some(..) with proj_unit=None
• one unit test testing proj=None with proj_unit=Some(..)
• one unit test testing proj=Some(..) with proj_unit=Some(..)
• the last would also make a nice integration test

[CommanderStorm]

I am not quite sure about the naming. I usually like to not hamming-code these things. Maybe srid_projection, srid_projection_unit?
But I think @nyurik has a better taste in this.

An alternative configuration format if these are srid-specific would be to set this as:
srid: {}

[todtb]

proj is pulled from the proj4text string so in this case has a specific meaning:

+proj=merc +a=6378137 +b=6378137 +lat_ts=0.0 +lon_0=0.0 +x_0=0.0 +y_0=0 +k=1.0 +units=m +nadgrids=@null +wktext +no_defs

Technically I suppose proj_unit would just be units but that seemed somewhat vague on its own.

Open to the other ideas though.

[todtb]

Seems like a reasonable approach to me. I am a but out of my depth in how these projections work though. 😰

I would like a bit more docs on how this can be used for our users. At lease this (and the parts under /docs) should explain how srid, proj and proj_unit interact

For us, testing that things work is quite important. To ensure that this works, I would like:

• one unit test testing proj=Some(..) with proj_unit=None
• one unit test testing proj=None with proj_unit=Some(..)
• one unit test testing proj=Some(..) with proj_unit=Some(..)
• the last would also make a nice integration test

Sure, makes sense.

It looks like martin/tests/pg_table_source_test.rs should be testing around the construction of table info from the database and so that should be covering:

• proj=Some(..) with proj_unit=None
• proj=Some(..) with proj_unit=Some(..)

by way of 4326 and 3857.

I'm not sure if there would be a scenario with proj=None unless it was a custom defined SRS. In which case this will always fallback to the existing default behavior anyway.

Yes, further integration tests are always helpful. I'm entirely new to rust and its ecosystem so I would appreciate any guidance on that, but it looks like we may already have a few integration tests with both 4326 and 3857 as a start.

[CommanderStorm]

I'm not sure if there would be a scenario with proj=None [with proj_unit=Some(..)] unless it was a custom defined SRS. In which case this will always fallback to the existing default behavior anyway.

Would it make sense to print a warning in such a case?

[todtb]

I think I understand a source of confusion here. There was an oversight on my part which was that table info can be serialized to an auto saved config file. This proj4 data is really only used internally and shouldn't be user configurable (it is based purely on database SRS definitions). I've marked it to not serialize.

I did still leave a 3857 table in martin/tests/pg_table_source_test.rs as the tests were previously only testing 4326, but this PR should no longer change config.yml output, so this is purely additional unrelated testing.

I think the best integration test additions here would be specifically around tile generation at x=0 (antemeridian). I'll look more into how test.sh is working, and see what data I can create to test this (such as in a tile like 2/0/1)

For what it's worth, I have tested these queries with a local data set and the changes are all looking great for 4326 and 3857 data sources, so I have a good idea of what data we can use in the tests to help protect against regressions.

[todtb]

@CommanderStorm - I've added integration tests with geometries specifically around the antimeridian. It includes geometries nearby and those that cross, both in the affected SRID 4326. Let me know if this looks good to you or if you have any suggested changes.

@nyurik - Overall, does the approach of reading this proj4 data for guidance on how to expand the bbox work for you?

[CommanderStorm]

I will need to look over the testcases in more depth (focus: is this correct).
This is hard to judge from githubs UI on an evening before the conference.

I will meet with @nyurik there and we can have a look if this is reasonable or if a case is left untested.

[todtb]

I will need to look over the testcases in more depth (focus: is this correct). This is hard to judge from githubs UI on an evening before the conference.

I will meet with @nyurik there and we can have a look if this is reasonable or if a case is left untested.

Thanks for the review. Looking forward to hearing what you both think and enjoy your conference!

[CommanderStorm]

Really nice work and the explaination really helps.

I approve of the changes, assuming that there is a testcase for the new, untested case 👍🏻

[CommanderStorm]

Will be merged once CI passes (not your fault):

• #1969