-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix(python): DataFrame descending sorting by single list element #19233
fix(python): DataFrame descending sorting by single list element #19233
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #19233 +/- ##
==========================================
- Coverage 80.00% 79.96% -0.04%
==========================================
Files 1527 1529 +2
Lines 209203 209820 +617
Branches 2415 2416 +1
==========================================
+ Hits 167371 167783 +412
- Misses 41284 41488 +204
- Partials 548 549 +1 ☔ View full report in Codecov by Sentry. |
py-polars/polars/lazyframe/frame.py
Outdated
@@ -1370,6 +1370,8 @@ def sort( | |||
""" | |||
# Fast path for sorting by a single existing column | |||
if isinstance(by, str) and not more_by: | |||
if isinstance(descending, list): | |||
descending = descending[0] |
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.
We should check and raise an error if the list length is not 1
I am not sure here. This will be a huge combinatorial nightmare checking this for the whole API. If you pass a single So it's either all lists or all scalars. |
Can't we fix this in some central location? |
The way I see it, there are two branches in the body of the func and signature does not convey which permutations are allowed and which not. |
py-polars/polars/lazyframe/frame.py
Outdated
@@ -1370,6 +1371,8 @@ def sort( | |||
""" | |||
# Fast path for sorting by a single existing column | |||
if isinstance(by, str) and not more_by: |
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.
I think we should just not hit the fast path if the user passes a list:
if isinstance(by, str) and isintance(descending, bool) and not more_by:
py-polars/polars/_utils/various.py
Outdated
@@ -629,3 +629,10 @@ def re_escape(s: str) -> str: | |||
# escapes _only_ those metachars with meaning to the rust regex crate | |||
re_rust_metachars = r"\\?()|\[\]{}^$#&~.+*-" | |||
return re.sub(f"([{re_rust_metachars}])", r"\\\1", s) | |||
|
|||
|
|||
def try_head(seq: Sequence[Any] | Any, default: Any) -> Any: |
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.
This isn't per se correct as the lenght is not checked.
fixed |
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.
I think the title should be fix(python): ...
When calling sort with single
str
column and list of singledescending
parameter it fails with error:It checks for types and avoids path that does not use list.