You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "petern48 (via GitHub)" <gi...@apache.org> on 2025/07/21 22:43:56 UTC
[PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
petern48 opened a new pull request, #2131:
URL: https://github.com/apache/sedona/pull/2131
## Did you read the Contributor Guide?
- Yes, I have read the [Contributor Rules](https://sedona.apache.org/latest/community/rule/) and [Contributor Development Guide](https://sedona.apache.org/latest/community/develop/)
## Is this PR related to a ticket?
- Yes, and the PR name follows the format `[GH-XXX] my subject`. Closes #2129
## What changes were proposed in this PR?
## How was this patch tested?
Ensured existing tests pass and added a new complex df test for geodataframe
## Did this PR include necessary documentation updates?
- No, this PR does not affect any public API so no need to change the documentation.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #2131:
URL: https://github.com/apache/sedona/pull/2131
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2131:
URL: https://github.com/apache/sedona/pull/2131#discussion_r2223464009
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -969,16 +912,38 @@ def length(self) -> pspd.Series:
3 4.828427
dtype: float64
"""
- col = self.get_first_geometry_column()
- select = f"""
+
+ """
CASE
WHEN GeometryType(`{col}`) IN ('LINESTRING', 'MULTILINESTRING') THEN ST_Length(`{col}`)
WHEN GeometryType(`{col}`) IN ('POLYGON', 'MULTIPOLYGON') THEN ST_Perimeter(`{col}`)
WHEN GeometryType(`{col}`) IN ('POINT', 'MULTIPOINT') THEN 0.0
WHEN GeometryType(`{col}`) IN ('GEOMETRYCOLLECTION') THEN ST_Length(`{col}`) + ST_Perimeter(`{col}`)
- END"""
+ END"""
+
Review Comment:
Remove commented out SQL code block. This multi-line comment containing old SQL logic should be deleted as it's no longer relevant after the refactoring.
```suggestion
```
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -725,46 +672,31 @@ def _query_geometry_column(
GeoSeries
A GeoSeries with the operation applied to the geometry column.
"""
- if not cols:
- raise ValueError("No valid geometry column found.")
df = self._internal.spark_frame if df is None else df
- if isinstance(cols, str):
- col = cols
- data_type = df.schema[col].dataType
- if isinstance(data_type, BinaryType):
- query = query.replace(f"`{cols}`", f"ST_GeomFromWKB(`{cols}`)")
+ rename = self.name if self.name else SPARK_DEFAULT_SERIES_NAME
- rename = col if not rename else rename
+ col_expr = spark_col.alias(rename)
- elif isinstance(cols, list):
- for col in cols:
- data_type = df.schema[col].dataType
-
- if isinstance(data_type, BinaryType):
- # the backticks here are important so we don't match strings that happen to be the same as the column name
- query = query.replace(f"`{col}`", f"ST_GeomFromWKB(`{col}`)")
-
- # must have rename for multiple columns since we don't know which name to default to
- assert rename
-
- query = f"{query} as `{rename}`"
-
- exprs = [query]
+ exprs = [col_expr]
index_spark_columns = []
index_fields = []
if not is_aggr:
# We always select NATURAL_ORDER_COLUMN_NAME, to avoid having to regenerate it in the result
# We always select SPARK_DEFAULT_INDEX_NAME, to retain series index info
- exprs.append(SPARK_DEFAULT_INDEX_NAME)
- exprs.append(NATURAL_ORDER_COLUMN_NAME)
+
+ exprs.append(scol_for(df, SPARK_DEFAULT_INDEX_NAME))
+ exprs.append(scol_for(df, NATURAL_ORDER_COLUMN_NAME))
+
+ # exprs.append(SPARK_DEFAULT_INDEX_NAME)
+ # exprs.append(NATURAL_ORDER_COLUMN_NAME)
Review Comment:
Remove commented out code. These commented lines serve no purpose and should be deleted to improve code cleanliness.
```suggestion
```
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -725,46 +672,31 @@ def _query_geometry_column(
GeoSeries
A GeoSeries with the operation applied to the geometry column.
"""
- if not cols:
- raise ValueError("No valid geometry column found.")
df = self._internal.spark_frame if df is None else df
- if isinstance(cols, str):
- col = cols
- data_type = df.schema[col].dataType
- if isinstance(data_type, BinaryType):
- query = query.replace(f"`{cols}`", f"ST_GeomFromWKB(`{cols}`)")
+ rename = self.name if self.name else SPARK_DEFAULT_SERIES_NAME
- rename = col if not rename else rename
+ col_expr = spark_col.alias(rename)
- elif isinstance(cols, list):
- for col in cols:
- data_type = df.schema[col].dataType
-
- if isinstance(data_type, BinaryType):
- # the backticks here are important so we don't match strings that happen to be the same as the column name
- query = query.replace(f"`{col}`", f"ST_GeomFromWKB(`{col}`)")
-
- # must have rename for multiple columns since we don't know which name to default to
- assert rename
-
- query = f"{query} as `{rename}`"
-
- exprs = [query]
+ exprs = [col_expr]
index_spark_columns = []
index_fields = []
if not is_aggr:
# We always select NATURAL_ORDER_COLUMN_NAME, to avoid having to regenerate it in the result
# We always select SPARK_DEFAULT_INDEX_NAME, to retain series index info
- exprs.append(SPARK_DEFAULT_INDEX_NAME)
- exprs.append(NATURAL_ORDER_COLUMN_NAME)
+
+ exprs.append(scol_for(df, SPARK_DEFAULT_INDEX_NAME))
+ exprs.append(scol_for(df, NATURAL_ORDER_COLUMN_NAME))
+
+ # exprs.append(SPARK_DEFAULT_INDEX_NAME)
+ # exprs.append(NATURAL_ORDER_COLUMN_NAME)
Review Comment:
Remove commented out code. These commented lines serve no purpose and should be deleted to improve code cleanliness.
```suggestion
```
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -1531,7 +1514,7 @@ def get_geometry(self, index) -> "GeoSeries":
"""
# Sedona errors on negative indexes, so we use a case statement to handle it ourselves
- select = """
+ """
Review Comment:
Remove commented out SQL code block. This multi-line comment containing old SQL logic should be deleted as it's no longer relevant after the refactoring.
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -1585,15 +1581,21 @@ def boundary(self) -> "GeoSeries":
GeoSeries.exterior : outer boundary (without interior rings)
"""
- col = self.get_first_geometry_column()
# Geopandas and shapely return NULL for GeometryCollections, so we handle it separately
# https://shapely.readthedocs.io/en/stable/reference/shapely.boundary.html
- select = f"""
+ """
CASE
WHEN GeometryType(`{col}`) IN ('GEOMETRYCOLLECTION') THEN NULL
ELSE ST_Boundary(`{col}`)
- END"""
- return self._query_geometry_column(select, col, rename="boundary")
+ END
+ """
Review Comment:
Remove commented out SQL code block. This multi-line comment containing old SQL logic should be deleted as it's no longer relevant after the refactoring.
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -2924,55 +2990,43 @@ def _row_wise_operation(
NATURAL_ORDER_COLUMN_NAME if align is False else SPARK_DEFAULT_INDEX_NAME
)
- if not isinstance(other, pspd.Series):
- # generator instead of a in-memory list
- data = [other for _ in range(len(self))]
-
- # e.g int, Geom, etc
- other = (
- GeoSeries(data)
- if isinstance(other, BaseGeometry)
- else pspd.Series(data)
- )
-
- # To make sure the result is the same length, we set natural column as the index
- # in case the index is not the default range index from 0.
- # Alternatively, we could create 'other' using the same index as self,
- # but that would require index=self.index.to_pandas() which is less scalable.
- index_col = NATURAL_ORDER_COLUMN_NAME
-
# This code assumes there is only one index (SPARK_DEFAULT_INDEX_NAME)
# and would need to be updated if Sedona later supports multi-index
+
df = self._internal.spark_frame.select(
- col(self.get_first_geometry_column()).alias("L"),
+ self.spark.column.alias("L"),
# For the left side:
# - We always select NATURAL_ORDER_COLUMN_NAME, to avoid having to regenerate it in the result
# - We always select SPARK_DEFAULT_INDEX_NAME, to retain series index info
col(NATURAL_ORDER_COLUMN_NAME),
col(SPARK_DEFAULT_INDEX_NAME),
)
other_df = other._internal.spark_frame.select(
- col(_get_first_column_name(other)).alias("R"),
+ other.spark.column.alias("R"),
# for the right side, we only need the column that we are joining on
col(index_col),
)
+
joined_df = df.join(other_df, on=index_col, how="outer")
if default_val is not None:
# ps.Series.fillna() doesn't always work for the output for some reason
# so we manually handle the nulls here.
- select = f"""
+ spark_col = F.when(
+ F.col("L").isNull() | F.col("R").isNull(),
+ default_val,
+ ).otherwise(spark_col)
+ # The above is equivalent to the following:
+ f"""
CASE
WHEN `L` IS NULL OR `R` IS NULL THEN {default_val}
- ELSE {select}
+ ELSE {spark_col}
END
"""
Review Comment:
Remove commented out SQL code block. This multi-line comment containing old SQL logic should be deleted as it's no longer relevant after the refactoring.
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -2007,16 +2018,30 @@ def crosses(self, other, align=None) -> pspd.Series:
"""
# Sedona does not support GeometryCollection (errors), so we return NULL for now to avoid error
- select = """
+ """
CASE
WHEN GeometryType(`L`) == 'GEOMETRYCOLLECTION' OR GeometryType(`R`) == 'GEOMETRYCOLLECTION' THEN NULL
ELSE ST_Crosses(`L`, `R`)
END
"""
+ other_series, extended = self._make_series_of_val(other)
+ align = False if extended else align
+ spark_expr = F.when(
+ (stf.GeometryType(F.col("L")) == "GEOMETRYCOLLECTION")
+ | (stf.GeometryType(F.col("R")) == "GEOMETRYCOLLECTION"),
+ None,
+ ).otherwise(stp.ST_Crosses(F.col("L"), F.col("R")))
result = self._row_wise_operation(
- select, other, align, rename="crosses", default_val="FALSE"
+ spark_expr,
+ other_series,
+ align,
+ default_val=False,
)
+
+ # result = self._row_wise_operation(
+ # select, other, align, rename="crosses", default_val="FALSE"
+ # )
Review Comment:
Remove commented out code block. These commented lines showing the old implementation should be deleted to improve code cleanliness.
```suggestion
# (Commented-out code removed for cleanliness)
```
##########
python/sedona/geopandas/geodataframe.py:
##########
@@ -746,17 +752,19 @@ def rename_geometry(self, col: str, inplace: bool = False) -> GeoDataFrame | Non
if col in self.columns:
raise ValueError(f"Column named {col} already exists")
else:
+ mapper = {col: col for col in list(self.columns)}
+ mapper[geometry_col] = col
+
+ # mapper = {geometry_col: col}
Review Comment:
Remove commented out code. This commented line serves no purpose and should be deleted to improve code cleanliness.
```suggestion
```
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -2007,16 +2018,30 @@ def crosses(self, other, align=None) -> pspd.Series:
"""
# Sedona does not support GeometryCollection (errors), so we return NULL for now to avoid error
- select = """
+ """
CASE
WHEN GeometryType(`L`) == 'GEOMETRYCOLLECTION' OR GeometryType(`R`) == 'GEOMETRYCOLLECTION' THEN NULL
ELSE ST_Crosses(`L`, `R`)
END
"""
Review Comment:
Remove commented out SQL code block. This multi-line comment containing old SQL logic should be deleted as it's no longer relevant after the refactoring.
```suggestion
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #2131:
URL: https://github.com/apache/sedona/pull/2131#issuecomment-3100245400
@zhangfengcdt Any idea what this is about? Saw you made a change recently to these files. Thought it might be flaky? But I got the failure in my own fork CI as well, but another PR managed to pass.
```
FAILED tests/stac/test_collection_client.py::TestStacReader::test_get_items_with_temporal_extent - assert 5 == 16
+ where 5 = len([<Item id=AST_L1T_00312252006021551_20150517141010>, <Item id=AST_L1T_00312252006021542_20150517141000>, <Item id=AST_L1T_00312252006021533_20150517141000>, <Item id=AST_L1T_00312252006021524_20150517141010>, <Item id=AST_L1T_00312252006021515_20150517141000>])
FAILED tests/stac/test_collection_client.py::TestStacReader::test_get_items_with_tuple_datetime - assert 5 == 16
+ where 5 = len([<Item id=AST_L1T_00312252006021551_20150517141010>, <Item id=AST_L1T_00312252006021542_20150517141000>, <Item id=AST_L1T_00312252006021533_20150517141000>, <Item id=AST_L1T_00312252006021524_20150517141010>, <Item id=AST_L1T_00312252006021515_20150517141000>])
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2131:
URL: https://github.com/apache/sedona/pull/2131#discussion_r2223469738
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -969,16 +912,38 @@ def length(self) -> pspd.Series:
3 4.828427
dtype: float64
"""
- col = self.get_first_geometry_column()
- select = f"""
+
+ """
CASE
WHEN GeometryType(`{col}`) IN ('LINESTRING', 'MULTILINESTRING') THEN ST_Length(`{col}`)
WHEN GeometryType(`{col}`) IN ('POLYGON', 'MULTIPOLYGON') THEN ST_Perimeter(`{col}`)
WHEN GeometryType(`{col}`) IN ('POINT', 'MULTIPOINT') THEN 0.0
WHEN GeometryType(`{col}`) IN ('GEOMETRYCOLLECTION') THEN ST_Length(`{col}`) + ST_Perimeter(`{col}`)
- END"""
+ END"""
+
Review Comment:
I left this intentionally to serve as kinda like a more readable comment to the spark dataframe api
EDIT: Actually, sure let's delete all of these
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2131:
URL: https://github.com/apache/sedona/pull/2131#discussion_r2223469738
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -969,16 +912,38 @@ def length(self) -> pspd.Series:
3 4.828427
dtype: float64
"""
- col = self.get_first_geometry_column()
- select = f"""
+
+ """
CASE
WHEN GeometryType(`{col}`) IN ('LINESTRING', 'MULTILINESTRING') THEN ST_Length(`{col}`)
WHEN GeometryType(`{col}`) IN ('POLYGON', 'MULTIPOLYGON') THEN ST_Perimeter(`{col}`)
WHEN GeometryType(`{col}`) IN ('POINT', 'MULTIPOINT') THEN 0.0
WHEN GeometryType(`{col}`) IN ('GEOMETRYCOLLECTION') THEN ST_Length(`{col}`) + ST_Perimeter(`{col}`)
- END"""
+ END"""
+
Review Comment:
I left this intentionally to serve as kinda like a more readable comment to the spark dataframe api
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #2131:
URL: https://github.com/apache/sedona/pull/2131#issuecomment-3104242035
Will this affect multiple ongoing PRs? If so, should we merge this first?
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-2129] Geopandas: Refactor to use Spark Dataframe API instead of Spark SQL [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #2131:
URL: https://github.com/apache/sedona/pull/2131#issuecomment-3104623675
> Will this affect multiple ongoing PRs? If so, should we merge this first?
Yes I want to merge this first. All checks passing
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org