Copilot commented on code in PR #3190:
URL: https://github.com/apache/sedona/pull/3190#discussion_r3659939816
##########
python/sedona/spark/geopandas/geodataframe.py:
##########
@@ -396,7 +437,11 @@ def _get_geometry(self) -> sgpd.GeoSeries:
)
raise MissingGeometryColumnError(msg)
- return self[self._geometry_column_name]
+ geometry = self[self._geometry_column_name]
+ empty_crs_source = getattr(self, "_empty_crs_source", None)
+ if empty_crs_source is not None:
+ geometry._empty_crs_source = empty_crs_source
+ return geometry
Review Comment:
Storing a frame-level CRS fallback as a `GeoSeries` object
(`_empty_crs_source`) and injecting it into each returned geometry series can
become stale if the underlying geometry column is updated in-place through the
returned `GeoSeries` (e.g., `df.geometry.set_crs(..., inplace=True)`), because
the DataFrame’s `_empty_crs_source` is not updated. In SRID-0/empty cases, this
can cause `df.crs` / `df.geometry.crs` to keep reporting the old CRS via the
stale fallback. A more robust approach is to store the explicit fallback as a
value on the frame (e.g., `_empty_crs_value`) and only use a lineage reference
when needed, or otherwise ensure `_empty_crs_source` is kept in sync whenever
the active geometry column’s CRS metadata is changed in-place.
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -343,6 +343,9 @@ def __init__(
self._col_label: Label
self._sindex: SpatialIndex = None
self._empty_crs_source: typing.Optional["GeoSeries"] = None
+ # Explicit CRS metadata wins over the lineage fallback below when
+ # geometry rows are empty or carry SRID 0.
+ self._empty_crs_value = None
Review Comment:
`GeoSeries.__init__` introduces `_empty_crs_value`/`_empty_crs_source`, but
the constructor does not copy these from an input `GeoSeries`/`GeoDataFrame`
when `crs` isn’t explicitly provided. As a result, `GeoSeries(existing_series)`
can drop the explicit CRS fallback metadata (especially for empty/SRID-0
cases), which undermines the goal of preserving CRS metadata through
construction. Consider copying `_empty_crs_value` and `_empty_crs_source` from
`data` when `isinstance(data, GeoSeries)` (and potentially when `data` is a
`GeoDataFrame` geometry) unless the `crs` argument is provided (in which case
the explicit override should win).
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -482,7 +489,16 @@ def crs(self) -> Union["CRS", None]:
srid = 0 if np.isnan(srid) else srid
# Sedona returns 0 if SRID doesn't exist.
- return CRS.from_user_input(srid) if srid != 0 else None
+ if srid != 0:
+ return CRS.from_user_input(srid)
+ # These fallbacks are metadata rather than a fresh read from geometry
+ # coordinates. Explicit metadata takes precedence over inherited
+ # lineage metadata, including for non-empty geometries with SRID 0.
+ if self._empty_crs_value is not None:
+ return self._empty_crs_value
+ if self._empty_crs_source is not None:
+ return self._empty_crs_source.crs
+ return None
Review Comment:
This introduces new precedence behavior for *non-empty* geometries with SRID
0 (explicit metadata should win over lineage). The added tests cover
empty/all-null cases, but they don’t appear to directly assert the SRID-0
non-empty path (e.g., setting a CRS that normalizes to no EPSG so `to_epsg()`
is `None` and SRID becomes 0, while also having a conflicting lineage
fallback). Adding a focused test for this case would better lock in the
intended precedence rules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
