From 4ca513153619449a2b5c14bd9f6ecb9f502aaf1b Mon Sep 17 00:00:00 2001
From: jangorecki <j.gorecki@wit.edu.pl>
Date: Mon, 3 Jun 2019 17:05:13 +0530
Subject: [PATCH 1/2] attempt for #1738

---
 src/forder.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/src/forder.c b/src/forder.c
index c507aacc2a..6d4dd3f711 100644
--- a/src/forder.c
+++ b/src/forder.c
@@ -467,6 +467,43 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrpArg, SEXP sortGroupsArg, SEXP ascArg, S
   // if n==1, the code is left to proceed below in case one or more of the 1-row by= columns are NA and na.last=NA. Otherwise it would be easy to return now.
   notFirst = false;
 
+  // first simple way, fail before test 1848 during bmerge typeof INT != DATE
+  for (int i=0; i<length(by); i++) { // #1738
+    SEXP this_col = VECTOR_ELT(DT, INTEGER(by)[i]);
+    if (INHERITS(this_col, char_Date)) {
+      SEXP this_col_int = PROTECT(coerceVector(this_col, INTSXP)); n_protect++;
+      SET_VECTOR_ELT(DT, INTEGER(by)[i], this_col_int);
+    }
+  }
+  
+  /* // another way, adding shallow copy, but so far 'memory not mapped somewhere'
+  // at the top of this fun add: SEXP DT = dt; and rename fun arg from DT to dt, also in data.table.h
+  int date_cols[length(by)];
+  int j=0;
+  Rprintf("first loop\n");
+  for (int i=0; i<length(by); i++) { // #1738
+    Rprintf("first loop; i: %d\n", i);
+    SEXP this_col = VECTOR_ELT(DT, INTEGER(by)[i]);
+    Rprintf("first loop; this_is_date: %d\n", INHERITS(this_col, char_Date));
+    bool this_is_date = INHERITS(this_col, char_Date);
+    if (this_is_date) {
+      Rprintf("first loop; i %d inherits! j %d\n", i, j);
+      date_cols[j] = INTEGER(by)[i];
+      j++;
+    }
+    Rprintf("first loop; iter end; i: %d\n", i);
+  }
+  Rprintf("j: %d\n", j);
+  if (j > 0) {
+    Rprintf("shallow wrapper\n");
+    DT = shallowwrapper(dt, R_NilValue);
+    Rprintf("second loop\n");
+    for (int i=0; i<j; i++) {
+      SEXP this_col = VECTOR_ELT(dt, date_cols[i]);
+      SET_VECTOR_ELT(DT, date_cols[i], coerceVector(this_col, INTSXP));
+    }
+  }*/
+
   SEXP ans = PROTECT(allocVector(INTSXP, nrow)); n_protect++;
   anso = INTEGER(ans);
   TEND(0)

From 692a1889a0c7699ddee96500969bd57d402a8dc0 Mon Sep 17 00:00:00 2001
From: mattdowle <mattjdowle@gmail.com>
Date: Fri, 19 Jul 2019 17:45:26 -0700
Subject: [PATCH 2/2] change to a message in verbose mode for now, news item
 asking for user feedback, and added comments to forder.c on how to do the
 automatic coerce in future

