`transform` is ~100x slower on data.table than on data.frame

[OfekShilon]

# Minimal reproducible example

> df <- data.frame(x = runif(n = 1e7))
> dt <- as.data.table(df)
> system.time(df <- transform(df, y = round(x)))
   user  system elapsed 
  0.122   0.031   0.152 
> system.time(dt <- transform(dt, y = round(x)))
   user  system elapsed 
 19.658   0.157  19.808 

# Output of sessionInfo()
Observed on multiple configurations, the particular one I'm reporting from is with data.table from github trunk:

R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Ubuntu 20.04.5 LTS

Matrix products: default
BLAS: /usr/lib/x86_64-linux-gnu/blas/libblas.so.3.9.0
LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.9.0

locale:
[1] LC_CTYPE=C.UTF-8 LC_NUMERIC=C LC_TIME=C.UTF-8 LC_COLLATE=C.UTF-8 LC_MONETARY=C.UTF-8
[6] LC_MESSAGES=C.UTF-8 LC_PAPER=C.UTF-8 LC_NAME=C LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=C.UTF-8 LC_IDENTIFICATION=C

attached base packages:
[1] stats graphics grDevices utils datasets methods base

other attached packages:
[1] data.table_1.14.5

loaded via a namespace (and not attached):
[1] compiler_3.6.3 tools_3.6.3

[OfekShilon]

I thought PRs with the bug# in their name are automatically assigned to it, for some reason this one didn't: #5493

[ColeMiller1]

Thanks for the findings! I can reproduce on R 4.2 / data.table 1.14.5.

To have the PR link automatically, start the PR with

Closes {Issue Number}

I assume you can edit the PR and GitHub will figure it out but let me know if not.

[MichaelChirico]

I reproduce, but only on the first iteration:

> system.time(dt <- transform(dt, y = round(x)))
   user  system elapsed 
 19.632   0.280  20.007 
> system.time(dt <- transform(dt, y = round(x)))
   user  system elapsed 
  0.225   0.523   0.754 
> system.time(dt <- transform(dt, y = round(x)))
   user  system elapsed 
  0.281   0.096   0.378 
> system.time(dt <- transform(dt, y = round(x)))
   user  system elapsed 
  0.282   0.000   0.287 
> system.time(dt <- transform(dt, y = round(x)))
   user  system elapsed 
  0.281   0.000   0.284 
> system.time(dt <- transform(dt, y = round(x)))
   user  system elapsed 
  0.275   0.000   0.278 

Makes it seem like something else is going on here... after the first iteration, y is already a column, so it's an overwrite instead of a new column.

not a fan of the deparse() solution because inevitably there will be some entry where nlines=1 is wrong.

[MichaelChirico]

The implementation of transform.data.table looks odd to me:

data.table/R/data.table.R

Lines 2290 to 2313 in 0895fa2

	transform.data.table = function (`_data`, ...)
	# basically transform.data.frame with data.table instead of data.frame, and retains key
	{
	if (!cedta()) return(NextMethod()) # nocov
	e = eval(substitute(list(...)), `_data`, parent.frame())
	tags = names(e)
	inx = chmatch(tags, names(`_data`))
	matched = !is.na(inx)
	if (any(matched)) {
	.Call(C_unlock, `_data`) # fix for #1641, now covered by test 104.2
	`_data`[,inx[matched]] = e[matched]
	`_data` = as.data.table(`_data`)
	}
	if (!all(matched)) {
	ans = do.call("data.table", c(list(`_data`), e[!matched]))
	} else {
	ans = `_data`
	}
	key.cols = key(`_data`)
	if (!any(tags %chin% key.cols)) {
	setattr(ans, "sorted", key.cols)
	}
	ans
	}

This line in particular does not look idiomatic at all:

data.table/R/data.table.R

Line 2304 in 0895fa2

ans = do.call("data.table", c(list(`_data`), e[!matched]))

I would have expected under the hood we transform it to a data.table::set() call, essentially.

# pseudocode
transform.data.table <- function(`_data`, ...) {
  `_data` = copy(`_data`)
  `_data`[, names(list(...)) := list(...)]
  # set keys... then
  `_data`
}

so that would be what I'd pursue in fixing this

[jangorecki]

Moreover, if possible, better to use as.data.table() rather than data.table()

[ColeMiller1]

I did some trials and this seems to work. Good call by Michael - I completely forgot we could copy data. I think Ofek's suggestion for the naming utility is still good, too. Anytime we're deparsing to determine a name, a multi-line name would not make sense.

transform.data.table = function (`_data`, ...)
# basically transform.data.frame with data.table instead of data.frame, and retains key
{
  if (!cedta()) return(NextMethod()) # nocov
  `_data` = copy(`_data`)
  e = eval(substitute(list(...)), `_data`, parent.frame()) 
  set(`_data`, ,names(e), e)
  `_data`
}

Note the dt[, names(e) := e] method works, too, but it needs to tack on [] to print.

[OfekShilon]

I modified the PR to match the suggestion by @ColeMiller1 and @jangorecki (call as.data.table instead of data.table), and the added copy suggested by @MichaelChirico . Also added NEWS item and modified transform.data.table.Rd to less harsh wording on performance.

I did leave nlines=1 in place, as I think (like @ColeMiller1) it would be beneficial in all code paths that still pass through this deparse. If anyone has a concrete example to the contrary - I'll gladly revert it.

[OfekShilon]

To have the PR link automatically, start the PR with

Closes {Issue Number}

I assume you can edit the PR and GitHub will figure it out but let me know if not.

Thanks! I edited the PR title, but learnt from github docs that the auto-assigning of PRs to bugs works if the PR is on "the default branch". This PR is on a dedicated branch, as many projects consider this a good practice - does DT have any preference for PR branches?

[OfekShilon]

@MichaelChirico @ColeMiller1 The copy at the beginning of your suggested implementation makes it still ~twice as slow as data.frame (in my tested data sizes). Why is it needed?

[jangorecki]

That's a good question. We use set() so copy seems to be good option, but we don't subassign, just replace whole columns. I think we could solve it with shallow copy here, no?

[OfekShilon]

Actually this might be a broader discussion - is transform intended to work on its argument by reference?

shallow copy can create a limbo state where the input and output data.tables have some columns in common, so subassigning to them in the output will change the input dt but subassigning other columns will not.

I think it's best to respect transform.data.frame (and the pre-existing transform.data.table) semantics and modify the argument dt by value, i.e. create a new copy. So I'm answering my own question..

[jangorecki]

if it would be working by reference then would have to be called settransform