review feedback

Author: felixcheung

R/pkg/R/SQLContext.R

@@ -38,7 +38,15 @@ getInternalType <- function(x) {
 }
 
 #' Temporary function to reroute old S3 Method call to new
-#' We need to check the class of x to ensure it is SQLContext before dispatching
+#' This function is specifically implemented to remove SQLContext from the parameter list.
+#' It determines the target to route the call by checking the parent of this callsite (say 'func').
+#' The target should be called 'func.default'.
+#' We need to check the class of x to ensure it is SQLContext/HiveContext before dispatching.
+#' @param newFuncSig name of the function the user should call instead in the deprecation message
+#' @param x the first parameter of the original call
+#' @param ... the rest of parameter to pass along
+#' @return whatever the target returns
+#' @noRd
 dispatchFunc <- function(newFuncSig, x, ...) {
   funcName <- as.character(sys.call(sys.parent())[[1]])
   f <- get(paste0(funcName, ".default"))

R/pkg/inst/tests/testthat/test_sparkSQL.R

@@ -32,6 +32,21 @@ markUtf8 <- function(s) {
   s
 }
 
+setHiveContext <- function() {
+  hiveCtx <- tryCatch({
+    newJObject("org.apache.spark.sql.hive.test.TestHiveContext", ssc)
+  },
+  error = function(err) {
+    skip("Hive is not build with SparkSQL, skipped")
+  })
+  assign(".sparkRHivesc", hiveCtx, envir = .sparkREnv)
+  hiveCtx
+}
+
+unsetHiveContext <- function() {
+  remove(".sparkRHivesc", envir = .sparkREnv)
+}
+
 # Tests for SparkSQL functions in SparkR
 
 sc <- sparkR.init()
@@ -163,20 +178,15 @@ test_that("create DataFrame from RDD", {
                list(name = "John", age = 19L, height = 176.5))
 
   ssc <- callJMethod(sc, "sc")
-  hiveCtx <- tryCatch({
-    newJObject("org.apache.spark.sql.hive.test.TestHiveContext", ssc)
-  },
-  error = function(err) {
-    skip("Hive is not build with SparkSQL, skipped")
-  })
-  assign(".sparkRHivesc", hiveCtx, envir = .sparkREnv)
+  setHiveContext()
   sql("CREATE TABLE people (name string, age double, height float)")
   df <- read.df(jsonPathNa, "json", schema)
   invisible(insertInto(df, "people"))
   expect_equal(collect(sql("SELECT age from people WHERE name = 'Bob'"))$age,
                c(16))
   expect_equal(collect(sql("SELECT height from people WHERE name ='Bob'"))$height,
                c(176.5))
+  unsetHiveContext()
   remove(".sparkRHivesc", envir = .sparkREnv)
 })
 
@@ -955,13 +965,7 @@ test_that("column calculation", {
 
 test_that("test HiveContext", {
   ssc <- callJMethod(sc, "sc")
-  hiveCtx <- tryCatch({
-    newJObject("org.apache.spark.sql.hive.test.TestHiveContext", ssc)
-  },
-  error = function(err) {
-    skip("Hive is not build with SparkSQL, skipped")
-  })
-  assign(".sparkRHivesc", hiveCtx, envir = .sparkREnv)
+  setHiveContext()
   df <- createExternalTable("json", jsonPath, "json")
   expect_is(df, "SparkDataFrame")
   expect_equal(count(df), 3)
@@ -989,7 +993,7 @@ test_that("test HiveContext", {
   expect_is(df5, "SparkDataFrame")
   expect_equal(count(df5), 3)
   unlink(parquetDataPath)
-  remove(".sparkRHivesc", envir = .sparkREnv)
+  unsetHiveContext()
 })
 
 test_that("column operators", {
@@ -2138,14 +2142,7 @@ test_that("repartition by columns on DataFrame", {
 
 test_that("Window functions on a DataFrame", {
   ssc <- callJMethod(sc, "sc")
-  hiveCtx <- tryCatch({
-    newJObject("org.apache.spark.sql.hive.test.TestHiveContext", ssc)
-  },
-  error = function(err) {
-    skip("Hive is not build with SparkSQL, skipped")
-  })
-
-  assign(".sparkRHivesc", hiveCtx, envir = .sparkREnv)
+  setHiveContext()
   df <- createDataFrame(list(list(1L, "1"), list(2L, "2"), list(1L, "1"), list(2L, "2")),
                         schema = c("key", "value"))
   ws <- orderBy(window.partitionBy("key"), "value")
@@ -2170,7 +2167,7 @@ test_that("Window functions on a DataFrame", {
   result <- collect(select(df, over(lead("key", 1), ws), over(lead("value", 1), ws)))
   names(result) <- c("key", "value")
   expect_equal(result, expected)
-  remove(".sparkRHivesc", envir = .sparkREnv)
+  unsetHiveContext()
 })
 
 test_that("createDataFrame sqlContext parameter backward compatibility", {