---
 NEWS.md               |  2 ++
 inst/tests/tests.Rraw |  6 ++++++
 src/forder.c          | 49 +++++++++----------------------------------
 3 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index 35f51f66ea..411a589857 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -133,6 +133,8 @@
 
 19. Type `complex` is now supported by `setkey`, `setorder`, `:=`, `by=`, `keyby=`, `shift`, `dcast`, `frank`, `rowid`, `rleid`, `CJ`, `coalesce`, `unique`, and `uniqueN`, [#3690](https://github.com/Rdatatable/data.table/issues/3690). Thanks to Gareth Ward and Elio Campitelli for their reports and input. Sorting `complex` is achieved the same way as base R; i.e., first by the real part then by the imaginary part (as if the `complex` column were two separate columns of `double`). There is no plan to support joining/merging on `complex` columns until a user demonstrates a need for that.
 
+20. `setkey`, `[key]by=` and `on=` in verbose mode (`options(datatable.verbose=TRUE)`) now detect any columns inheriting from `Date` which are stored as 8 byte double, test if any fractions are present, and if not suggest using a 4 byte integer instead (such as `data.table::IDate`) to save space and time, [#1738](https://github.com/Rdatatable/data.table/issues/1738). In future this could be upgraded to `message` or `warning` depending on feedback.
+
 #### BUG FIXES
 
 1. `first`, `last`, `head` and `tail` by group no longer error in some cases, [#2030](https://github.com/Rdatatable/data.table/issues/2030) [#3462](https://github.com/Rdatatable/data.table/issues/3462). Thanks to @franknarf1 for reporting.
diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw
index 5f149ab845..03fecf3da6 100644
--- a/inst/tests/tests.Rraw
+++ b/inst/tests/tests.Rraw
@@ -15398,6 +15398,12 @@ test(2069.32, DT1[DT2, .(y = sum(b, na.rm=TRUE)), by=.EACHI, on=c(a = 'a', b="b"
 DT = data.table(z = 1i)
 test(2069.33, DT[DT, on = 'z'], error = "Type 'complex' not supported for joining/merging")
 
+# forder verbose message when !isReallyReal Date, #1738
+DT = data.table(d=sample(seq(as.Date("2015-01-01"), as.Date("2015-01-05"), by="days"), 20, replace=TRUE))
+test(2070.01, typeof(DT$d), "double")
+test(2070.02, DT[, .N, keyby=d, verbose=TRUE], output="Column 1.*date.*8 byte double.*no fractions are present.*4 byte integer.*to save space and time")
+
+
 ###################################
 #  Add new tests above this line  #
 ###################################
diff --git a/src/forder.c b/src/forder.c
index a9cf954535..e798a9eea9 100644
--- a/src/forder.c
+++ b/src/forder.c
@@ -424,6 +424,7 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrpArg, SEXP sortGroupsArg, SEXP ascArg, S
 #endif
 
   int n_protect = 0;
+  const bool verbose = GetVerbose();
 
   if (!isNewList(DT)) {
     if (!isVectorAtomic(DT)) error("Input is not either a list of columns, or an atomic vector.");
@@ -469,43 +470,6 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrpArg, SEXP sortGroupsArg, SEXP ascArg, S
   // if n==1, the code is left to proceed below in case one or more of the 1-row by= columns are NA and na.last=NA. Otherwise it would be easy to return now.
   notFirst = false;
 
-  // first simple way, fail before test 1848 during bmerge typeof INT != DATE
-  for (int i=0; i<length(by); i++) { // #1738
-    SEXP this_col = VECTOR_ELT(DT, INTEGER(by)[i]);
-    if (INHERITS(this_col, char_Date)) {
-      SEXP this_col_int = PROTECT(coerceVector(this_col, INTSXP)); n_protect++;
-      SET_VECTOR_ELT(DT, INTEGER(by)[i], this_col_int);
-    }
-  }
-  
-  /* // another way, adding shallow copy, but so far 'memory not mapped somewhere'
-  // at the top of this fun add: SEXP DT = dt; and rename fun arg from DT to dt, also in data.table.h
-  int date_cols[length(by)];
-  int j=0;
-  Rprintf("first loop\n");
-  for (int i=0; i<length(by); i++) { // #1738
-    Rprintf("first loop; i: %d\n", i);
-    SEXP this_col = VECTOR_ELT(DT, INTEGER(by)[i]);
-    Rprintf("first loop; this_is_date: %d\n", INHERITS(this_col, char_Date));
-    bool this_is_date = INHERITS(this_col, char_Date);
-    if (this_is_date) {
-      Rprintf("first loop; i %d inherits! j %d\n", i, j);
-      date_cols[j] = INTEGER(by)[i];
-      j++;
-    }
-    Rprintf("first loop; iter end; i: %d\n", i);
-  }
-  Rprintf("j: %d\n", j);
-  if (j > 0) {
-    Rprintf("shallow wrapper\n");
-    DT = shallowwrapper(dt, R_NilValue);
-    Rprintf("second loop\n");
-    for (int i=0; i<j; i++) {
-      SEXP this_col = VECTOR_ELT(dt, date_cols[i]);
-      SET_VECTOR_ELT(DT, date_cols[i], coerceVector(this_col, INTSXP));
-    }
-  }*/
-
   SEXP ans = PROTECT(allocVector(INTSXP, nrow)); n_protect++;
   anso = INTEGER(ans);
   TEND(0)
@@ -521,7 +485,7 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrpArg, SEXP sortGroupsArg, SEXP ascArg, S
   int spare=0;  // the amount of bits remaining on the right of the current nradix byte
   bool isReal=false;
   bool complexRerun = false;   // see comments below in CPLXSXP case
-  SEXP CplxPart = R_NilValue; 
+  SEXP CplxPart = R_NilValue;
   if (n_cplx) { CplxPart=PROTECT(allocVector(REALSXP, nrow)); n_protect++; } // one alloc is reused for each part
   TEND(2);
   for (int col=0; col<ncol; col++) {
@@ -550,9 +514,16 @@ SEXP forder(SEXP DT, SEXP by, SEXP retGrpArg, SEXP sortGroupsArg, SEXP ascArg, S
       x = CplxPart;
     } // !no break! so as to fall through to REAL case
     case REALSXP :
-      if (inherits(x, "integer64")) {
+      if (INHERITS(x, char_integer64)) {
         range_i64((int64_t *)REAL(x), nrow, &min, &max, &na_count);
       } else {
+        if (verbose && INHERITS(x, char_Date) && INTEGER(isReallyReal(x))[0]==0) {
+          Rprintf("\n*** Column %d passed to forder is a date stored as an 8 byte double but no fractions are present. Please consider a 4 byte integer date such as IDate to save space and time.\n", col+1);
+          // Note the (slightly expensive) isReallyReal will only run when verbose is true. Prefix '***' just to make it stand out in verbose output
+          // In future this could be upgraded to option warning. But I figured that's what we use verbose to do (to trace problems and look for efficiencies).
+          // If an automatic coerce is desired (see discussion in #1738) then this is the point to do that in this file. Move the INTSXP case above to be
+          // next, do the coerce of Date to integer now to a tmp, and then let this case fall through to INTSXP in the same way as CPLXSXP falls through to REALSXP.
+        }
         range_d(REAL(x), nrow, &min, &max, &na_count, &infnan_count);
         if (min==0 && na_count<nrow) { min=3; max=4; } // column contains no finite numbers and is not-all NA; create dummies to yield positive min-2 later
         isReal = true;