-
Notifications
You must be signed in to change notification settings - Fork 1.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
Expose DataFrame select_exprs method #12520
Conversation
5065c9e
to
6235c9c
Compare
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.
Thank you @milenkovicm
@timsaucer / @Omega359 as you are more familar with DataFrame APIs would you possibly have a chance to review this PR to make sure the API is consistent?
This is an interesting addition! I'll take a look at it today. |
thanks lads! |
The API looks reasonable to me and this is a very nice add. One suggestion on the unit test. |
/// let ctx = SessionContext::new(); | ||
/// let df = ctx.read_csv("tests/data/example.csv", CsvReadOptions::new()).await?; | ||
/// let df = df.select_exprs(&["a * b", "c"])?; | ||
/// # Ok(()) |
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.
why do we have commented out parts of example? 🤔
and I found it would be really nice to have an expected answer in example. Like what exactly select_exprs
returns
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.
not sure what do you mean @comphead, I just copied doc from select_columns
and adapt it for this interface (description, method name and args)
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.
alright, its probably what we can do next. If one read the select_expr
its great to have a usage but would be nice to see what the method returns
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 get you, will change
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.
Filed #12527
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.
lgtm thanks @milenkovicm
The expected answers we can as a followup
6235c9c
to
e85a6c6
Compare
Thanks everyone |
Which issue does this PR close?
Closes #12519.
Rationale for this change
Explained in #12519
What changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
yes, there is additional public method in
DataFrame
No braking changes