Skip to content
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

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Conversation

milenkovicm
Copy link
Contributor

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

@github-actions github-actions bot added the core Core DataFusion crate label Sep 18, 2024
Copy link
Contributor

@alamb alamb left a 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?

@Omega359
Copy link
Contributor

This is an interesting addition! I'll take a look at it today.

@milenkovicm
Copy link
Contributor Author

thanks lads!
btw, if anybody knows why parse_sql_expr strips alias would be of the great help :)

@timsaucer
Copy link
Contributor

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(())
Copy link
Contributor

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

Copy link
Contributor Author

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)

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #12527

Copy link
Contributor

@comphead comphead left a 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

datafusion/core/src/dataframe/mod.rs Outdated Show resolved Hide resolved
@comphead comphead merged commit b1bdd8d into apache:main Sep 19, 2024
24 checks passed
@comphead
Copy link
Contributor

Thanks everyone

@milenkovicm milenkovicm deleted the sql_expression branch September 19, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame select_exprs
5 participants