Merge pull request #381 from tidyverse/arrange-setorder-na_last

Use `setorder(na.last = TRUE)` in `arrange()`

R/step-subset-arrange.R

@@ -28,6 +28,7 @@ arrange.dtplyr_step <- function(.data, ..., .by_group = FALSE) {
   # Order without grouping then restore
   dots <- set_names(dots, NULL)
   if (is_copied(.data) && no_transmute) {
+    dots <- c(dots, na.last = TRUE)
     step <- step_call(.data, "setorder", dots)
   } else {
     step <- step_subset(.data, i = call2("order", !!!dots), groups = character())

tests/testthat/test-step-subset-arrange.R

@@ -70,7 +70,7 @@ test_that("uses setorder when there is already a copy", {
 
   expect_equal(
     show_query(step_implicit),
-    expr(setorder(DT[x < 4], x, y))
+    expr(setorder(DT[x < 4], x, y, na.last = TRUE))
   )
 
   # Works with explicit copy
@@ -80,10 +80,22 @@ test_that("uses setorder when there is already a copy", {
 
   expect_equal(
     show_query(step_explicit),
-    expr(setorder(copy(DT)[, `:=`(x = x * 2)], x, -y))
+    expr(setorder(copy(DT)[, `:=`(x = x * 2)], x, -y, na.last = TRUE))
   )
 })
 
+test_that("setorder places NAs last", {
+  dt <- lazy_dt(tibble(x = c("b", NA, "a")), "DT")
+  dt$needs_copy <- TRUE
+
+  # Works with implicit copy
+  res <- dt %>%
+    arrange(x) %>%
+    as.data.table()
+
+  expect_equal(res$x, c("a", "b", NA))
+})
+
 test_that("works with a transmute expression", {
   dt <- lazy_dt(data.frame(x = 1:3, y = 1:3), "DT")