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

Change $drop() input from columns to ... #914

Merged
merged 4 commits into from
Mar 12, 2024
Merged

Conversation

etiennebacher
Copy link
Collaborator

Close #913

@etiennebacher etiennebacher requested a review from eitsupi March 11, 2024 17:23
NEWS.md Outdated
@@ -2,6 +2,11 @@

## Polars R Package (development version)

### New features

- `$drop()` now accepts several character vectors, such as `$drop("a", "b")`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect this is a potentially breaking change.
In other words, if you specify it with the columns argument instead of the position argument, won't unexpected behavior occur?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Actually I think there's a bug in dots_to_colnames() because passing c() in second values doesn't work:

### this works
pl$DataFrame(mtcars)$drop(c("mpg", "drat"), "hp") |> ncol()
[1] 8

### this doesn't
pl$DataFrame(mtcars)$drop("hp", c("mpg", "drat")) |> ncol()

Error: Execution halted with the following contexts
   0: In R: in $drop():
   0: During function call [ncol(pl$DataFrame(mtcars)$drop("hp", c("mpg", "drat")))]
   1: When constructing a Column Expr
   2: The argument [ `...` ] caused an error
   3: Possibly because element no. [1] 
   4: Expected a value of type [alloc::string::String]
   5: Got value [Rvalue: ["mpg", "drat"], Rsexp: Strings, Rclass: ["character"]]

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a problem of pl$col(), not dots_to_colnames().

> library(polars)

> pl$col(c("mpg", "drat"), "hp")
polars Expr: cols(["mpg", "drat", "hp"])

> pl$col("hp", c("mpg", "drat"))
Error: Execution halted with the following contexts
   0: In R: in pl$col()
   0: During function call [pl$col("hp", c("mpg", "drat"))]
   1: When constructing a Column Expr
   2: The argument [ `...` ] caused an error
   3: Possibly because element no. [1] 
   4: Expected a value of type [alloc::string::String]
   5: Got value [Rvalue: ["mpg", "drat"], Rsexp: Strings, Rclass: ["character"]]

> pl$col("hp", name = c("mpg", "drat"))
polars Expr: cols(["mpg", "drat", "hp"])

> pl$col("hp", columns = c("mpg", "drat"))
Error: Execution halted with the following contexts
   0: In R: in pl$col()
   0: During function call [pl$col("hp", columns = c("mpg", "drat"))]
   1: When constructing a Column Expr
   2: The argument [ `...` ] caused an error
   3: Possibly because element no. [1] 
   4: Expected a value of type [alloc::string::String]
   5: Got value [Rvalue: ["mpg", "drat"], Rsexp: Strings, Rclass: ["character"]]

We should rewrite pl$col()...(#912)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like pl$DataFrame(mtcars)$drop(columns = c("mpg", "drat")) works in the main branch, but should fail in this PR because of pl$col(name = "", ...).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I've updated news, thanks

@etiennebacher etiennebacher merged commit 6ff3c4b into main Mar 12, 2024
@etiennebacher etiennebacher deleted the change-input-drop branch March 12, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update $drop() input
2 participants