jiayuasu commented on PR #1052: URL: https://github.com/apache/sedona-db/pull/1052#issuecomment-5087795024
Thanks for the thorough review, and for the green light to land this as experimental. Addressed the polish: - **EXPERIMENTAL** markers on the module docstring, `from_geopandas`, and both class docstrings. - **Dependencies**: dropped the hard `geopandas` dependency — now `sedonadb`, `sedonadb-expr` (for `.geo`), and `pyarrow` (for literals), with `geopandas` as an optional extra for the interop helpers. - **Cheap `__repr__`**: `GeoDataFrame.__repr__` no longer collects (it reports columns + active geometry); Jupyter gets a `_repr_html_` that collects a small preview. `Series.__repr__` now shows the underlying expression with a hint to call `.to_pandas()`. - **`.crs`** now reads `schema.field(name).type.crs` instead of a zero-row collect. - **`to_crs`** uses `lit(crs)` — confirmed `lit` handles strings, `pyproj.CRS`, and EPSG ints. - **Geometry column**: defaults via the primary-geometry heuristic (`primary_geometry_column()`), and an explicitly-provided name is validated as an actual geometry column (`KeyError` if absent, `ValueError` if not geometry). Geography is left permitted. - **No-geometry frames** handled: `.crs` returns `None`, `.geometry` raises a clear `AttributeError`, and a column subset that drops geometry re-derives it rather than carrying a stale name. - **`_operand`** passes a raw SedonaDB expression through and raises a clear `TypeError` for a pandas `Series` / numpy array (no row alignment), instead of failing obscurely as a multi-element literal. - **Docstrings** converted from reST to markdown. - **Testing**: added the `assert_geopandas_expr_equal(gdf, op)` harness you suggested (applies the same op to both and compares, sorting first since order isn't preserved), plus tests for lazy `repr`, array-like rejection, geometry validation, and no-geometry frames. 9 → 14 tests. Left the idea of generating method docstrings from the `.geo` accessor docs for a follow-up — agreed it's the right direction for getting more value out of the markdown docs, but it needs a bit more thought about how it interacts with IDE completion. On the framing: I've rewritten the PR description around two motivations rather than GeoPandas compatibility as an end in itself. The one I'd underweighted is **cross-engine consistency** — Sedona already ships `sedona.spark.geopandas`, so someone moving between SedonaSpark and SedonaDB meeting a similar API seems worth something, in the same spirit as aligning Python/R/SQL. Interestingly the conventions already converge: that implementation also uses the `sgpd` alias, puts geometry ops on `GeoSeries` and delegates from the active geometry column, and its contributor guide makes exactly the lazy-evaluation point about not eagerly resolving the CRS — which is what motivated the `.crs`/`repr` changes above. Later slices will match its signatures (`sjoin(other, predicate=..., distance=...)`, `bounds`, `envelope`, `geom_type`, `to_wkt()`, …) rather than inventing our own. The second motivation is the one you named, and it's holding up: this exercise has already produced #1050 (`unnest` was genuinely missing) and #1093 (`ST_Union_Agg` returns NULL for point inputs, which `dissolve` needs). One packaging question worth raising separately: for exact symmetry with `sedona.spark.geopandas`, the natural namespace would be `sedona.db.geopandas`. That lives in the `apache-sedona` package rather than here, so I've kept `sedonadb_geopandas` as the import for now — but happy to pursue it if that symmetry seems worth the coordination. -- 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]
