From 537c81fce54e1046e5b1d679d7165f2c2678adc1 Mon Sep 17 00:00:00 2001
From: jangorecki <j.gorecki@wit.edu.pl>
Date: Wed, 11 Sep 2019 19:16:10 +0200
Subject: [PATCH] first and last not load xts namespace when not needed, first
 on empty dt returns empty dt, closes #3857, #3858

---
 NEWS.md               |  2 ++
 R/last.R              | 44 +++++++++++++++++++++++--------------------
 inst/tests/tests.Rraw | 19 +++++++++++++++++++
 3 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index e3820812ee..7fc1b86c4c 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -336,6 +336,8 @@
 
 22. Optimized `mean` of `integer` columns no longer warns about a coercion to numeric, [#986](https://github.com/Rdatatable/data.table/issues/986). Thanks @dgrtwo for his [YouTube tutorial at 3:01](https://youtu.be/AmE4LXPQErM?t=175) where the warning occurs.
 
+23. Using `first` and `last` function on `POSIXct` object no longer loads `xts` namespace, [#3857](https://github.com/Rdatatable/data.table/issues/3857). `first` on empty `data.table` returns empty `data.table` now [#3858](https://github.com/Rdatatable/data.table/issues/3858).
+
 
 ### Changes in [v1.12.2](https://github.com/Rdatatable/data.table/milestone/14?closed=1)  (07 Apr 2019)
 
diff --git a/R/last.R b/R/last.R
index 608877e1e9..052f8e8277 100644
--- a/R/last.R
+++ b/R/last.R
@@ -5,33 +5,37 @@
 # If xts is loaded higher than data.table, xts::last will work but slower.
 last = function(x, n=1L, ...) {
   if (nargs()==1L) {
-    if (is.vector(x)) {
-      if (!length(x)) return(x) else return(x[[length(x)]])  # for vectors, [[ works like [
-    } else if (is.data.frame(x)) return(x[NROW(x),])
-  }
-  if(!requireNamespace("xts", quietly = TRUE)) {
-    tail(x, n=n, ...) # nocov
+    if (is.vector(x) || is.atomic(x)) {
+      if (!length(x)) x else x[[length(x)]]
+    } else if (is.data.frame(x)) {
+      x[NROW(x),]
+    }
+  } else if ("package:xts" %chin% search()) {
+    if (!requireNamespace("xts", quietly=TRUE))
+      stop("internal error, package:xts is on search path but could not be loaded via requireNamespace") # nocov
+    if (isTRUE(getOption("datatable.verbose", FALSE)))
+      cat("last: using xts::last\n")
+    xts::last(x, n=n, ...) # UseMethod("last") doesn't find xts's methods, not sure what I did wrong.
   } else {
-    # fix with suggestion from Joshua, #1347
-    if (!"package:xts" %chin% search()) {
-      tail(x, n=n, ...) # nocov
-    } else xts::last(x, n=n, ...) # UseMethod("last") doesn't find xts's methods, not sure what I did wrong.
+    tail(x, n=n, ...) # nocov
   }
 }
 
 # first(), similar to last(), not sure why this wasn't exported in the first place...
 first = function(x, n=1L, ...) {
   if (nargs()==1L) {
-    if (is.vector(x)) {
-      if (!length(x)) return(x) else return(x[[1L]])
-    } else if (is.data.frame(x)) return(x[1L,])
-  }
-  if(!requireNamespace("xts", quietly = TRUE)) {
-    head(x, n=n, ...) # nocov
+    if (is.vector(x) || is.atomic(x)) {
+      if (!length(x)) x else x[[1L]]
+    } else if (is.data.frame(x)) {
+      if (!NROW(x)) x else x[1L,]
+    }
+  } else if ("package:xts" %chin% search()) {
+    if (!requireNamespace("xts", quietly=TRUE))
+      stop("internal error, package:xts is on search path but could not be loaded via requireNamespace") # nocov
+    if (isTRUE(getOption("datatable.verbose", FALSE)))
+      cat("first: using xts::first\n")
+    xts::first(x, n=n, ...)
   } else {
-    # fix with suggestion from Joshua, #1347
-    if (!"package:xts" %chin% search()) {
-      head(x, n=n, ...) # nocov
-    } else xts::first(x, n=n, ...) # nocov
+    head(x, n=n, ...) # nocov
   }
 }
diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw
index 93aa828bf2..02681b2f27 100644
--- a/inst/tests/tests.Rraw
+++ b/inst/tests/tests.Rraw
@@ -15936,6 +15936,25 @@ test(2100.14, fifelse(c(T,F,NA),c(1,1,1),c(2,2,2),NA), c(1,2,NA))
 DT = data.table(id=1:3, v=4:6, key="id")
 test(2101, DT[.(logical())], data.table(id=logical(), v=integer(), key="id"))
 
+# first and last not loading xts namespace anymore
+x = as.POSIXct("2019-09-09")+0:1
+options(datatable.verbose=TRUE)
+test(2102.01, last(x), x[length(x)], notOutput="last: using xts::last")
+test(2102.02, first(x), x[1L], notOutput="first: using xts::first")
+if (test_xts) {
+  xt = xts(1:2, x)
+  test(2102.03, last(xt, 2L), xt, output="last: using xts::last")
+  test(2102.04, first(xt, 2L), xt, output="first: using xts::first")
+  xt = xts(matrix(1:4, 2L, 2L), x)
+  test(2102.05, last(xt, 2L), xt, output="last: using xts::last")
+  test(2102.06, first(xt, 2L), xt, output="first: using xts::first")
+}
+# first on empty df now match head(df, n=1L)
+df = data.frame(a=integer(), b=integer())
+test(2102.11, first(df), df, notOutput="first: using xts::first")
+test(2102.12, tail(df), df, notOutput="tail: using xts::tail")
+options(datatable.verbose=FALSE)
+
 
 ###################################
 #  Add new tests above this line  #