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/08/02 02:14:54 UTC
[GitHub] [spark] bzhaoopenstack opened a new pull request, #37367: [SPARK-39941][PYTHON][PS] period and min_periods should be integer in rolling func
bzhaoopenstack opened a new pull request, #37367:
URL: https://github.com/apache/spark/pull/37367
window and min_periods parameters is not be validated in rolling function.
### What changes were proposed in this pull request?
Validate the said 2 parameters to be a integer only in rolling func, will raise ValueError if they are not Integer
### Why are the changes needed?
I guess we miss the validation towards those parameters.
PySpark will raise Error during using other type inputs
```
>>> s = ps.Series([4, 3, 5, 2, 6])
>>> s.rolling(1).sum()
0 4
1 3
2 5
3 2
4 6
dtype: int64
>>> s.rolling('STRING').sum()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/spark/spark/python/pyspark/pandas/generic.py", line 2707, in rolling
return Rolling(self, window=window, min_periods=min_periods)
File "/home/spark/spark/python/pyspark/pandas/window.py", line 179, in __init__
super().__init__(window, min_periods)
File "/home/spark/spark/python/pyspark/pandas/window.py", line 147, in __init__
if window < 0:
TypeError: '<' not supported between instances of 'str' and 'int'
>>>
```
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Input other types of window and min_periods in rolling func will raise ValueError
--
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] xinrong-meng commented on a diff in pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
xinrong-meng commented on code in PR #37367:
URL: https://github.com/apache/spark/pull/37367#discussion_r947083497
##########
python/pyspark/pandas/window.py:
##########
@@ -144,10 +144,15 @@ def __init__(
window: int,
min_periods: Optional[int] = None,
):
+ if type(window) is not int:
+ raise ValueError("window must be an integer")
Review Comment:
Thanks @itholic.
If so, shall we check negative `window` 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 pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #37367:
URL: https://github.com/apache/spark/pull/37367#issuecomment-1201972549
cc @zhengruifeng @xinrong-meng @itholic 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] github-actions[bot] commented on pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #37367:
URL: https://github.com/apache/spark/pull/37367#issuecomment-1341790133
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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] github-actions[bot] closed pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
URL: https://github.com/apache/spark/pull/37367
--
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 #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #37367:
URL: https://github.com/apache/spark/pull/37367#discussion_r956856890
##########
python/pyspark/pandas/window.py:
##########
@@ -144,10 +144,15 @@ def __init__(
window: int,
min_periods: Optional[int] = None,
):
+ if type(window) is not int:
+ raise ValueError("window must be an integer")
Review Comment:
> If so, shall we check negative window altogether (and remove the window < 0 below)?
Sounds good!
--
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] xinrong-meng commented on a diff in pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
xinrong-meng commented on code in PR #37367:
URL: https://github.com/apache/spark/pull/37367#discussion_r947083497
##########
python/pyspark/pandas/window.py:
##########
@@ -144,10 +144,15 @@ def __init__(
window: int,
min_periods: Optional[int] = None,
):
+ if type(window) is not int:
+ raise ValueError("window must be an integer")
Review Comment:
Thanks @itholic.
If so, shall we check negative `window` altogether (and remove the `window < 0` below)?
--
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] xinrong-meng commented on a diff in pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
xinrong-meng commented on code in PR #37367:
URL: https://github.com/apache/spark/pull/37367#discussion_r947085356
##########
python/pyspark/pandas/window.py:
##########
@@ -144,10 +144,15 @@ def __init__(
window: int,
min_periods: Optional[int] = None,
):
+ if type(window) is not int:
+ raise ValueError("window must be an integer")
if window < 0:
raise ValueError("window must be >= 0")
- if (min_periods is not None) and (min_periods < 0):
- raise ValueError("min_periods must be >= 0")
+ if min_periods is not None:
+ if type(min_periods) is not int:
Review Comment:
nit: `isinstance`?
--
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] AmplabJenkins commented on pull request #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #37367:
URL: https://github.com/apache/spark/pull/37367#issuecomment-1203205040
Can one of the admins verify this patch?
--
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 #37367: [SPARK-39941][PYTHON][PS] window and min_periods should be integer in rolling func
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #37367:
URL: https://github.com/apache/spark/pull/37367#discussion_r946645052
##########
python/pyspark/pandas/window.py:
##########
@@ -144,10 +144,15 @@ def __init__(
window: int,
min_periods: Optional[int] = None,
):
+ if type(window) is not int:
Review Comment:
Can we use `isinstance` instead ?
```python
if not isinstance(window, int):
...
```
##########
python/pyspark/pandas/window.py:
##########
@@ -144,10 +144,15 @@ def __init__(
window: int,
min_periods: Optional[int] = None,
):
+ if type(window) is not int:
+ raise ValueError("window must be an integer")
Review Comment:
Can we follow the error message same as pandas ?
```python
>>> ps.range(10).to_pandas().rolling(window=1.1)
Traceback (most recent call last):
...
ValueError: window must be an integer 0 or greater
```
--
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