You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "itholic (via GitHub)" <gi...@apache.org> on 2023/06/08 11:45:16 UTC
[GitHub] [spark] itholic opened a new pull request, #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
itholic opened a new pull request, #41514:
URL: https://github.com/apache/spark/pull/41514
### What changes were proposed in this pull request?
This PR proposes to fix `NullOps.(eq|ne)` and `NumOps.(eq|ne)` for pandas API on Spark with Spark Connect.
This includes SPARK-43684, SPARK-43685, SPARK-43686, SPARK-43691 at once, because they are all related similar modifications in single file.
### Why are the changes needed?
To support all features for pandas API on Spark with Spark Connect.
### Does this PR introduce _any_ user-facing change?
Yes, NullOps.eq, NullOps.ne, NumOps.eq, NumOps.ne are now working as expected on Spark Connect.
### How was this patch tested?
Uncomment the UTs, tested manually for vanilla PySpark.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227412988
##########
python/pyspark/pandas/data_type_ops/base.py:
##########
@@ -219,6 +219,10 @@ def _is_boolean_type(right: Any) -> bool:
)
+def _is_extension_dtypes(object: Any) -> bool:
Review Comment:
Yeah, basically this function just simply checks whether the given `object` is an extension dtype or not.
Looking at the changes in the `num_ops.py`, it is necessary to handle binary operations differently when either the `left` or `right` is an extension dtype. If this util function doesn't exist, the code:
```python
if isinstance(getattr(left, "dtype", None), extension_dtypes) or isinstance(getattr(right, "dtype", None), extension_dtypes):
...
```
would have to be added to every function.
So I added this utility function to simplify the code because I believe that adding such code to every function would compromise the readability of code base.
Of course, we can just check manually for all functions instead of adding new util. I have no strong preference for this, though.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
URL: https://github.com/apache/spark/pull/41514
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1223797787
##########
python/pyspark/pandas/data_type_ops/null_ops.py:
##########
@@ -43,6 +43,22 @@ class NullOps(DataTypeOps):
def pretty_name(self) -> str:
return "nulls"
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ result = pyspark_column_op("__eq__")(left, right)
+ if is_remote():
+ # TODO(SPARK-43877): Fix behavior difference for compare binary functions.
+ result = result.fillna(False)
+ return result
+
+ def ne(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ result = pyspark_column_op("__ne__")(left, right)
+ if is_remote():
Review Comment:
Sounds good. Let me address
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1228833815
##########
python/pyspark/pandas/data_type_ops/num_ops.py:
##########
@@ -213,37 +214,31 @@ def abs(self, operand: IndexOpsLike) -> IndexOpsLike:
F.abs(operand.spark.column), field=operand._internal.data_fields[0]
)
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ # We can directly use `super().eq` when given object is list, tuple, dict or set.
+ if not isinstance(right, IndexOpsMixin) and is_list_like(right):
+ return super().eq(left, right)
+ return pyspark_column_op("__eq__", left, right, fillna=False)
+
+ def ne(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ return pyspark_column_op("__ne__", left, right, fillna=True)
Review Comment:
For `ne`:
```python
>>> pser = pd.Series([1.0, 2.0, np.nan])
>>> psser = ps.from_pandas(pser)
>>> pser.ne(pser)
0 False
1 False
2 True
dtype: bool
>>> psser.ne(psser)
0 False
1 False
2 None
dtype: bool
```
We expect `True` for non-equal case, but it returns `None` in Spark Connect. So we cast `None` to `True` for `ne`.
For `eq`:
```python
>>> pser = pd.Series([1.0, 2.0, np.nan])
>>> psser = ps.from_pandas(pser)
>>> pser.eq(pser)
0 True
1 True
2 False
dtype: bool
>>> psser.eq(psser)
0 True
1 True
2 None
dtype: bool
```
We expect `False` for non-equal case, but it returns `None` in Spark Connect. So we cast `None` to `False` for `eq`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227737630
##########
python/pyspark/pandas/data_type_ops/num_ops.py:
##########
@@ -213,37 +215,43 @@ def abs(self, operand: IndexOpsLike) -> IndexOpsLike:
F.abs(operand.spark.column), field=operand._internal.data_fields[0]
)
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ # We can directly use `super().eq` when given object is list, tuple, dict or set.
+ if not isinstance(right, IndexOpsMixin) and is_list_like(right):
+ return super().eq(left, right)
+ if _is_extension_dtypes(left) or _is_extension_dtypes(right):
+ return pyspark_column_op("__eq__", left, right)
+ return pyspark_column_op("__eq__", left, right, fillna=False)
+
+ def ne(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ if _is_extension_dtypes(left) or _is_extension_dtypes(right):
+ return pyspark_column_op("__ne__", left, right)
+ return pyspark_column_op("__ne__", left, right, fillna=True)
+
def lt(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
_sanitize_list_like(right)
- result = pyspark_column_op("__lt__")(left, right)
- if is_remote():
- # TODO(SPARK-43877): Fix behavior difference for compare binary functions.
- result = result.fillna(False)
- return result
+ if _is_extension_dtypes(left) or _is_extension_dtypes(right):
+ return pyspark_column_op("__lt__", left, right)
+ return pyspark_column_op("__lt__", left, right, fillna=False)
Review Comment:
I think you can put `_is_extension_dtypes(left) or _is_extension_dtypes(right)` condition to `fillna`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227973177
##########
python/pyspark/sql/utils.py:
##########
@@ -237,17 +237,25 @@ def wrapped(*args: Any, **kwargs: Any) -> Any:
return cast(FuncT, wrapped)
-def pyspark_column_op(func_name: str) -> Callable[..., "SeriesOrIndex"]:
+def pyspark_column_op(
+ func_name: str, left: "IndexOpsLike", right: Any, fillna: Any = None
+) -> Union["SeriesOrIndex", None]:
"""
Wrapper function for column_op to get proper Column class.
"""
from pyspark.pandas.base import column_op
from pyspark.sql.column import Column as PySparkColumn
+ from pyspark.pandas.data_type_ops.base import _is_extension_dtypes
if is_remote():
from pyspark.sql.connect.column import Column as ConnectColumn
Column = ConnectColumn
else:
Column = PySparkColumn # type: ignore[assignment]
- return column_op(getattr(Column, func_name))
+ result = column_op(getattr(Column, func_name))(left, right)
+ # It works as expected on extension dtype, so we don't need to call `fillna` for this case.
Review Comment:
What's "works as expected"?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [WIP][SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227505892
##########
python/pyspark/pandas/data_type_ops/num_ops.py:
##########
@@ -213,37 +214,44 @@ def abs(self, operand: IndexOpsLike) -> IndexOpsLike:
F.abs(operand.spark.column), field=operand._internal.data_fields[0]
)
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ try:
+ _sanitize_list_like(right)
+ except TypeError:
Review Comment:
Because currently `eq` only support for list-like object properly.
Added some comments and test for `NullOps` as well.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227974019
##########
python/pyspark/pandas/data_type_ops/num_ops.py:
##########
@@ -213,37 +214,31 @@ def abs(self, operand: IndexOpsLike) -> IndexOpsLike:
F.abs(operand.spark.column), field=operand._internal.data_fields[0]
)
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ # We can directly use `super().eq` when given object is list, tuple, dict or set.
+ if not isinstance(right, IndexOpsMixin) and is_list_like(right):
+ return super().eq(left, right)
+ return pyspark_column_op("__eq__", left, right, fillna=False)
+
+ def ne(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ return pyspark_column_op("__ne__", left, right, fillna=True)
Review Comment:
Why `fillna=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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227412988
##########
python/pyspark/pandas/data_type_ops/base.py:
##########
@@ -219,6 +219,10 @@ def _is_boolean_type(right: Any) -> bool:
)
+def _is_extension_dtypes(object: Any) -> bool:
Review Comment:
Yeah, basically this function just simply checks whether the given `object` is an extension dtype or not.
Looking at the changes in the `num_ops.py`, it is necessary to handle binary operations differently when either the `left` or `right` is an extension dtype. If this util function doesn't exist, the code:
```python
if isinstance(getattr(left, "dtype", None), extension_dtypes) or isinstance(getattr(right, "dtype", None), extension_dtypes):
...
```
would have to be added to every function.
So I added this utility function to simplify the code because I believe that adding such code to every function would compromise the code's readability.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1223720225
##########
python/pyspark/pandas/data_type_ops/null_ops.py:
##########
@@ -43,6 +43,22 @@ class NullOps(DataTypeOps):
def pretty_name(self) -> str:
return "nulls"
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
Review Comment:
Yeah, this PR already include the same issues in batch. There is `CategoricalOps.eq` is left as a similar operation, but we need further investigation since it has different issues.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1222955048
##########
python/pyspark/pandas/data_type_ops/null_ops.py:
##########
@@ -43,6 +43,22 @@ class NullOps(DataTypeOps):
def pretty_name(self) -> str:
return "nulls"
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
Review Comment:
@itholic are there a lot left? Can we fix them in batch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1223797237
##########
python/pyspark/pandas/data_type_ops/null_ops.py:
##########
@@ -43,6 +43,22 @@ class NullOps(DataTypeOps):
def pretty_name(self) -> str:
return "nulls"
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ result = pyspark_column_op("__eq__")(left, right)
+ if is_remote():
+ # TODO(SPARK-43877): Fix behavior difference for compare binary functions.
+ result = result.fillna(False)
+ return result
+
+ def ne(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ result = pyspark_column_op("__ne__")(left, right)
+ if is_remote():
Review Comment:
Can you put this into `pyspark_column_op`, and add a parameter to deduplicate the codes? e.g., `fillna=value`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1228837088
##########
python/pyspark/sql/utils.py:
##########
@@ -237,17 +237,25 @@ def wrapped(*args: Any, **kwargs: Any) -> Any:
return cast(FuncT, wrapped)
-def pyspark_column_op(func_name: str) -> Callable[..., "SeriesOrIndex"]:
+def pyspark_column_op(
+ func_name: str, left: "IndexOpsLike", right: Any, fillna: Any = None
+) -> Union["SeriesOrIndex", None]:
"""
Wrapper function for column_op to get proper Column class.
"""
from pyspark.pandas.base import column_op
from pyspark.sql.column import Column as PySparkColumn
+ from pyspark.pandas.data_type_ops.base import _is_extension_dtypes
if is_remote():
from pyspark.sql.connect.column import Column as ConnectColumn
Column = ConnectColumn
else:
Column = PySparkColumn # type: ignore[assignment]
- return column_op(getattr(Column, func_name))
+ result = column_op(getattr(Column, func_name))(left, right)
+ # It works as expected on extension dtype, so we don't need to call `fillna` for this case.
Review Comment:
I means that it works the same as pandas without any additional computation such as `fillna` for extension dtype as below:
```python
>>> pser = pd.Series([1.0, 2.0, np.nan], dtype="Float64")
>>> psser = ps.from_pandas(pser)
>>> pser.eq(pser)
0 True
1 True
2 <NA>
dtype: boolean
>>> psser.eq(psser)
0 True
1 True
2 <NA>
dtype: boolean
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227777781
##########
python/pyspark/pandas/data_type_ops/num_ops.py:
##########
@@ -213,37 +215,43 @@ def abs(self, operand: IndexOpsLike) -> IndexOpsLike:
F.abs(operand.spark.column), field=operand._internal.data_fields[0]
)
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ # We can directly use `super().eq` when given object is list, tuple, dict or set.
+ if not isinstance(right, IndexOpsMixin) and is_list_like(right):
+ return super().eq(left, right)
+ if _is_extension_dtypes(left) or _is_extension_dtypes(right):
+ return pyspark_column_op("__eq__", left, right)
+ return pyspark_column_op("__eq__", left, right, fillna=False)
+
+ def ne(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ if _is_extension_dtypes(left) or _is_extension_dtypes(right):
+ return pyspark_column_op("__ne__", left, right)
+ return pyspark_column_op("__ne__", left, right, fillna=True)
+
def lt(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
_sanitize_list_like(right)
- result = pyspark_column_op("__lt__")(left, right)
- if is_remote():
- # TODO(SPARK-43877): Fix behavior difference for compare binary functions.
- result = result.fillna(False)
- return result
+ if _is_extension_dtypes(left) or _is_extension_dtypes(right):
+ return pyspark_column_op("__lt__", left, right)
+ return pyspark_column_op("__lt__", left, right, fillna=False)
Review Comment:
Sounds good. Just moved the logic into util.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41514:
URL: https://github.com/apache/spark/pull/41514#issuecomment-1590254841
Merged to master.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1228833815
##########
python/pyspark/pandas/data_type_ops/num_ops.py:
##########
@@ -213,37 +214,31 @@ def abs(self, operand: IndexOpsLike) -> IndexOpsLike:
F.abs(operand.spark.column), field=operand._internal.data_fields[0]
)
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ # We can directly use `super().eq` when given object is list, tuple, dict or set.
+ if not isinstance(right, IndexOpsMixin) and is_list_like(right):
+ return super().eq(left, right)
+ return pyspark_column_op("__eq__", left, right, fillna=False)
+
+ def ne(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ _sanitize_list_like(right)
+ return pyspark_column_op("__ne__", left, right, fillna=True)
Review Comment:
**For `ne`:**
```python
>>> pser = pd.Series([1.0, 2.0, np.nan])
>>> psser = ps.from_pandas(pser)
>>> pser.ne(pser)
0 False
1 False
2 True
dtype: bool
>>> psser.ne(psser)
0 False
1 False
2 None
dtype: bool
```
We expect `True` for non-equal case, but it returns `None` in Spark Connect. So we cast `None` to `True` for `ne`.
**For `eq`:**
```python
>>> pser = pd.Series([1.0, 2.0, np.nan])
>>> psser = ps.from_pandas(pser)
>>> pser.eq(pser)
0 True
1 True
2 False
dtype: bool
>>> psser.eq(psser)
0 True
1 True
2 None
dtype: bool
```
We expect `False` for non-equal case, but it returns `None` in Spark Connect. So we cast `None` to `False` for `eq`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227418655
##########
python/pyspark/pandas/data_type_ops/num_ops.py:
##########
@@ -213,37 +214,44 @@ def abs(self, operand: IndexOpsLike) -> IndexOpsLike:
F.abs(operand.spark.column), field=operand._internal.data_fields[0]
)
+ def eq(self, left: IndexOpsLike, right: Any) -> SeriesOrIndex:
+ try:
+ _sanitize_list_like(right)
+ except TypeError:
Review Comment:
Can you update your PR description, add a bit of docstring in `_is_extension_dtypes`? I can't follow why we need try-except here but not in `ne`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #41514: [SPARK-43684][SPARK-43685][SPARK-43686][SPARK-43691][CONNECT][PS] Fix `(NullOps|NumOps).(eq|ne)` for Spark Connect.
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41514:
URL: https://github.com/apache/spark/pull/41514#discussion_r1227363769
##########
python/pyspark/pandas/data_type_ops/base.py:
##########
@@ -219,6 +219,10 @@ def _is_boolean_type(right: Any) -> bool:
)
+def _is_extension_dtypes(object: Any) -> bool:
Review Comment:
can you explain the fix please?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org