Repository: geopandas/geopandas

Issue:
Changing a geodataframe's row order inplace should invalidate its spatial index
This bit me recently, but when a geodataframe's order of rows changes "in place", the `sindex` should be invalidated and recomputed. Here's a reproducible example:
```
import geopandas
import pandas as pd
from shapely import wkt

# create an example gdf
df = pd.DataFrame(
    {'City': ['Buenos Aires', 'Brasilia', 'Santiago', 'Bogota', 'Caracas'],
     'Country': ['Argentina', 'Brazil', 'Chile', 'Colombia', 'Venezuela'],
     'Coordinates': ['POINT(-34.58 -58.66)', 'POINT(-15.78 -47.91)',
                     'POINT(-33.45 -70.66)', 'POINT(4.60 -74.08)',
                     'POINT(10.48 -66.86)']})

df['Coordinates'] = df['Coordinates'].apply(wkt.loads)
gdf = geopandas.GeoDataFrame(df, geometry='Coordinates')

# create spatial index without invalidating sindex
before_spatial_index = gdf.sindex
gdf.sort_values('City', inplace=True)
after_spatial_index = gdf.sindex

before_spatial_index == after_spatial_index # returns True (I expected False)

# create spatial index after invalidating sindex
gdf._invalidate_sindex()
after_spatial_index = gdf.sindex
before_spatial_index == after_spatial_index  # returns False (what I initially expected)
```

Discussion:
martinfleis: Thanks for the PR!

This is indeed the safest option, but also a bit unnecessary. As you correctly pointed out, there are some inplace changes which are perfectly fine. Spatial index is expensive to compute, so I'd try to minimise the need of its invalidation and recomputation. But `sort_values` might not be the only case causing the issue. 
koshy1123: @martinfleis Thanks for the comment! I updated the PR to check if the dataframe's index is identical before and after the update. The logic here is to only invalidate the spatial index if the dataframe's index has changed.
martinfleis: That is not ideal either. .sindex is based on integer location, not the index. So if you do reset_index() or set_index() inplace, index will be different but the order remains the same. Hence spatial index will be still valid. I would maybe try to specify which cases causes problem and then fix those specifically, instead of trying to catch them all here. 
martinfleis: It seems that this has been resolved in the meantime. Thanks for looking into that @koshy1123!
jorisvandenbossche: It might still be good to add the tests for this, though, so we ensure this keeps working correctly. 

(I suppose the actual fix might have happened by moving the sindex to the GeometryArray, and `sort_values` will then still mutate the array even if updating the geodataframe in place)
martinfleis: > It might still be good to add the tests for this, though, so we ensure this keeps working correctly.

In that case we should move them to `test_sindex.py` (and ensure they pass).
jorisvandenbossche: > In that case we should move them to `test_sindex.py` (and ensure they pass).

Yep, just did both :)

Patch:
diff --git a/geopandas/geodataframe.py b/geopandas/geodataframe.py
index 54adb0ed..c82cba5f 100644
--- a/geopandas/geodataframe.py
+++ b/geopandas/geodataframe.py
@@ -633,7 +633,10 @@ class GeoDataFrame(GeoPandasBase, DataFrame):
                 "geometry": shape(feature["geometry"]) if feature["geometry"] else None
             }
             # load properties
-            row.update(feature["properties"])
+            properties = feature["properties"]
+            if properties is None:
+                properties = {}
+            row.update(properties)
             rows.append(row)
         return GeoDataFrame(rows, columns=columns, crs=crs)
 
@@ -1024,6 +1027,16 @@ individually so that features may have different properties
         GeoDataFrame.to_file : write GeoDataFrame to file
         """
 
+        # Accept engine keyword for compatibility with pandas.DataFrame.to_parquet
+        # The only engine currently supported by GeoPandas is pyarrow, so no
+        # other engine should be specified.
+        engine = kwargs.pop("engine", "auto")
+        if engine not in ("auto", "pyarrow"):
+            raise ValueError(
+                f"GeoPandas only supports using pyarrow as the engine for "
+                f"to_parquet: {engine!r} passed instead."
+            )
+
         from geopandas.io.arrow import _to_parquet
 
         _to_parquet(self, path, compression=compression, index=index, **kwargs)


Test patch:
diff --git a/geopandas/io/tests/test_arrow.py b/geopandas/io/tests/test_arrow.py
index 1e701581..4c8d82c5 100644
--- a/geopandas/io/tests/test_arrow.py
+++ b/geopandas/io/tests/test_arrow.py
@@ -7,7 +7,7 @@ import pytest
 from pandas import DataFrame, read_parquet as pd_read_parquet
 from pandas.testing import assert_frame_equal
 import numpy as np
-from shapely.geometry import box
+from shapely.geometry import box, Point
 
 import geopandas
 from geopandas import GeoDataFrame, read_file, read_parquet, read_feather
@@ -23,6 +23,7 @@ from geopandas.io.arrow import (
     METADATA_VERSION,
 )
 from geopandas.testing import assert_geodataframe_equal, assert_geoseries_equal
+from geopandas.tests.util import mock
 
 
 # Skip all tests in this module if pyarrow is not available
@@ -172,6 +173,28 @@ def test_validate_metadata_invalid(metadata, error):
         _validate_metadata(metadata)
 
 
+def test_to_parquet_fails_on_invalid_engine(tmpdir):
+    df = GeoDataFrame(data=[[1, 2, 3]], columns=["a", "b", "a"], geometry=[Point(1, 1)])
+
+    with pytest.raises(
+        ValueError,
+        match=(
+            "GeoPandas only supports using pyarrow as the engine for "
+            "to_parquet: 'fastparquet' passed instead."
+        ),
+    ):
+        df.to_parquet(tmpdir / "test.parquet", engine="fastparquet")
+
+
+@mock.patch("geopandas.io.arrow._to_parquet")
+def test_to_parquet_does_not_pass_engine_along(mock_to_parquet):
+    df = GeoDataFrame(data=[[1, 2, 3]], columns=["a", "b", "a"], geometry=[Point(1, 1)])
+    df.to_parquet("", engine="pyarrow")
+    # assert that engine keyword is not passed through to _to_parquet (and thus
+    # parquet.write_table)
+    mock_to_parquet.assert_called_with(df, "", compression="snappy", index=None)
+
+
 # TEMPORARY: used to determine if pyarrow fails for roundtripping pandas data
 # without geometries
 def test_pandas_parquet_roundtrip1(tmpdir):
diff --git a/geopandas/tests/test_geodataframe.py b/geopandas/tests/test_geodataframe.py
index 94672e4e..59f93592 100644
--- a/geopandas/tests/test_geodataframe.py
+++ b/geopandas/tests/test_geodataframe.py
@@ -619,7 +619,7 @@ class TestDataFrame:
         p3 = Point(3, 3)
         f3 = {
             "type": "Feature",
-            "properties": {"a": 2},
+            "properties": None,
             "geometry": p3.__geo_interface__,
         }
 
@@ -627,10 +627,103 @@ class TestDataFrame:
 
         result = df[["a", "b"]]
         expected = pd.DataFrame.from_dict(
-            [{"a": 0, "b": np.nan}, {"a": np.nan, "b": 1}, {"a": 2, "b": np.nan}]
+            [{"a": 0, "b": np.nan}, {"a": np.nan, "b": 1}, {"a": np.nan, "b": np.nan}]
         )
         assert_frame_equal(expected, result)
 
+    def test_from_features_empty_properties(self):
+        geojson_properties_object = """{
+          "type": "FeatureCollection",
+          "features": [
+            {
+              "type": "Feature",
+              "properties": {},
+              "geometry": {
+                "type": "Polygon",
+                "coordinates": [
+                  [
+                    [
+                      11.3456529378891,
+                      46.49461446367692
+                    ],
+                    [
+                      11.345674395561216,
+                      46.494097442978195
+                    ],
+                    [
+                      11.346918940544128,
+                      46.49385370294394
+                    ],
+                    [
+                      11.347616314888,
+                      46.4938352377453
+                    ],
+                    [
+                      11.347514390945435,
+                      46.49466985846028
+                    ],
+                    [
+                      11.3456529378891,
+                      46.49461446367692
+                    ]
+                  ]
+                ]
+              }
+            }
+          ]
+        }"""
+
+        geojson_properties_null = """{
+          "type": "FeatureCollection",
+          "features": [
+            {
+              "type": "Feature",
+              "properties": null,
+              "geometry": {
+                "type": "Polygon",
+                "coordinates": [
+                  [
+                    [
+                      11.3456529378891,
+                      46.49461446367692
+                    ],
+                    [
+                      11.345674395561216,
+                      46.494097442978195
+                    ],
+                    [
+                      11.346918940544128,
+                      46.49385370294394
+                    ],
+                    [
+                      11.347616314888,
+                      46.4938352377453
+                    ],
+                    [
+                      11.347514390945435,
+                      46.49466985846028
+                    ],
+                    [
+                      11.3456529378891,
+                      46.49461446367692
+                    ]
+                  ]
+                ]
+              }
+            }
+          ]
+        }"""
+
+        # geoJSON with empty properties
+        gjson_po = json.loads(geojson_properties_object)
+        gdf1 = GeoDataFrame.from_features(gjson_po)
+
+        # geoJSON with null properties
+        gjson_null = json.loads(geojson_properties_null)
+        gdf2 = GeoDataFrame.from_features(gjson_null)
+
+        assert_frame_equal(gdf1, gdf2)
+
     def test_from_features_geom_interface_feature(self):
         class Placemark(object):
             def __init__(self, geom, val):
diff --git a/geopandas/tests/test_op_output_types.py b/geopandas/tests/test_op_output_types.py
index 5e9b5d96..dd25ff91 100644
--- a/geopandas/tests/test_op_output_types.py
+++ b/geopandas/tests/test_op_output_types.py
@@ -88,8 +88,8 @@ def test_getitem(df):
     assert_object(df[["value1", "value2"]], pd.DataFrame)
     assert_object(df[[geo_name, "geometry2"]], GeoDataFrame, geo_name)
     assert_object(df[[geo_name]], GeoDataFrame, geo_name)
-    assert_object(df[["geometry2", "value1"]], GeoDataFrame, None, None)
-    assert_object(df[["geometry2"]], GeoDataFrame, None, None)
+    assert_object(df[["geometry2", "value1"]], GeoDataFrame, None)
+    assert_object(df[["geometry2"]], GeoDataFrame, None)
     assert_object(df[["value1"]], pd.DataFrame)
     # Series
     assert_object(df[geo_name], GeoSeries, geo_name)
@@ -102,12 +102,8 @@ def test_loc(df):
     assert_object(df.loc[:, ["value1", "value2"]], pd.DataFrame)
     assert_object(df.loc[:, [geo_name, "geometry2"]], GeoDataFrame, geo_name)
     assert_object(df.loc[:, [geo_name]], GeoDataFrame, geo_name)
-    # TODO: should this set the geometry column to geometry2 or return a DataFrame?
     assert_object(df.loc[:, ["geometry2", "value1"]], GeoDataFrame, None)
     assert_object(df.loc[:, ["geometry2"]], GeoDataFrame, None)
-    # Ideally this would mirror __getitem__ and below would be true
-    # assert_object(df.loc[:, ["geometry2", "value1"]], pd.DataFrame)
-    # assert_object(df.loc[:, ["geometry2"]], pd.DataFrame)
     assert_object(df.loc[:, ["value1"]], pd.DataFrame)
     # Series
     assert_object(df.loc[:, geo_name], GeoSeries, geo_name)
@@ -120,12 +116,8 @@ def test_iloc(df):
     assert_object(df.iloc[:, 0:2], pd.DataFrame)
     assert_object(df.iloc[:, 2:4], GeoDataFrame, geo_name)
     assert_object(df.iloc[:, [2]], GeoDataFrame, geo_name)
-    # TODO: should this set the geometry column to geometry2 or return a DataFrame?
     assert_object(df.iloc[:, [3, 0]], GeoDataFrame, None)
     assert_object(df.iloc[:, [3]], GeoDataFrame, None)
-    # Ideally this would mirror __getitem__ and below would be true
-    # assert_object(df.iloc[:, [3, 0]], pd.DataFrame)
-    # assert_object(df.iloc[:, [3]], pd.DataFrame)
     assert_object(df.iloc[:, [0]], pd.DataFrame)
     # Series
     assert_object(df.iloc[:, 2], GeoSeries, geo_name)
@@ -157,12 +149,8 @@ def test_reindex(df):
     assert_object(df.reindex(columns=[geo_name, "geometry2"]), GeoDataFrame, geo_name)
     assert_object(df.reindex(columns=[geo_name]), GeoDataFrame, geo_name)
     assert_object(df.reindex(columns=["new_col", geo_name]), GeoDataFrame, geo_name)
-    # TODO: should this set the geometry column to geometry2 or return a DataFrame?
     assert_object(df.reindex(columns=["geometry2", "value1"]), GeoDataFrame, None)
     assert_object(df.reindex(columns=["geometry2"]), GeoDataFrame, None)
-    # Ideally this would mirror __getitem__ and below would be true
-    # assert_object(df.reindex(columns=["geometry2", "value1"]), pd.DataFrame)
-    # assert_object(df.reindex(columns=["geometry2"]), pd.DataFrame)
     assert_object(df.reindex(columns=["value1"]), pd.DataFrame)
 
     # reindexing the rows always preserves the GeoDataFrame
@@ -181,12 +169,8 @@ def test_drop(df):
     assert_object(df.drop(columns=["value1", "value2"]), GeoDataFrame, geo_name)
     cols = ["value1", "value2", "geometry2"]
     assert_object(df.drop(columns=cols), GeoDataFrame, geo_name)
-    # TODO: should this set the geometry column to geometry2 or return a DataFrame?
     assert_object(df.drop(columns=[geo_name, "value2"]), GeoDataFrame, None)
     assert_object(df.drop(columns=["value1", "value2", geo_name]), GeoDataFrame, None)
-    # Ideally this would mirror __getitem__ and below would be true
-    # assert_object(df.drop(columns=[geo_name, "value2"]), pd.DataFrame)
-    # assert_object(df.drop(columns=["value1", "value2", geo_name]), pd.DataFrame)
     assert_object(df.drop(columns=["geometry2", "value2", geo_name]), pd.DataFrame)
 
 
@@ -226,7 +210,6 @@ def test_apply(df):
     assert_object(df[["geometry2", "value1"]].apply(identity, axis=1), pd.DataFrame)
 
     assert_object(df[["value1"]].apply(identity, axis=1), pd.DataFrame)
-    # if compat # https://github.com/pandas-dev/pandas/pull/30091
 
 
 def test_apply_axis1_secondary_geo_cols(df):
diff --git a/geopandas/tests/test_overlay.py b/geopandas/tests/test_overlay.py
index 14ea914a..166e9da5 100644
--- a/geopandas/tests/test_overlay.py
+++ b/geopandas/tests/test_overlay.py
@@ -14,6 +14,7 @@ from geopandas import _compat
 from geopandas.testing import assert_geodataframe_equal, assert_geoseries_equal
 import pytest
 
+
 DATA = os.path.join(os.path.abspath(os.path.dirname(__file__)), "data", "overlay")
 
 
@@ -795,3 +796,79 @@ def test_no_intersection():
     expected = GeoDataFrame(columns=["foo", "bar", "geometry"])
     result = overlay(gdf1, gdf2, how="intersection")
     assert_geodataframe_equal(result, expected, check_index_type=False)
+
+
+class TestOverlayWikiExample:
+    def setup_method(self):
+        self.layer_a = GeoDataFrame(geometry=[box(0, 2, 6, 6)])
+
+        self.layer_b = GeoDataFrame(geometry=[box(4, 0, 10, 4)])
+
+        self.intersection = GeoDataFrame(geometry=[box(4, 2, 6, 4)])
+
+        self.union = GeoDataFrame(
+            geometry=[
+                box(4, 2, 6, 4),
+                Polygon([(4, 2), (0, 2), (0, 6), (6, 6), (6, 4), (4, 4), (4, 2)]),
+                Polygon([(10, 0), (4, 0), (4, 2), (6, 2), (6, 4), (10, 4), (10, 0)]),
+            ]
+        )
+
+        self.a_difference_b = GeoDataFrame(
+            geometry=[Polygon([(4, 2), (0, 2), (0, 6), (6, 6), (6, 4), (4, 4), (4, 2)])]
+        )
+
+        self.b_difference_a = GeoDataFrame(
+            geometry=[
+                Polygon([(10, 0), (4, 0), (4, 2), (6, 2), (6, 4), (10, 4), (10, 0)])
+            ]
+        )
+
+        self.symmetric_difference = GeoDataFrame(
+            geometry=[
+                Polygon([(4, 2), (0, 2), (0, 6), (6, 6), (6, 4), (4, 4), (4, 2)]),
+                Polygon([(10, 0), (4, 0), (4, 2), (6, 2), (6, 4), (10, 4), (10, 0)]),
+            ]
+        )
+
+        self.a_identity_b = GeoDataFrame(
+            geometry=[
+                box(4, 2, 6, 4),
+                Polygon([(4, 2), (0, 2), (0, 6), (6, 6), (6, 4), (4, 4), (4, 2)]),
+            ]
+        )
+
+        self.b_identity_a = GeoDataFrame(
+            geometry=[
+                box(4, 2, 6, 4),
+                Polygon([(10, 0), (4, 0), (4, 2), (6, 2), (6, 4), (10, 4), (10, 0)]),
+            ]
+        )
+
+    def test_intersection(self):
+        df_result = overlay(self.layer_a, self.layer_b, how="intersection")
+        assert df_result.geom_equals(self.intersection).bool()
+
+    def test_union(self):
+        df_result = overlay(self.layer_a, self.layer_b, how="union")
+        assert_geodataframe_equal(df_result, self.union)
+
+    def test_a_difference_b(self):
+        df_result = overlay(self.layer_a, self.layer_b, how="difference")
+        assert_geodataframe_equal(df_result, self.a_difference_b)
+
+    def test_b_difference_a(self):
+        df_result = overlay(self.layer_b, self.layer_a, how="difference")
+        assert_geodataframe_equal(df_result, self.b_difference_a)
+
+    def test_symmetric_difference(self):
+        df_result = overlay(self.layer_a, self.layer_b, how="symmetric_difference")
+        assert_geodataframe_equal(df_result, self.symmetric_difference)
+
+    def test_a_identity_b(self):
+        df_result = overlay(self.layer_a, self.layer_b, how="identity")
+        assert_geodataframe_equal(df_result, self.a_identity_b)
+
+    def test_b_identity_a(self):
+        df_result = overlay(self.layer_b, self.layer_a, how="identity")
+        assert_geodataframe_equal(df_result, self.b_identity_a)
diff --git a/geopandas/tests/test_sindex.py b/geopandas/tests/test_sindex.py
index 0b53a8d4..c36c77d1 100644
--- a/geopandas/tests/test_sindex.py
+++ b/geopandas/tests/test_sindex.py
@@ -174,6 +174,31 @@ class TestFrameSindex:
         subset2 = self.df[["A", "geom"]]
         assert subset2.sindex is original_index
 
+    def test_rebuild_on_update_inplace(self):
+        gdf = self.df.copy()
+        old_sindex = gdf.sindex
+        # sorting in place
+        gdf.sort_values("A", ascending=False, inplace=True)
+        # spatial index should be invalidated
+        assert not gdf.has_sindex
+        new_sindex = gdf.sindex
+        # and should be different
+        assert new_sindex is not old_sindex
+
+        # sorting should still have happened though
+        assert gdf.index.tolist() == [4, 3, 2, 1, 0]
+
+    @pytest.mark.skipif(not compat.PANDAS_GE_11, reason="fails on pd<1.1.0")
+    def test_update_inplace_no_rebuild(self):
+        gdf = self.df.copy()
+        old_sindex = gdf.sindex
+        gdf.rename(columns={"A": "AA"}, inplace=True)
+        # a rename shouldn't invalidate the index
+        assert gdf.has_sindex
+        # and the "new" should be the same
+        new_sindex = gdf.sindex
+        assert old_sindex is new_sindex
+
 
 # Skip to accommodate Shapely geometries being unhashable
 @pytest.mark.skip