select() updates order_var
#1106
Conversation
| lf %>% window_order(desc(y)) %>% select(y2 = y) %>% op_sort(), | ||
| list(expr(desc(y2))) | ||
| ) | ||
| # keeps sort order |
There was a problem hiding this comment.
I think the order of variables shouldn't be touched by select()
| test_that("select handles order vars", { | ||
| lf <- lazy_frame(x = 1, y = 1, z = 1) | ||
| # can drop order vars | ||
| expect_equal(lf %>% window_order(y) %>% select(-y) %>% op_sort(), list()) |
There was a problem hiding this comment.
This is different to the handling of grouping variables but keeping them is probably unwanted and unexpected. Also, this would otherwise probably be a big breaking change.
|
@hadley You're now happy with the PR and only forgot to approve? Or do you need to take a closer look? 😉 |
|
|
||
| desc <- purrr::map_lgl(order, ~ is_call(.x, "desc", n = 1L)) | ||
| order <- purrr::map_if(order, desc, ~ call_args(.x)[[1L]]) | ||
| order <- purrr::map_chr(order, as_name) |
There was a problem hiding this comment.
Shouldn't check_window_order_dots() ensure that this is already a symbol?
There was a problem hiding this comment.
I don't quite follow. check_window_order_dots() ensures that each element of order is either a symbol or desc(<symbol>). And these two steps extract the underlying symbol and convert it to a string so that it can be mapped more easily.
There was a problem hiding this comment.
When is an element of order not already going to be a name?
There was a problem hiding this comment.
After line 175 every element of order is a symbol. Then as_name() converts the symbols to strings.
There was a problem hiding this comment.
Oooooh, I was confused by as_name() being very different to as.name()
There was a problem hiding this comment.
I just looked into the documentation
rlang::as_name()is the opposite ofbase::as.name()
which is indeed quite mean 😄
| window_order({{order_by}}) %>% | ||
| filter(!!window_fun) %>% | ||
| # must use `add_order()` as `window_order()` only allows variables | ||
| .data$lazy_query <- add_order(.data, quos({{order_by}})) |
There was a problem hiding this comment.
Does any existing test cover this? What happens if there's a select after this step?
There was a problem hiding this comment.
The previous window order is restored below and there is a test for this. A select() after this step would drop the window order in case of slice_sample() and keep the window order for the others (unless the window order variables are de-selected).
Fixes #1103.
I had to slightly adapt my initial proposal to allow symbols and
desc(<symbol>). Note that this is only enforced inwindow_order()but not inadd_order(). Without this the query ofslice_sample()would have been quite a bit more complicated. It is a bit of a hack but asadd_order()is an internal function this should be fine.