revdep dtts test failure due to CsubsetDT rename

[tdhock]

With data.table from github master installed, I get the following test failure from revdep dtts:

* checking tests ...
  Running 'tinytest.R'
 ERROR
Running the tests in 'tests/tinytest.R' failed.
Last 13 lines of output:
  test_dtts.R...................   32 tests OK 
  test_dtts.R...................   32 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK 
  test_dtts.R...................   33 tests OK Error in .align_duration_cpp(x[[1]], sort(y), x, start, end, sopen, eopen,  : 
    function 'CsubsetDT' not provided by package 'data.table'
  Calls: <Anonymous> ... .local -> data.table -> do.call -> .align_duration_cpp
  Execution halted

git bisect says this started happening after merging #4753 which renamed an exported C function so the easy fix is for dtts to use the new name. Maintainer of dtts is @eddelbuettel who reviewed that PR so I assume he is aware of the rename and will do it when we submit the new version of data.table to CRAN.

[eddelbuettel]

I will take a closer look tomorrow.

From a first look, there is a mismatch as CsubsetDT and CsubsetVector are exported in src/init.c via callMethods[] but later DT_subsetDT is the function that is registered. Those two need to align, but do not.

For example, just weeks ago I exported functions from RcppSpdlog and there we have (in Rcpp auuto-generated code, so a little verbose) the same functions in both CallEntries[] going into R_registerRoutines() as we have in the different R_RegisterCCallable() calls, see https://github.com/eddelbuettel/rcppspdlog/blob/master/src/RcppExports.cpp (and I have better/simper human-written examples elsewhere).

Now, dtts has been on CRAN unchanged for a while. But it is possible that this was always unfinished business. The patches by @lsilvest and myself had been dormant for some time. It won't take much to clean this up.

[eddelbuettel]

How exactly are you triggering an error in dtts? R CMD check --as-cran works fine for me in dtts.

Also running the tinytest file directly (via a trivial wrapper) shows no issue:

edd@rob:~/git/dtts(feature/small_refactor)$ tt.r -f inst/tinytest/test_dtts.R 
test_dtts.R...................   84 tests OK 0.9s
All ok, 84 results (0.9s)
edd@rob:~/git/dtts(feature/small_refactor)$ 

Oh, I see it with master in data.table. That is odder as nothing appears to have changed and releases 0.14.0, 0.14.2, 0.14.4, and 0.14.6 all passed.

[eddelbuettel]

Here is a diff between 0.14.6 and master than may explain it:

 void attribute_visible R_init_data_table(DllInfo *info)
 {
-  // C exported routines, see ?cdt for details
-  R_RegisterCCallable("data.table", "CsubsetDT", (DL_FUNC) &subsetDT);
+  // C exported routines
+  // must be also listed in inst/include/datatableAPI.h
+  // for end user documentation see ?cdt
+  R_RegisterCCallable("data.table", "DT_subsetDT", (DL_FUNC) &subsetDT);

[lsilvest]

Exactly. Looks like after the change of name dtts will be dependent on the latest data.table version as R_GetCCallable calls error if it can't find the function we ask it to get.

[eddelbuettel]

And, for completeness, the same issue in inst/include/datatableAPI.h:

edd@rob:/tmp/data.table-1.14.6$ diff -ru inst/include/datatableAPI.h ~/git/data.table/inst/include/datatableAPI.h 
--- inst/include/datatableAPI.h 2022-11-15 18:47:20.000000000 -0600
+++ /home/edd/git/data.table/inst/include/datatableAPI.h        2021-09-28 17:34:28.619110668 -0500
@@ -21,11 +21,14 @@
 /* provided the interface for the function exported in
    ../src/init.c via R_RegisterCCallable()             */
 
+// subsetDT #3751
 inline SEXP attribute_hidden DT_subsetDT(SEXP x, SEXP rows, SEXP cols) {
      static SEXP(*fun)(SEXP, SEXP, SEXP) =
-       (SEXP(*)(SEXP,SEXP,SEXP)) R_GetCCallable("data.table", "CsubsetDT");
+       (SEXP(*)(SEXP,SEXP,SEXP)) R_GetCCallable("data.table", "DT_subsetDT");
      return fun(x,rows,cols);
 }
+// forder #4015
+// setalloccol alloccolwrapper setDT #4439
 
 /* permit opt-in to redefine shorter identifiers */
 #if defined(DATATABLE_REMAP_API)
edd@rob:/tmp/data.table-1.14.6$ 

[eddelbuettel]

So what appears to be happening here is that

• your master branch is ahead of the release branches carrying 2.14.{0,2,4,6}, and has been for some time
• the current master branch imposes a downstream renaming; we could do that but it has to be coordinated
• the names at the R_RegisterCCallable() and R_GetCCallable() do not actually matter all that much:
  • it is all inside a namespace,
  • it is not directly user-facing
• there are two functions ready to be exported so we may as well have a conversation about both

[eddelbuettel]

It is also point 2 under 'Notes' in the current NEWS.md and as such not a new issue but something that could probably be closed as a duplicate.

[tdhock]

This issue should not block new DT CRAN submission (revdep will update after new DT appears on CRAN).