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

refactor: remove RPolarsProtoExprArray and rewrite <Expr>$over() #984

Merged
merged 4 commits into from
Mar 30, 2024

Conversation

etiennebacher
Copy link
Collaborator

It's called only 3 times on the R side. I think we now have other mechanisms to replace it.

This PR also introduces mapping_strategy for $over() but it's mostly because it was needed to compile. I'll add tests, examples, and a bullet point in NEWS in a followup PR.

R/expr__expr.R Outdated
Comment on lines 1843 to 1848
Expr_over = function(expr, ..., mapping_strategy = "group_to_rows") {
expr = c(
wrap_elist_result(expr, str_to_lit = FALSE) |>
unwrap("in $over():"),
list2(...)
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not very happy about this but I couldn't find an alternative that works with all our tests.

This comment was marked as outdated.

Copy link
Collaborator

@eitsupi eitsupi Mar 30, 2024

Choose a reason for hiding this comment

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

I rewrote this in 88c2f46
How about that?

R side:

  list_of_exprs = list2(...) |>
    lapply(\(x) {
      if (is.character(x)) {
        as.list(x)
      } else {
        x
      }
    }) |>
    unlist(recursive = FALSE) |>
    lapply(\(x) {
      if (is.character(x)) {
        pl$col(x)
      } else {
        x
      }
    })

Rust side:

robj_to!(Vec, PLExpr, list_of_exprs)?

It is similar to pl$col(), but differs in that it converts vectors of length 2 or greater to [col("a"), col("b")], whereas pl$col() converts them as cols(["a", "b"]).

This would be the equivalent of parse_as_list_of_expressions in Python, so it would be worth rewriting the following sections and others using this.

pl_concat_list = function(exprs) {
concat_list(as.list(exprs)) |>
unwrap(" in pl$concat_list():")
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about that?

This seems good since it passes tests but it departs a bit from the python version which has expr + .... This was the reason why I had implemented it like that:

  expr = c(
    wrap_elist_result(expr, str_to_lit = FALSE) |>
      unwrap("in $over():"),
    list2(...)
  )

Do you think that's a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is similar to pl$col() before rewriting it in #923.
In R, I think it is more natural to handle it in variable length arguments ....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is similar to pl$col() before rewriting it in #923.

Indeed.

Thanks for completing this PR!

@etiennebacher etiennebacher changed the title refactor: remove RProtoExprArray refactor: remove RPolarsProtoExprArray Mar 29, 2024
@eitsupi eitsupi changed the title refactor: remove RPolarsProtoExprArray refactor: remove RPolarsProtoExprArray and rewrite <Expr>$over() Mar 30, 2024
Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Good catch, thanks!

@etiennebacher etiennebacher merged commit b3cb553 into main Mar 30, 2024
5 checks passed
@etiennebacher etiennebacher deleted the remove-protoexpr-array branch March 30, 2024 09:30
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.

None yet

2 participants