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

Deprecating with argument makes expr column selector less obvious #2826

Open
renkun-ken opened this issue May 3, 2018 · 3 comments
Open
Labels
programming parameterizing queries: get, mget, eval, env

Comments

@renkun-ken
Copy link
Member

renkun-ken commented May 3, 2018

Glad to see 1.11.0 version is just released. I noticed from NEWS that with= will be slowly deprecated in future versions (as thoroughly discussed in #2655). However, I find it less obvious for the following cases:

library(data.table)
dt <- data.table(x1 = 1:5, x2 = 5:1, y1 = 0:4, y2 = 4:0)
selector <- list(x = c("x1", "x2"), y = c("y1", "y2"))

To use selector$x or selector$y to select columns in dt, with argument is more obvious.

dt[, selector$x, with = FALSE]

But if with= is no longer available, it seems that we can only store selector$x in another symbol that allows direct access to do the same.

selector_x <- selector$x
dt[, ..selector_x]

Another similar case is calling a function to get the column names we need:

f <- function(prefix, n) {
  paste0(prefix, 1:n)
}
dt[, f("x", 2), with = FALSE]

I'm not sure if I'm missing something new in the latest release but these cases can be ugly without with argument. If .. has to be applied to these cases, it seems that all ..symbol is needed to be directed to symbol in the calling scope (#2589) so that something like dt[, ..selector$x] or dt[, ..f("x")] work.

@jangorecki
Copy link
Member

jangorecki commented May 3, 2018

Personally I would prefer keep with argument and set its default dynamically to something like

"[.data.table" = function (x, i, j, with=inspectj(j), ...) ...
# or
"[.data.table" = function (x, i, j, with=NA, ...) {...; if (is.na(with)) auto_detect else force_behavior; ...}
# where auto_detect has logic like `inspectj` but does not have to be separate function

Then user can always force with behavior if needed.

@mattdowle
Copy link
Member

mattdowle commented May 3, 2018

On first glance, there seems to be two bugs.

 dt[, ..selector$x]   
[1] "x1" "x2"

j= contains a symbol there ($x) so it's reasonable the result is as if with=TRUE.

dt[, ..selector[["x"]]]
[1] "x1" "x2"

That should work as if with=FALSE because j= contains no symbols. But doesn't. Bug 1 is that ..selector is considered a symbol there even though we know it explicitly isn't a column name due to the prefix.

This already works :

> dt[, paste0("x", 1:2)]
      x1    x2
   <int> <int>
1:     1     5
2:     2     4
3:     3     3
4:     4     2
5:     5     1
> 

But this doesn't :

> dt[, f("x", 2)]
[1] "x1" "x2"

I remember some special casing for paste0 and similar in the internals, which isn't clean. There's an argument to all.vars to exclude function symbols, so turning that on should clean it up (bug 2).

Let's try that first anyway, demo the new fixed behaviour, and then see if you're ok with it.

@mattdowle mattdowle added this to the 1.11.4 milestone May 3, 2018
@jangorecki jangorecki changed the title Deprecating with argument makes expr column selector less obvious Deprecating with argument makes expr column selector less obvious May 10, 2018
@mattdowle mattdowle modified the milestones: 1.12.0, 1.12.2 Jan 11, 2019
@jangorecki jangorecki modified the milestones: 1.12.2, 1.12.4 Jan 24, 2019
@jangorecki jangorecki modified the milestones: 1.12.4, 1.13.0 Jul 25, 2019
@jangorecki
Copy link
Member

.. / with mentioned in https://nathaneastwood.github.io/2019/08/18/no-visible-binding-for-global-variable/
+1 for not deprecating with but changing default into with=guess_with(j)

@mattdowle mattdowle modified the milestones: 1.12.7, 1.12.9 Dec 8, 2019
@jangorecki jangorecki added the programming parameterizing queries: get, mget, eval, env label Apr 5, 2020
@mattdowle mattdowle modified the milestones: 1.13.1, 1.13.3 Oct 17, 2020
@mattdowle mattdowle removed this from the 1.14.1 milestone Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
programming parameterizing queries: get, mget, eval, env
Projects
None yet
Development

No branches or pull requests

3 participants