jiayuasu commented on code in PR #3279:
URL: https://github.com/apache/sedona/pull/3279#discussion_r3850236892
##########
python/sedona/spark/geopandas/geodataframe.py:
##########
@@ -830,6 +851,21 @@ def __init__(
"or by providing a DataFrame with column name 'geometry'",
)
+ # A locally owned geometry column that still carries no CRS metadata
+ # at this point genuinely has no CRS (it was never derived from a raw
+ # or Sedona-managed source with its own CRS state) -- record that
+ # explicitly so later `.crs` reads don't fall back to a distributed
+ # SRID lookup just to discover there isn't one.
+ if crs is None and is_locally_owned and self._geometry_column_name is
not None:
+ active_geometry = self.geometry
+ has_crs_metadata, _ = read_crs_metadata(
+ active_geometry._internal.data_fields[0]
+ )
+ if not has_crs_metadata:
+ self[self._geometry_column_name] = (
Review Comment:
I think this assignment can cross pandas-on-Spark anchors.
`_record_no_crs_metadata()` builds a projected frame, so assigning its Series
back here goes through different-frame alignment.
A small reproducer is:
```python
with ps.option_context("compute.ops_on_diff_frames", False):
sgpd.GeoDataFrame(
{"zone": ["a"], "geometry": [Point(0, 0)]}
)
```
The Spark 3.4 CI run currently raises `ValueError: Cannot combine ...
different dataframe` at this line. With cross-frame operations enabled,
duplicate indexes may be joined and multiply rows instead. Could we attach the
alias and updated `InternalField` through the existing internal frame, for
example with `with_new_spark_column`, rather than assigning a derived Series?
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -662,6 +673,26 @@ def __init__(
if crs is not None:
self.set_crs(crs, inplace=True, allow_override=True)
+ elif is_locally_owned:
+ self._record_no_crs_metadata(inplace=True)
+
+ def _record_no_crs_metadata(self, inplace: bool = False) -> "GeoSeries":
+ """Record authoritative "no CRS" metadata without touching geometry
bytes.
+
+ Unlike ``set_crs``, this never calls ``ST_SetSRID``, so any SRID
+ already embedded in the geometry (e.g. from WKB/EWKB input) survives
+ untouched even though the public ``.crs`` becomes authoritatively
+ ``None`` instead of falling back to a distributed SRID lookup.
+ """
+ result = self._query_geometry_column(
Review Comment:
Could this path preserve valid falsy names?
```python
s = sgpd.GeoSeries([Point(0, 0)], name=0)
assert s.name == 0
s = sgpd.GeoSeries([Point(0, 0)], name="")
assert s.name == ""
```
Both names appear to become `None` because `_query_geometry_column(...,
keep_name=True)` later checks `if keep_name and self.name`. A metadata-only
update that keeps the existing label would avoid that; alternatively, the
helper could test `self.name is not None`.
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -662,6 +673,26 @@ def __init__(
if crs is not None:
self.set_crs(crs, inplace=True, allow_override=True)
+ elif is_locally_owned:
Review Comment:
I think a CRS carried by a local `GeometryArray` is lost before this branch:
```python
arr = gpd.GeoSeries([Point(0, 0)], crs=4326).array
s = sgpd.GeoSeries(arr)
assert s.crs.to_epsg() == 4326
```
Only a `gpd.GeoSeries` is checked for an inherited CRS above; a
`GeometryArray`, or a plain `pd.Series` backed by one, is converted to object
and then stamped as authoritative `None` here. Could we read `data.crs` /
`data.array.crs` before conversion and use that value when setting the metadata
and embedded SRID?
##########
python/tests/geopandas/test_geoseries.py:
##########
@@ -6678,3 +6679,104 @@ def test_crs_metadata_propagation(self):
assert frame.crs.to_epsg() == 4326
assert round_tripped.crs.to_epsg() == 4326
assert transformed.crs.to_epsg() == 3857
+
+ def test_local_construction_records_authoritative_no_crs(self):
+ """Locally built GeoSeries with no explicit crs must record an
+ authoritative "no CRS" state (metadata present, value None) instead
+ of being left indistinguishable from a raw column of unknown CRS
+ provenance (metadata absent)."""
+
+ def assert_no_crs_metadata(series: GeoSeries):
+ has_metadata, value =
read_crs_metadata(series._internal.data_fields[0])
+ assert has_metadata is True
+ assert value is None
+ assert series.crs is None
+
+ assert_no_crs_metadata(sgpd.GeoSeries([Point(1, 1), Point(2, 2)]))
+ assert_no_crs_metadata(sgpd.GeoSeries([Point(1, 1)], crs=None))
+ assert_no_crs_metadata(sgpd.GeoSeries([], name="polygons", crs=None))
+ assert_no_crs_metadata(sgpd.GeoSeries([None, None]))
+ assert_no_crs_metadata(sgpd.GeoSeries(gpd.GeoSeries([Point(1, 1)])))
+ assert_no_crs_metadata(sgpd.GeoSeries.from_wkt(["POINT (1 1)", "POINT
(2 2)"]))
+ assert_no_crs_metadata(
+ sgpd.GeoSeries.from_wkb([Point(1, 1).wkb, Point(2, 2).wkb])
+ )
+
+ gdf = sgpd.GeoDataFrame({"geometry": [Point(1, 1), Point(2, 2)]})
+ assert_no_crs_metadata(gdf.geometry)
+
+ with ps.option_context("compute.ops_on_diff_frames", True):
+ set_via_array = sgpd.GeoDataFrame({"value": [1, 2]}).set_geometry(
+ [Point(0, 0), Point(1, 1)]
+ )
+ assert_no_crs_metadata(set_via_array.geometry)
+
+ def test_wrapped_raw_column_keeps_unknown_crs_state(self):
+ """Wrapping an existing distributed column of unknown provenance (no
+ Sedona CRS metadata) must not fabricate a "no CRS" state -- the
+ legacy SRID-inference fallback must still apply."""
+ raw_ps_series = (
+ self.spark.createDataFrame([(Point(1, 1).wkb,)], "wkb binary")
+ .selectExpr("ST_GeomFromWKB(wkb) as geometry")
+ .pandas_api()["geometry"]
+ )
+
+ wrapped = GeoSeries(raw_ps_series)
+ has_metadata, _ = read_crs_metadata(wrapped._internal.data_fields[0])
+ assert has_metadata is False
+
+ def test_no_crs_stamp_preserves_embedded_srid(self):
+ """Recording an authoritative "no CRS" state must be metadata-only:
+ it must never rewrite the geometry bytes, so any SRID already
+ embedded via WKB/EWKB survives untouched."""
+ srid_series = sgpd.GeoSeries([Point(1, 1)]).set_crs(4326,
allow_override=True)
+ # Detach the authoritative CRS metadata but keep the embedded SRID,
+ # simulating a column whose bytes carry a SRID Sedona doesn't yet
+ # know about as metadata.
+ srid_series._record_no_crs_metadata(inplace=True)
+
+ assert srid_series.crs is None
+ embedded_srid = srid_series._internal.spark_frame.select(
+ stf.ST_SRID(srid_series.spark.column).alias("srid")
+ ).first()["srid"]
+ assert embedded_srid == 4326
+
+ def test_local_construction_no_spark_job_for_crs_discovery(self):
+ """Accessing .crs on a locally constructed GeoSeries with no CRS
+ metadata should not launch any Spark jobs - it should read metadata
+ directly without SRID inference."""
+ s = sgpd.GeoSeries([Point(1, 1), Point(2, 2)])
+
+ # Capture the number of pending jobs before access
+ before_jobs = 0
+ try:
+ _ = s.crs
+ after_jobs =
len(self.session._sc._jvm.scala.concurrent.FutureCache.getPythonPlans(s))
Review Comment:
I may be missing a fixture, but `TestBase` exposes `spark` and `sc`, not
`session`. That would make this line raise `AttributeError`, and the broad
`except Exception` then turns the result into zero jobs, so the test still
passes without measuring anything. `before_jobs` is also unused.
Could we use a Spark job group/status tracker around the operation instead,
without swallowing instrumentation errors? For example, assign a unique job
group, construct the required binary predicate from two local CRS-less
operands, and assert `self.sc.statusTracker().getJobIdsForGroup(group)` stays
empty before materialization. The companion plan test could inspect
`queryExecution().optimizedPlan()` and check for `BatchEvalPython`,
`ArrowEvalPython`, and `PythonUDF`.
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -4376,7 +4407,13 @@ def _create_from_select(
ps_series = first_series(PandasOnSparkDataFrame(internal))
name = None if name == SPARK_DEFAULT_SERIES_NAME else name
ps_series.rename(name, inplace=True)
- return GeoSeries(ps_series, index, crs=crs)
+ result = GeoSeries(ps_series, index, crs=crs)
+ if crs is None:
+ # ST_GeomFromWKB/WKT always produce fresh geometries with no
Review Comment:
Could we soften this rationale? `ST_GeomFromWKB` can preserve an SRID
carried by EWKB.
For example:
```python
geom = shapely.set_srid(Point(0, 0), 4326)
ewkb = shapely.to_wkb(geom, include_srid=True)
s = sgpd.GeoSeries.from_wkb([ewkb])
assert s._internal.spark_frame.select(
stf.ST_SRID(s.spark.column)
).first()[0] == 4326
```
The metadata-only stamping still seems right. Suggested wording: WKT and
ordinary WKB are generally SRID-less, while EWKB may carry an embedded SRID;
recording public `crs=None` must not rewrite it.
##########
python/tests/geopandas/test_geodataframe.py:
##########
@@ -1041,3 +1056,45 @@ def test_to_arrow(self):
def check_geodataframe(df):
assert isinstance(df, GeoDataFrame)
+
+ def test_local_construction_no_spark_job_for_crs_discovery(self):
Review Comment:
It looks like these two tests are nested inside the top-level
`check_geodataframe` helper, so pytest does not collect them.
This should show the issue:
```bash
pytest --collect-only -q python/tests/geopandas/test_geodataframe.py |
grep local_construction_no
```
Only the earlier named-geometry test is collected. Could we move both
methods into `TestGeoDataFrame`, then use the same real job-count and
query-plan assertions as the GeoSeries coverage?
##########
python/sedona/spark/geopandas/geoseries.py:
##########
@@ -588,6 +588,17 @@ def __init__(
if crs is None and isinstance(data, gpd.GeoSeries):
crs = data.crs
+ # Locally owned data (a Python list, WKT/WKB, a plain pandas Series, a
+ # bare geopandas.GeoSeries, ...) is distinct from wrapping an existing
+ # Sedona-managed or raw distributed structure: only in the former case
+ # do we know, right now, whether a CRS is set, so only there do we
+ # need to record an authoritative "no CRS" state below when the user
+ # didn't pass one. Wrapping an existing structure just inherits
+ # whatever CRS state (known, known-absent, or unknown) it already had.
+ is_locally_owned = not isinstance(
Review Comment:
Could the file-constructor path be handled explicitly? `GeoSeries.from_file`
passes `df.geometry` here, so a CRS-less file is classified as distributed and
remains metadata-free.
A regression check could be:
```python
s = sgpd.GeoSeries.from_file(crsless_geojson, format="geojson")
assert read_crs_metadata(s._internal.data_fields[0]) == (True, None)
```
At the moment `from_file` also reads `df.crs` eagerly, which can run SRID
inference, and a later `s.crs` can repeat it. Could the reader attach the
authoritative known or known-absent state directly from the file metadata
instead?
##########
python/sedona/spark/geopandas/geodataframe.py:
##########
@@ -830,6 +851,21 @@ def __init__(
"or by providing a DataFrame with column name 'geometry'",
)
+ # A locally owned geometry column that still carries no CRS metadata
+ # at this point genuinely has no CRS (it was never derived from a raw
+ # or Sedona-managed source with its own CRS state) -- record that
+ # explicitly so later `.crs` reads don't fall back to a distributed
+ # SRID lookup just to discover there isn't one.
+ if crs is None and is_locally_owned and self._geometry_column_name is
not None:
Review Comment:
Could we apply this to every locally created geometry column rather than
only the active one?
```python
gdf = sgpd.GeoDataFrame({
"geometry": [Point(0, 0)],
"secondary": [Point(1, 1)],
})
has_metadata, _ = read_crs_metadata(
gdf["secondary"]._internal.data_fields[0]
)
assert has_metadata
```
The secondary column still appears metadata-free, so
`gdf.set_geometry("secondary").crs` takes the distributed SRID fallback. For
pandas/GeoPandas inputs with different per-column CRSs, those inactive CRS
values can also be lost during the object conversion. One option would be to
capture CRS by column position before conversion, then update every geometry
field with either its CRS or explicit no-CRS metadata.
--
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]
