-
Notifications
You must be signed in to change notification settings - Fork 888
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
Reorganize cudf_polars
expression code
#17014
base: branch-24.12
Are you sure you want to change the base?
Reorganize cudf_polars
expression code
#17014
Conversation
from polars.polars import _expr_nodes as pl_expr | ||
|
||
from cudf_polars.containers import Column, NamedColumn | ||
from cudf_polars.utils import dtypes, sorting | ||
from cudf_polars.containers import Column |
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.
Do we want to keep this file at all?
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.
Additionally a few of these didn't seem to have an obvious home, maybe a misc.py
?
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 it is useful to have everything importable from one place. So yes, I think it makes sense to keep this file.
I think it would be nice if all this file did was to import the expression names into the dsl.expr
namespace.
So moving the remainder makes sense to me.
How about Cast
into unary.py
, Ternary
into ternary.py
and BooleanFunction
into boolean.py
?
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.
Thanks @brandon-b-miller! This looks like a good step to me. Some small further suggestions.
from polars.polars import _expr_nodes as pl_expr | ||
|
||
from cudf_polars.containers import Column, NamedColumn | ||
from cudf_polars.utils import dtypes, sorting | ||
from cudf_polars.containers import Column |
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 it is useful to have everything importable from one place. So yes, I think it makes sense to keep this file.
I think it would be nice if all this file did was to import the expression names into the dsl.expr
namespace.
So moving the remainder makes sense to me.
How about Cast
into unary.py
, Ternary
into ternary.py
and BooleanFunction
into boolean.py
?
from collections.abc import Mapping | ||
|
||
from cudf_polars.containers import DataFrame | ||
|
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.
nit: throughout, can we advertise __all__
?
This PR seeks to break up
expr.py
into a less unwieldy monolith.