Introduce NamedColumn concept in cudf-polars
#15914
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Python
wence-
added
improvement
wence-
commented
Jun 4, 2024
| class NamedExpr(Expr): | ||
| __slots__ = ("name", "children") | ||
| _non_child = ("dtype", "name") | ||
| class NamedExpr: |
There was a problem hiding this comment.
Decided to deliberately not make this one an Expr (it should not appear when evaluating expressions themselves, only when constructing return values in dataframe (IR) nodes)
There was a problem hiding this comment.
This could be helpful to leave as a comment.
Suggested change
| class NamedExpr: | |
| class NamedExpr: | |
| # NamedExpr does not inherit from Expr because it should not appear when | |
| # evaluating expressions themselves, only when constructing return values | |
| # in dataframe (IR) nodes). |
mroeschke
reviewed
Jun 5, 2024
wence-
commented
Jun 5, 2024
Names in the result dataframe only appear from PyExprIR and thence NamedExpr nodes. To avoid name tracking issues, only require a name when translating a NamedExpr.
Expressions must now be translated with the node which is to provide the schema active.
wence-
force-pushed
the
wence/fea/expunge-scalar-dataframe
branch
from
769b248 to
9b87759
Compare
We can't decide expression-by-expression whether the result should be broadcast to the size of the context DataFrame. It is only when we return "out" to construct a new DataFrame (i.e. when we are evaluating an IR node) that we have the necessary information.
lithomas1
approved these changes
Jun 5, 2024
mroeschke
approved these changes
Jun 5, 2024
|
This looks good (to the best of my knowledge). Maybe @vyasr or @brandon-b-miller should double check this though. |
bdice
approved these changes
Jun 5, 2024
| class NamedExpr(Expr): | ||
| __slots__ = ("name", "children") | ||
| _non_child = ("dtype", "name") | ||
| class NamedExpr: |
There was a problem hiding this comment.
This could be helpful to leave as a comment.
Suggested change
| class NamedExpr: | |
| class NamedExpr: | |
| # NamedExpr does not inherit from Expr because it should not appear when | |
| # evaluating expressions themselves, only when constructing return values | |
| # in dataframe (IR) nodes). |
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Simplify name tracking in expression evaluation by only requiring names for columns when putting them in to a
DataFrame. At the same time, this allows us to have one place where we broadcast-expandScalars to the size of theDataFrame, so we can expunge tracking them in theDataFrameitself.Additionally, adapt to minor changes on the polars side in terms of translating the DSL: we no longer need to handle CSE expressions specially, and sorting by multiple keys takes a list of
descendingflags, rather than a single bool as previously.Checklist