Fix non CRS incompatibility when saving to postgis #3048
Conversation
|
Do you mean Edit: looking at the code, are you using |
|
Hi @m-richards ! Thanks for your response. I did'nt notice that the existing test "test_write_postgis_without_crs" doesn't catch the if_exists conditional. I will review and submit a new PR. |
|
You can submit changes as new commits directly on this PR |
|
@m-richards , just added "append" on the test. Now we try to save on PostGIS twice: first replacing, second appending. One thing: somehow, CI/CD pre-commit is failing after linting my changes. Do you know why? |
This is due to the code formatting and linting rules we have as part of geopandas to keep things consistent. You might like to have a look at https://geopandas.org/en/latest/community/contributing.html#style-guide-linting, in particular the bit about |
geopandas/io/tests/test_sql.py
Outdated
| @@ -467,12 +467,15 @@ def test_write_postgis_without_crs(self, engine_postgis, df_nybb): | |||
| df_nybb.crs = None | |||
| with pytest.warns(UserWarning, match="Could not parse CRS from the GeoDataF"): | |||
| write_postgis(df_nybb, con=engine, name=table, if_exists="replace") | |||
| write_postgis(df_nybb, con=engine, name=table, if_exists="append") | |||
There was a problem hiding this comment.
can you move this after the assertion assert target_srid == 0, "SRID should be 0, found %s" % target_srid and add the code to test the assertion again. This works as is, but it would be nice to keep a test for if_exists="replace" only.
| # Testing when append table | ||
| with pytest.warns(UserWarning, match="Could not parse CRS from the GeoDataF"): | ||
| write_postgis(df_nybb, con=engine, name=table, if_exists="append") | ||
|
|
There was a problem hiding this comment.
| assert target_srid == 0, "SRID should be 0, found %s" % target_srid | |
You should add the assertion in too, this current just tests that if_exists="append" doesn't crash, but we want it to not crash and also give the right answer 😄
| @@ -483,6 +484,17 @@ def test_write_postgis_without_crs(self, engine_postgis, df_nybb): | |||
| with pytest.warns(UserWarning, match="Could not parse CRS from the GeoDataF"): | |||
| write_postgis(df_nybb, con=engine, name=table, if_exists="append") | |||
|
|
|||
| sql = text( | |||
| "SELECT Find_SRID('{schema}', '{table}', '{geom_col}')" | |||
| " ORDER BY '{geom_col}' DESC;".format( | |||
There was a problem hiding this comment.
is there a reason this order by is needed/ why we cannot reuse the same sql from above?
Could we instead just run conn.execute(sql).fetchall() below and check the srids for each row?
Before this fix, when the user tries to write a non CRS geometry on a PostGIS database, the action fails because the SRID was converted to -1 (Geopandas pattern), not 0 (PostGIS pattern).
Well, one important observation: I also think that the test related to the last commit on this issue was not executed, because the test was correctly implemented and it must fail on the previous implementation.