Skip to content
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-1320309: Remove unnecessary warning from sort_values #2306

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

sfc-gh-nkumar
Copy link
Contributor

Remove warning from sort_values when sort algorithm is 'quicksort'. We still raise warning if user passed a sort algorithm explicitly like mergesort for quicksort is default value so it doesn't make sense to raise this warning.

@sfc-gh-joshi
Copy link
Contributor

Do we have any tests for this warning behavior?

@sfc-gh-nkumar sfc-gh-nkumar force-pushed the nkumar-SNOW-1320309-quicksort branch from 9204047 to 2f713e5 Compare September 18, 2024 22:56
@sfc-gh-nkumar sfc-gh-nkumar added the NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs label Sep 18, 2024
@sfc-gh-nkumar sfc-gh-nkumar force-pushed the nkumar-SNOW-1320309-quicksort branch from 2f713e5 to f7c96ee Compare September 19, 2024 18:03
Copy link
Collaborator

@sfc-gh-evandenberg sfc-gh-evandenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -15,6 +15,7 @@
- Improved `to_pandas` to persist the original timezone offset for TIMESTAMP_TZ type.
- Improved `dtype` results for TIMESTAMP_TZ type to show correct timezone offset.
- Improved error message when passing non-bool value to `numeric_only` for groupby aggregations.
- Removed unnecessary warning about sort algorithm in `sort_values`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems so minor, is it even worth calling out as an improvement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it is pretty minor. But it does have user facing impact (right now this warning shows up in notebook in red) so might be good include this here.

@sfc-gh-nkumar sfc-gh-nkumar merged commit 5087ce5 into main Sep 19, 2024
35 checks passed
@sfc-gh-nkumar sfc-gh-nkumar deleted the nkumar-SNOW-1320309-quicksort branch September 19, 2024 18:49
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NO-PANDAS-CHANGEDOC-UPDATES This PR does not update Snowpark pandas docs snowpark-pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants