-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SNOW-1458130 Implement Index.sort_values
#1901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty clean. Thanks for adding descriptive comments.
@@ -159,7 +159,8 @@ Methods | |||
+-----------------------------+---------------------------------+----------------------------------+----------------------------------------------------+ | |||
| ``searchsorted`` | N | | | | |||
+-----------------------------+---------------------------------+----------------------------------+----------------------------------------------------+ | |||
| ``sort_values`` | N | | | | |||
| ``sort_values`` | P | key | Snowpark pandas currently uses stable sort when | | |||
| | | | sorting the index values. Pandas uses quicksort. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this comment also apply to Series.sort_values
and DataFrame.sort_values
? If so, then we should update their docs too. It can be in a separate PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! Should apply there as well, I'll make a separate PR for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks, Varnika.
Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-1458130
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Added support for
Index.sort_values
. Currently, thekey
parameter is not supported in Snowpark pandas.One peculiar feature about
Index.sort_values
is the option to return an indexer (return_indexer
) - this is a numpy array with the old row numbers/positions in their sorted positions.I reused the
sort_index
implementation in the query compiler since it's the exact same logic. I added a new parameterinclude_indexer
to retrieve the indexer used to sort the Index values. It is import to note here that pandas uses quicksort (which Snowpark pandas does not support yet) while Snowpark pandas uses stable sort. Therefore, it is not guaranteed that the indexer returned by Snowpark pandas is going to be the same as the one returned by native pandas.Examples: