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/31 19:18:01 UTC
[PR] [GH-2208] Geopandas: Fix sjoin and implement rest of parameters [sedona]
petern48 opened a new pull request, #2209:
URL: https://github.com/apache/sedona/pull/2209
## 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 #2208
## What changes were proposed in this PR?
Fix sjoin and implement rest of parameters
## How was this patch tested?
Refactored tests
## Did this PR include necessary documentation updates?
- Yes, I am adding a new API. I am using the [current SNAPSHOT version number](https://github.com/apache/sedona/blob/99239524f17389fc4ae9548ea88756f8ea538bb9/pom.xml#L29) in `vX.Y.Z` format.
- Yes, I have updated the documentation.
- 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-2208] Geopandas: Fix sjoin implementation + proper naming and index behavior [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #2209:
URL: https://github.com/apache/sedona/pull/2209
--
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-2208] Geopandas: Fix sjoin implementation + proper naming and index behavior [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2209:
URL: https://github.com/apache/sedona/pull/2209#discussion_r2249369029
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -465,30 +465,21 @@ def crs(self) -> Union["CRS", None]:
if len(self) == 0:
return None
- if parse_version(pyspark.__version__) >= parse_version("3.5.0"):
- spark_col = stf.ST_SRID(F.first_value(self.spark.column, ignoreNulls=True))
- # Set this to avoid error complaining that we don't have a groupby column
- is_aggr = True
- else:
- spark_col = stf.ST_SRID(self.spark.column)
- is_aggr = False
+ # F.first is non-deterministic, but it doesn't matter because all non-null values should be the same
+ spark_col = stf.ST_SRID(F.first(self.spark.column, ignorenulls=True))
Review Comment:
The parameter name should be `ignoreNulls` (camelCase) not `ignorenulls` (lowercase). This is the correct parameter name for Spark SQL functions.
```suggestion
spark_col = stf.ST_SRID(F.first(self.spark.column, ignoreNulls=True))
```
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -91,30 +89,21 @@ def _frame_join(
right_geom_col = None
# Find geometry columns in left dataframe
- for field in left_sdf.schema.fields:
- if field.dataType.typeName() in ("geometrytype", "binary"):
- left_geom_col = field.name
- break
+ left_geom_col = left_df.active_geometry_name
# Find geometry columns in right dataframe
- for field in right_sdf.schema.fields:
- if field.dataType.typeName() in ("geometrytype", "binary"):
- right_geom_col = field.name
- break
+ right_geom_col = right_df.active_geometry_name
+
+ if not left_geom_col:
+ raise ValueError("Left dataframe geometry column not set")
+ if not right_geom_col:
+ raise ValueError("Right dataframe geometry column not set")
if left_geom_col is None or right_geom_col is None:
raise ValueError("Both datasets must have geometry columns")
Review Comment:
This check is redundant since the previous checks on lines 97-100 already validate the geometry columns. Consider removing the duplicate validation or consolidate all geometry column validation into a single check.
```suggestion
```
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -91,30 +89,21 @@ def _frame_join(
right_geom_col = None
# Find geometry columns in left dataframe
- for field in left_sdf.schema.fields:
- if field.dataType.typeName() in ("geometrytype", "binary"):
- left_geom_col = field.name
- break
+ left_geom_col = left_df.active_geometry_name
# Find geometry columns in right dataframe
- for field in right_sdf.schema.fields:
- if field.dataType.typeName() in ("geometrytype", "binary"):
- right_geom_col = field.name
- break
+ right_geom_col = right_df.active_geometry_name
+
+ if not left_geom_col:
Review Comment:
This check will never be True since `left_geom_col` is assigned from `left_df.active_geometry_name` on line 92, and the subsequent check on line 102 already handles the case where `left_geom_col is None`. The check should be `if left_geom_col is None:` to be consistent.
```suggestion
if left_geom_col is None:
```
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -91,30 +89,21 @@ def _frame_join(
right_geom_col = None
# Find geometry columns in left dataframe
- for field in left_sdf.schema.fields:
- if field.dataType.typeName() in ("geometrytype", "binary"):
- left_geom_col = field.name
- break
+ left_geom_col = left_df.active_geometry_name
# Find geometry columns in right dataframe
- for field in right_sdf.schema.fields:
- if field.dataType.typeName() in ("geometrytype", "binary"):
- right_geom_col = field.name
- break
+ right_geom_col = right_df.active_geometry_name
+
+ if not left_geom_col:
+ raise ValueError("Left dataframe geometry column not set")
+ if not right_geom_col:
Review Comment:
This check will never be True since `right_geom_col` is assigned from `right_df.active_geometry_name` on line 95, and the subsequent check on line 102 already handles the case where `right_geom_col is None`. The check should be `if right_geom_col is None:` to be consistent.
```suggestion
if right_geom_col is None:
```
--
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-2208] Geopandas: Fix sjoin implementation + proper naming and index behavior [sedona]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on code in PR #2209:
URL: https://github.com/apache/sedona/pull/2209#discussion_r2252053060
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -195,36 +203,35 @@ def _frame_join(
final_columns.append(f"{col_name} as {base_name}")
# Select final columns
- result_df = spatial_join_df.selectExpr(*final_columns)
-
- # Return appropriate type based on input
- if isinstance(left_df, GeoSeries) and isinstance(right_df, GeoSeries):
- # Return GeoSeries for GeoSeries inputs
- internal = InternalFrame(
- spark_frame=result_df,
- index_spark_columns=None,
- column_labels=[left_df._col_label],
- data_spark_columns=[scol_for(result_df, "geometry")],
- data_fields=[left_df._internal.data_fields[0]],
- column_label_names=left_df._internal.column_label_names,
- )
- return _to_geo_series(first_series(ps.DataFrame(internal)))
- else:
- # Return GeoDataFrame for GeoDataFrame inputs
- return GeoDataFrame(result_df)
+ result_df = spatial_join_df.selectExpr(*final_columns).orderBy(
+ SPARK_DEFAULT_INDEX_NAME
+ )
Review Comment:
Yeah, maintaining the same order after a join probably isn't very important for a lot of cases. Looks like PySpark Pandas also doesn't guarantee sort order "not preserve key order unlike pandas" [according to their docs](https://spark.apache.org/docs/latest/api/python/reference/pyspark.pandas/api/pyspark.pandas.DataFrame.merge.html). I removed the orderBy and updated the docs accordingly, mentioning they can sort after using a method like `sort_index()`.
--
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-2208] Geopandas: Fix sjoin implementation + proper naming and index behavior [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2209:
URL: https://github.com/apache/sedona/pull/2209#discussion_r2249637599
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -195,36 +203,35 @@ def _frame_join(
final_columns.append(f"{col_name} as {base_name}")
# Select final columns
- result_df = spatial_join_df.selectExpr(*final_columns)
-
- # Return appropriate type based on input
- if isinstance(left_df, GeoSeries) and isinstance(right_df, GeoSeries):
- # Return GeoSeries for GeoSeries inputs
- internal = InternalFrame(
- spark_frame=result_df,
- index_spark_columns=None,
- column_labels=[left_df._col_label],
- data_spark_columns=[scol_for(result_df, "geometry")],
- data_fields=[left_df._internal.data_fields[0]],
- column_label_names=left_df._internal.column_label_names,
- )
- return _to_geo_series(first_series(ps.DataFrame(internal)))
- else:
- # Return GeoDataFrame for GeoDataFrame inputs
- return GeoDataFrame(result_df)
+ result_df = spatial_join_df.selectExpr(*final_columns).orderBy(
+ SPARK_DEFAULT_INDEX_NAME
+ )
Review Comment:
This is a good comment. We also noticed that in the demo this week, although you were using 144 cores, the distributed mode was only 10X faster than GeoPandas. Is it because we pay extra cost on maintaining the sorted order?
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -195,36 +203,35 @@ def _frame_join(
final_columns.append(f"{col_name} as {base_name}")
# Select final columns
- result_df = spatial_join_df.selectExpr(*final_columns)
-
- # Return appropriate type based on input
- if isinstance(left_df, GeoSeries) and isinstance(right_df, GeoSeries):
- # Return GeoSeries for GeoSeries inputs
- internal = InternalFrame(
- spark_frame=result_df,
- index_spark_columns=None,
- column_labels=[left_df._col_label],
- data_spark_columns=[scol_for(result_df, "geometry")],
- data_fields=[left_df._internal.data_fields[0]],
- column_label_names=left_df._internal.column_label_names,
- )
- return _to_geo_series(first_series(ps.DataFrame(internal)))
- else:
- # Return GeoDataFrame for GeoDataFrame inputs
- return GeoDataFrame(result_df)
+ result_df = spatial_join_df.selectExpr(*final_columns).orderBy(
+ SPARK_DEFAULT_INDEX_NAME
+ )
Review Comment:
@petern48
--
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-2208] Geopandas: Fix sjoin implementation + proper naming and index behavior [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2209:
URL: https://github.com/apache/sedona/pull/2209#discussion_r2252375654
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -196,35 +206,33 @@ def _frame_join(
# Select final columns
result_df = spatial_join_df.selectExpr(*final_columns)
+ # Note, we do not .orderBy(SPARK_DEFAULT_INDEX_NAME) to avoid a performance hit
Review Comment:
This comment should be more descriptive about the trade-off. Consider explaining that this means the result order may not match geopandas exactly, and users should call .sort_index() if order preservation is needed.
```suggestion
# Note: we do not call .orderBy(SPARK_DEFAULT_INDEX_NAME) here to avoid a performance hit.
# As a result, the order of the returned rows may not match the order produced by geopandas.
# If you require the result to preserve the original order, call .sort_index() on the output.
```
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -465,30 +465,21 @@ def crs(self) -> Union["CRS", None]:
if len(self) == 0:
return None
- if parse_version(pyspark.__version__) >= parse_version("3.5.0"):
- spark_col = stf.ST_SRID(F.first_value(self.spark.column, ignoreNulls=True))
- # Set this to avoid error complaining that we don't have a groupby column
- is_aggr = True
- else:
- spark_col = stf.ST_SRID(self.spark.column)
- is_aggr = False
+ # F.first is non-deterministic, but it doesn't matter because all non-null values should be the same
+ spark_col = stf.ST_SRID(F.first(self.spark.column, ignorenulls=True))
Review Comment:
The parameter name should be 'ignoreNulls' (camelCase) not 'ignorenulls' (lowercase). This follows Spark's naming convention for function parameters.
```suggestion
spark_col = stf.ST_SRID(F.first(self.spark.column, ignoreNulls=True))
```
--
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-2208] Geopandas: Fix sjoin implementation + proper naming and index behavior [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2209:
URL: https://github.com/apache/sedona/pull/2209#discussion_r2249634624
##########
python/sedona/geopandas/geoseries.py:
##########
@@ -465,30 +465,21 @@ def crs(self) -> Union["CRS", None]:
if len(self) == 0:
return None
- if parse_version(pyspark.__version__) >= parse_version("3.5.0"):
- spark_col = stf.ST_SRID(F.first_value(self.spark.column, ignoreNulls=True))
- # Set this to avoid error complaining that we don't have a groupby column
- is_aggr = True
- else:
- spark_col = stf.ST_SRID(self.spark.column)
- is_aggr = False
+ # F.first is non-deterministic, but it doesn't matter because all non-null values should be the same
+ spark_col = stf.ST_SRID(F.first(self.spark.column, ignorenulls=True))
Review Comment:
The parameter name should be `ignoreNulls` (camel case) instead of `ignorenulls` (lowercase) to match Spark SQL function naming conventions.
```suggestion
spark_col = stf.ST_SRID(F.first(self.spark.column, ignoreNulls=True))
```
##########
python/sedona/geopandas/tools/sjoin.py:
##########
@@ -195,36 +203,35 @@ def _frame_join(
final_columns.append(f"{col_name} as {base_name}")
# Select final columns
- result_df = spatial_join_df.selectExpr(*final_columns)
-
- # Return appropriate type based on input
- if isinstance(left_df, GeoSeries) and isinstance(right_df, GeoSeries):
- # Return GeoSeries for GeoSeries inputs
- internal = InternalFrame(
- spark_frame=result_df,
- index_spark_columns=None,
- column_labels=[left_df._col_label],
- data_spark_columns=[scol_for(result_df, "geometry")],
- data_fields=[left_df._internal.data_fields[0]],
- column_label_names=left_df._internal.column_label_names,
- )
- return _to_geo_series(first_series(ps.DataFrame(internal)))
- else:
- # Return GeoDataFrame for GeoDataFrame inputs
- return GeoDataFrame(result_df)
+ result_df = spatial_join_df.selectExpr(*final_columns).orderBy(
+ SPARK_DEFAULT_INDEX_NAME
+ )
Review Comment:
Adding an `orderBy` operation on the entire result could be expensive for large datasets. Consider if this ordering is necessary or if it can be optimized for better performance in distributed environments.
```suggestion
result_df = spatial_join_df.selectExpr(*final_columns)
```
--
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