[SPARK-46167][PS] Add axis implementation to DataFrame.rank #54009
Conversation
Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com> Co-authored-by: Devin Petersohn <devin.petersohn@snowflake.com>
JIRA Issue Information=== Sub-task SPARK-46167 === This comment was automatically generated by GitHub Actions |
|
cc @gaogaotiantian FYI |
python/pyspark/pandas/frame.py
Outdated
| def rank( | ||
| self, method: str = "average", ascending: bool = True, numeric_only: bool = False | ||
| self, | ||
| method: str = "average", |
There was a problem hiding this comment.
Probably need to change to a Literal here for type hint.
| method: str = "average", | ||
| ascending: bool = True, | ||
| numeric_only: bool = False, | ||
| axis: Axis = 0, |
There was a problem hiding this comment.
We need to make a decision for where axis should be. pandas has it at the very beginning - we are doing a different thing, which means if the user is sending the argument positionally, we would have a different result. On the other hand, if they are doing that, moving axis to the beginning would break their existing code too.
On a side note, pandas is moving towards keyword-only APIs very eagerly. We could also consider doing that here to avoid user sending the wrong argument.
We are incompatible with pandas now - might be a good chance to fix that and hurt the users early.
There was a problem hiding this comment.
It's a good point, do we break compatibility with prior versions of Spark or pandas here? Moving to keyword only could break everyone, but most pandas code I've seen explicitly uses the keyword args. It makes sense to me to move to strict keyword support for this API and others moving forward to make the API more explicit (and Pythonic) anyway. Thoughts?
There was a problem hiding this comment.
I think the test is comprehensive. Took a quick look at the implementation - not the pandas/pyspark df expert but the pattern looks familiar.
The only thing I have is the API, which is brought up inline.
Signed-off-by: Devin Petersohn <devin.petersohn@gmail.com>
What changes were proposed in this pull request?
Adds axis implementation to DataFrame.rank
Why are the changes needed?
Implements missing API
Does this PR introduce any user-facing change?
Yes, new API support
How was this patch tested?
CI
Was this patch authored or co-authored using generative AI tooling?
Co-authored-by: Claude Sonnet 4.5