devin-petersohn commented on code in PR #58254:
URL: https://github.com/apache/spark/pull/58254#discussion_r3868022557


##########
python/pyspark/pandas/frame.py:
##########
@@ -11756,12 +11756,13 @@ def _result_aggregated(
         # dtype: bool
         return first_series(DataFrame(internal))
 
-    # TODO(SPARK-46167): add pct, na_option parameter
     def rank(
         self,
         method: Literal["average", "min", "max", "first", "dense"] = "average",
         ascending: bool = True,
         numeric_only: bool = False,
+        na_option: Literal["keep", "top", "bottom"] = "keep",
+        pct: bool = False,

Review Comment:
   The reason we put them at the end was in case users are using positional 
notation for their args. This won't be fully backwards compatible if we put the 
new args in the middle.



##########
python/pyspark/pandas/frame.py:
##########
@@ -11756,12 +11756,13 @@ def _result_aggregated(
         # dtype: bool
         return first_series(DataFrame(internal))
 
-    # TODO(SPARK-46167): add pct, na_option parameter
     def rank(
         self,
         method: Literal["average", "min", "max", "first", "dense"] = "average",
         ascending: bool = True,
         numeric_only: bool = False,
+        na_option: Literal["keep", "top", "bottom"] = "keep",
+        pct: bool = False,

Review Comment:
   In the past we've put new parameters at the end.
   
   Another option is to require them as kwargs with the `*` notation.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to