Allow type override in := when i is an ordering call

NEWS.md

@@ -46,6 +46,8 @@
 
 12. `frank()` gains an `order` argument (matching `frankv()`) and now intercepts the unary minus symbol (e.g., `frank(-dates)`) to support reverse ranking even for types where unary `-` is not defined in R, such as `Date` or `character` vectors, [#5489](https://github.com/Rdatatable/data.table/issues/5489). Thanks @hope-data-science for the request and @venom1204 for the implementation.
 
+13. Type overrides (e.g. changing an integer column to character) are now permitted in `:=` when `i` is a call to `order()` or `forder()`, [#2925](https://github.com/Rdatatable/data.table/issues/2925). Previously, any non-missing `i` forced `data.table` into "subset update" mode which prevents type changes; this restriction is now relaxed for ordering calls since they encompass all rows. Thanks @MichaelChirico for the report and @venom1204 for the fix.
+
 ### BUG FIXES
 
 1. `fread()` with `skip=0` and `(header=TRUE|FALSE)` no longer skips the first row when it has fewer fields than subsequent rows, [#7463](https://github.com/Rdatatable/data.table/issues/7463). Thanks @emayerhofer for the report and @ben-schwen for the fix.

R/data.table.R

@@ -548,6 +548,7 @@ replace_dot_alias = function(e) {
     on.exit(options(oldverbose))
   }
   .global$print=""
+  i_is_order = FALSE
   missingby = missing(by) && missing(keyby)  # for tests 359 & 590 where passing by=NULL results in data.table not vector
   if (missingby || missing(j)) {
     if (!missingby) warningf("Ignoring by/keyby because 'j' is not supplied")
@@ -727,6 +728,7 @@ replace_dot_alias = function(e) {
   }
   if (!missing(i)) {
     xo = NULL
+    i_is_order = is.call(isub) && any(as.character(isub[[1L]]) %chin% c("order", "forder"))
     if (identical(isub, NA)) {
       # only possibility *isub* can be NA (logical) is the symbol NA itself; i.e. DT[NA]
       # replace NA in this case with NA_integer_ as that's almost surely what user intended to
@@ -1839,6 +1841,9 @@ replace_dot_alias = function(e) {
         }
       }
       # TODO?: use set() here now that it can add new columns. Then remove newnames and alloc logic above.
+      if (isTRUE(i_is_order) && !is.null(irows)) {
+        setattr(irows, ".datatable.full_order", TRUE)
+      }
       .Call(Cassign,x,irows,cols,newnames,jval)
       return(suppPrint(x))
     }

inst/tests/tests.Rraw

@@ -21974,3 +21974,15 @@ test(2386.10, frank(-dates, order=-1L), frankv(dates, order=-1L), warning=warn)
 test(2386.11, frank(+dates), frankv(dates, order=1L))
 chars = c("b", "a", "c", "a")
 test(2386.12, frank(-chars), frankv(chars, order=-1L))
+
+# #2925: Allow type override in := when i is order()
+DT = data.table(a = 1:10)
+DT[order(-a), a := paste0(a)]
+test(2925.1, class(DT$a), "character")
+test(2925.2, DT$a, as.character(1:10))
+DT = data.table(a = rep(letters[1:3], 3), b = 1:9)
+DT[order(-b), a := factor(a, levels = unique(a))]
+test(2925.3, is.factor(DT$a), TRUE)
+test(2925.4, levels(DT$a), c("c", "b", "a"))
+DT = data.table(a = 1:10)
+test(2925.5, DT[1:5, a := as.character(a)], data.table(a=1:10), warning="Coercing 'character' RHS to 'integer'")

src/assign.c

@@ -363,6 +363,8 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values)
   bool verbose=GetVerbose();
   int ndelete=0;  // how many columns are being deleted
   int *buf;
+  SEXP full_order_attr = (isNull(rows)) ? R_NilValue : getAttrib(rows, install(".datatable.full_order"));
+  bool is_full_order = (isLogical(full_order_attr) && length(full_order_attr)>0 && LOGICAL(full_order_attr)[0]==1);
   if (isNull(dt)) error(_("assign has been passed a NULL dt"));
   if (TYPEOF(dt) != VECSXP) error(_("dt passed to %s isn't type VECSXP"), "assign");
   if (islocked(dt))
@@ -602,14 +604,20 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values)
 
     if (coln+1 > oldncol) {  // new column
       SET_VECTOR_ELT(dt, coln, targetcol=allocNAVectorLike(thisvalue, nrow));
+      if (isVectorAtomic(thisvalue)) copyMostAttrib(thisvalue,targetcol);
       // initialize with NAs for when 'rows' is a subset and it doesn't touch
       // do not try to save the time to NA fill (contiguous branch free assign anyway) since being
       // sure all items will be written to (isNull(rows), length(rows), vlen<1, targetlen) is not worth the risk.
-      if (isVectorAtomic(thisvalue)) copyMostAttrib(thisvalue,targetcol);  // class etc but not names
       // else for lists (such as data.frame and data.table) treat them as raw lists and drop attribs
       if (vlen<1) continue;   // e.g. DT[,newcol:=integer()] (adding new empty column)
     } else {                 // existing column
       targetcol = VECTOR_ELT(dt,coln);
+      if (is_full_order && !isNull(rows) && (TYPEOF(targetcol) != TYPEOF(thisvalue) || isFactor(thisvalue))) {
+          targetcol = PROTECT(allocNAVectorLike(thisvalue, nrow));
+          if (isVectorAtomic(thisvalue)) copyMostAttrib(thisvalue, targetcol);
+          SET_VECTOR_ELT(dt, coln, targetcol);
+          UNPROTECT(1);
+      }
     }
     const char *ret = memrecycle(targetcol, rows, 0, targetlen, thisvalue, 0, -1, coln+1, CHAR(STRING_ELT(names, coln)));
     if (ret) warning("%s", ret); // # notranslate