You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/01/11 13:31:46 UTC
[GitHub] [spark] Yikun opened a new pull request #35167: Add eager_check for LocIndexer._select_cols_by_iterable
Yikun opened a new pull request #35167:
URL: https://github.com/apache/spark/pull/35167
### What changes were proposed in this pull request?
Skip equal check in LocIndexer._select_cols_by_iterable if eager_check disable
### Why are the changes needed?
To improve the performance if eager_check disable.
### Does this PR introduce _any_ user-facing change?
Yes, with eager_check disable, didn't raise exception in nan case.
### How was this patch tested?
UT
--
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] Yikun commented on a change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782675144
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
Ah, yes, and there are some differet behaviour on pandas and pandas on spark.
I hit this problem because original case pdf[[True, Fasle, True]], in pandas is rowselect, but for pandas on spark [is colselect](https://github.com/apache/spark/blob/master/python/pyspark/pandas/indexing.py#L805-L806). So colselect it's okay in here maybe.
Should I do a next commits to fix above comments or maybe just close this PR?
--
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] Yikun closed pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
Yikun closed pull request #35167:
URL: https://github.com/apache/spark/pull/35167
--
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 change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782670983
##########
File path: python/docs/source/user_guide/pandas_on_spark/options.rst
##########
@@ -290,7 +290,8 @@ compute.eager_check True 'compute.eager_check' se
`Series.asof`, `Series.compare`,
`FractionalExtensionOps.astype`,
`IntegralExtensionOps.astype`,
- `FractionalOps.astype`, `DecimalOps.astype`.
+ `FractionalOps.astype`, `DecimalOps.astype`,
+ `LocIndexer._select_cols_by_iterable`.
Review comment:
I think maybe we should explain it instead of saying `LocIndexer._select_cols_by_iterable`. e.g.) `psdf.loc[:, iterable]`
--
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] Yikun commented on a change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782675144
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
Ah, yes, and there are some differet behaviour on pandas and pandas on spark.
I hit this problem because pdf[[True, Fasle, True]], in pandas is rowselect, but for pandas on spark [is colselect](https://github.com/apache/spark/blob/master/python/pyspark/pandas/indexing.py#L805-L806). So colselect it's okay in here maybe.
Should I do a next commits to fix above comments or maybe just close this.
--
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 change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782672775
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
Actually, I think this is fine @Yikun. `self._psdf.columns` returns a pandas index. So sort_values on the column labels should be not so expensive in general.
--
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] Yikun commented on pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35167:
URL: https://github.com/apache/spark/pull/35167#issuecomment-1011847609
Anyway, let's close this PR 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: 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] Yikun commented on a change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782675144
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
Ah, yes, and there are some differet behaviour on pandas and pandas on spark.
I hit this problem because original case pdf[[True, Fasle, True]], in pandas is rowselect, but for pandas on spark [is colselect](https://github.com/apache/spark/blob/master/python/pyspark/pandas/indexing.py#L805-L806). So colselect it's okay in here maybe.
Should I do a next commits to fix above comments or maybe just close this.
--
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 change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782697894
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
Oh I see .. we should probably fix that case then.. maybe let's create a new PR to address that problem first, and then get this one merged after that.
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
cc @ueshin FYI
--
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] Yikun commented on a change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782675144
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
Ah, yes, and there are some differet behaviour on pandas and pandas on spark.
I hit this problem because original case pdf[[True, Fasle, True]], in pandas is rowselect, but for pandas on spark [is colselect](https://github.com/apache/spark/blob/master/python/pyspark/pandas/indexing.py#L805-L806). So colselect it's okay in here maybe.
Should I do a next commits to fix doc to address above comments or maybe just close this PR?
--
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] Yikun commented on a change in pull request #35167: [SPARK-37868][PYTHON] Add eager_check for LocIndexer._select_cols_by_iterable
Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35167:
URL: https://github.com/apache/spark/pull/35167#discussion_r782675144
##########
File path: python/pyspark/pandas/indexing.py
##########
@@ -1311,7 +1312,9 @@ def _select_cols_by_iterable(
% (len(cast(Sized, cols_sel)), len(self._internal.column_labels))
)
if isinstance(cols_sel, pd.Series):
- if not cols_sel.index.sort_values().equals(self._psdf.columns.sort_values()):
+ if get_option("compute.eager_check") and not cols_sel.index.sort_values().equals(
+ self._psdf.columns.sort_values()
Review comment:
Ah, yes, colselect it's okay in here.
and there are some differet behaviour on pandas and pandas on spark. I hitted this problem because original case pdf[[True, Fasle, True]], in pandas is rowselect, but for pandas on spark [is colselect](https://github.com/apache/spark/blob/master/python/pyspark/pandas/indexing.py#L805-L806). I didn't realize it before.
Should I do a next commits to fix doc to address above comments or maybe just close this PR?
--
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