Ensure Consistent Key Handling in as.data.table S3 Methods (#6865)

* fixing key setting order

* fix key setting order

* document argument keep.rownames and key

* add tests and news.md

* add rownames and key in .Rd

* Extract keep.rownames and key from ...

* eliminate %>% operator

* update test 2309.2

* update tests

* set key

* update key handling

* update test

* corrected last test

* correct test case

* correct 2309.04

* update fix

* update changes

* Update R/as.data.table.R

* Update R/as.data.table.R

* update tests

* correct signature of as.data.table.data.table

---------

Co-authored-by: Benjamin Schwendinger <[email protected]>

Author: Mukulyadav2004

NEWS.md

@@ -14,6 +14,8 @@
 
 3. `fread(keepLeadingZeros=TRUE)` now correctly parses dates with components with leading zeros as dates instead of strings, [#6851](https://github.com/Rdatatable/data.table/issues/6851). Thanks @TurnaevEvgeny for the report and @ben-schwen for the fix.
 
+4. `as.data.table()` now properly handles keys: specifying keys sets them, omitting keys preserves existing ones, and setting `key=NULL` clears them, [#6859](https://github.com/Rdatatable/data.table/issues/6859). Thanks @brookslogan for the report and @Mukulyadav2004 for the fix.
+
 ## NOTES
 
 1. Continued work to remove non-API C functions, [#6180](https://github.com/Rdatatable/data.table/issues/6180). Thanks Ivan Krylov for the PRs and for writing a clear and concise guide about the R API: https://aitap.codeberg.page/R-api/.

R/as.data.table.R

@@ -214,8 +214,8 @@ as.data.table.list = function(x,
 }
 
 as.data.table.data.frame = function(x, keep.rownames=FALSE, key=NULL, ...) {
-  if (is.data.table(x)) return(as.data.table.data.table(x)) # S3 is weird, #6739. Also # nocov; this is tested in 2302.{2,3}, not sure why it doesn't show up in coverage.
-  if (!identical(class(x), "data.frame")) return(as.data.table(as.data.frame(x)))
+  if (is.data.table(x)) return(as.data.table.data.table(x, key=key)) # S3 is weird, #6739. Also # nocov; this is tested in 2302.{2,3}, not sure why it doesn't show up in coverage.
+  if (!identical(class(x), "data.frame")) return(as.data.table(as.data.frame(x), keep.rownames=keep.rownames, key=key, ...))
   if (!isFALSE(keep.rownames)) {
     # can specify col name to keep.rownames, #575; if it's the same as key,
     #   kludge it to 'rn' since we only apply the new name afterwards, #4468
@@ -228,7 +228,7 @@ as.data.table.data.frame = function(x, keep.rownames=FALSE, key=NULL, ...) {
   if (any(cols_with_dims(x))) {
     # a data.frame with a column that is data.frame needs to be expanded; test 2013.4
     # x may be a class with [[ method that behaves differently, so as.list first for default [[, #4526
-    return(as.data.table.list(as.list(x), keep.rownames=keep.rownames, ...))
+    return(as.data.table.list(as.list(x), keep.rownames=keep.rownames, key = key,...))
   }
   ans = copy(x)  # TO DO: change this deep copy to be shallow.
   setattr(ans, "row.names", .set_row_names(nrow(x)))
@@ -245,13 +245,14 @@ as.data.table.data.frame = function(x, keep.rownames=FALSE, key=NULL, ...) {
   ans
 }
 
-as.data.table.data.table = function(x, ...) {
+as.data.table.data.table = function(x, ..., key=NULL) {
   # as.data.table always returns a copy, automatically takes care of #473
   if (any(cols_with_dims(x))) { # for test 2089.2
-    return(as.data.table.list(x, ...))
+    return(as.data.table.list(x, key = key, ...))
   }
   x = copy(x) # #1681
   # fix for #1078 and #1128, see .resetclass() for explanation.
   setattr(x, 'class', .resetclass(x, "data.table"))
+  if (!missing(key)) setkeyv(x, key)
   x
 }

inst/tests/tests.Rraw

@@ -21084,3 +21084,23 @@ test(2307, { capture.output(print(DT, class = TRUE, show.indices = TRUE)); TRUE
 dt = data.table(date=as.IDate(c(NA, "2014-12-05")))
 test(2308.01, fread("date\nNA\n2014-12-05", keepLeadingZeros=TRUE), dt)
 test(2308.02, fread("date\nNA\n2014-12-05", keepLeadingZeros=FALSE), dt)
+
+# Test that as.data.table.data.table preserves key when explicitly specified but not when omitted
+DF = data.frame(t = c(3:1, 4:5), y = 1:5)
+# data.frame to data.table with key
+test(2309.01, key(as.data.table(DF, key="t")), "t")
+# tibble to data.table with key
+class(DF) = c("tbl_df", "tbl", "data.frame")
+test(2309.02, key(as.data.table(DF, key="t")), "t")
+
+# data.table keyed with "b"
+DT = data.table(a = 1:5, b = 1:5, x = 1:5, key = "b") 
+test(2309.03, key(as.data.table(DT, key="a")), "a")   
+test(2309.04, key(as.data.table(DT)), "b")            
+test(2309.05, key(as.data.table(DT, key=NULL)), NULL) 
+
+# non-keyed data.table
+DT = data.table(a = 1:5, b = 1:5, x = 1:5)
+test(2309.06, key(as.data.table(DT, key="a")), "a")   
+test(2309.07, key(as.data.table(DT)), NULL)          
+test(2309.08, key(as.data.table(DT, key=NULL)), NULL) 

man/as.data.table.Rd

@@ -21,7 +21,7 @@ Functions to check if an object is \code{data.table}, or coerce it if possible.
 \usage{
 as.data.table(x, keep.rownames=FALSE, \dots)
 
-\method{as.data.table}{data.table}(x, \dots)
+\method{as.data.table}{data.table}(x, \dots, key=NULL)
 
 \method{as.data.table}{array}(x, keep.rownames=FALSE, key=NULL, sorted=TRUE,
               value.name="value", na.rm=TRUE, \dots)