-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: Support pl.all().filter(..)
#18993
Conversation
) | ||
|
||
q = df.lazy().select(pl.all().filter(~pl.col("p"))) | ||
assert "__POLARS_CSER" in q.explain() |
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.
CSE is important for this PR so that we don't evaluate the predicate more than once
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18993 +/- ##
==========================================
- Coverage 79.88% 79.87% -0.01%
==========================================
Files 1524 1524
Lines 207634 207631 -3
Branches 2904 2904
==========================================
- Hits 165873 165850 -23
- Misses 41214 41234 +20
Partials 547 547 ☔ View full report in Codecov by Sentry. |
@@ -1150,9 +1150,6 @@ impl Expr { | |||
/// Should be used in aggregation context. If you want to filter on a | |||
/// DataFrame level, use `LazyFrame::filter`. | |||
pub fn filter<E: Into<Expr>>(self, predicate: E) -> Self { |
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 this in dsl
conversion and either raise an error or translate to a Filter
node.
e3635c8
to
0703188
Compare
input, | ||
options, | ||
} if { | ||
// This is a hack so that we don't break `list.eval(pl.element().filter())`. That expression |
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.
Auch, we really need to replace that element => col("")
hack. With a proper Expr::Element
type and something that can run it.
But probably not this PR.
Fixes #18733
This removes the restriction, but we could also raise a
PolarsError
instead if we want