-
-
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: prevent panic with arg_sort_by
#15247
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15247 +/- ##
=======================================
Coverage ? 81.32%
=======================================
Files ? 1359
Lines ? 176086
Branches ? 2524
=======================================
Hits ? 143204
Misses ? 32399
Partials ? 483 ☔ View full report in Codecov by Sentry. |
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 shouldn't panic, but I think it should just work rather than throw an error.
Also, can you add the example you gave in the PR description as a test in the Python test suite?
My understanding is that if something like >>> import polars as pl
>>> pl.arg_sort_by("*")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/workspaces/polars/py-polars/polars/functions/lazy.py", line 1601, in arg_sort_by
return wrap_expr(plr.arg_sort_by(exprs, descending))
polars.exceptions.ComputeError: cannot determine output column without a context for this expression |
73a3e7d
to
9122f91
Compare
Ok( | ||
int_range(lit(0 as IdxSize), len().cast(IDX_DTYPE), 1, IDX_DTYPE) | ||
.sort_by(by, descending) | ||
.alias(name.as_ref()), |
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.
Can you try keep_name
here? That might work.
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 tried name().keep()
, but `keep`, `suffix`, `prefix` should be last expression
error occurred in Python tests.
9122f91
to
04fa3cf
Compare
CodSpeed Performance ReportMerging #15247 will not alter performanceComparing Summary
|
From pola-rs/r-polars#929
Correct panic when multiple column names are specified as the first argument of
arg_sort_by
.In Python, current release version: