Merge pull request #588 from duckdb/f-un-enable

chore: Remove `enable_materialization` argument in favor of creating a new data frame when needed

Author: krlmlr

R/cpp11.R

@@ -181,8 +181,8 @@ rapi_rel_to_altrep <- function(rel, allow_materialization = TRUE) {
   .Call(`_duckdb_rapi_rel_to_altrep`, rel, allow_materialization)
 }
 
-rapi_rel_from_altrep_df <- function(df, strict, allow_materialized, enable_materialization) {
-  .Call(`_duckdb_rapi_rel_from_altrep_df`, df, strict, allow_materialized, enable_materialization)
+rapi_rel_from_altrep_df <- function(df, strict, allow_materialized) {
+  .Call(`_duckdb_rapi_rel_from_altrep_df`, df, strict, allow_materialized)
 }
 
 rapi_release <- function(stmt) {

R/relational.R

@@ -392,17 +392,15 @@ rel_to_altrep <- function(rel, allow_materialization = TRUE) {
 #' @param strict whether to throw an error if the data frame is not an altrep
 #'   or if other criteria are not met
 #' @param allow_materialized whether to succeed if the data frame is already materialized
-#' @param enable_materialization set to `TRUE` for side effect: allow materialization of this a data frame
-#'   if it was not allowed previously
 #' @return the relation object
 #' @noRd
 #' @examples
 #' con <- DBI::dbConnect(duckdb())
 #' rel <- rel_from_df(con, mtcars)
 #' df = rel_to_altrep(rel)
 #' print(rel_from_altrep_df(df))
-rel_from_altrep_df <- function(df, strict = TRUE, allow_materialized = TRUE, enable_materialization = FALSE) {
-  rethrow_rapi_rel_from_altrep_df(df, strict, allow_materialized, enable_materialization)
+rel_from_altrep_df <- function(df, strict = TRUE, allow_materialized = TRUE) {
+  rethrow_rapi_rel_from_altrep_df(df, strict, allow_materialized)
 }
 
 

R/rethrow-gen.R

@@ -406,9 +406,9 @@ rethrow_rapi_rel_to_altrep <- function(rel, allow_materialization = TRUE, call =
   )
 }
 
-rethrow_rapi_rel_from_altrep_df <- function(df, strict, allow_materialized, enable_materialization, call = parent.frame(2)) {
+rethrow_rapi_rel_from_altrep_df <- function(df, strict, allow_materialized, call = parent.frame(2)) {
   rlang::try_fetch(
-    rapi_rel_from_altrep_df(df, strict, allow_materialized, enable_materialization),
+    rapi_rel_from_altrep_df(df, strict, allow_materialized),
     error = function(e) {
       rethrow_error_from_rapi(e, call)
     }

src/cpp11.cpp

@@ -329,10 +329,10 @@ extern "C" SEXP _duckdb_rapi_rel_to_altrep(SEXP rel, SEXP allow_materialization)
   END_CPP11
 }
 // reltoaltrep.cpp
-SEXP rapi_rel_from_altrep_df(SEXP df, bool strict, bool allow_materialized, bool enable_materialization);
-extern "C" SEXP _duckdb_rapi_rel_from_altrep_df(SEXP df, SEXP strict, SEXP allow_materialized, SEXP enable_materialization) {
+SEXP rapi_rel_from_altrep_df(SEXP df, bool strict, bool allow_materialized);
+extern "C" SEXP _duckdb_rapi_rel_from_altrep_df(SEXP df, SEXP strict, SEXP allow_materialized) {
   BEGIN_CPP11
-    return cpp11::as_sexp(rapi_rel_from_altrep_df(cpp11::as_cpp<cpp11::decay_t<SEXP>>(df), cpp11::as_cpp<cpp11::decay_t<bool>>(strict), cpp11::as_cpp<cpp11::decay_t<bool>>(allow_materialized), cpp11::as_cpp<cpp11::decay_t<bool>>(enable_materialization)));
+    return cpp11::as_sexp(rapi_rel_from_altrep_df(cpp11::as_cpp<cpp11::decay_t<SEXP>>(df), cpp11::as_cpp<cpp11::decay_t<bool>>(strict), cpp11::as_cpp<cpp11::decay_t<bool>>(allow_materialized)));
   END_CPP11
 }
 // statement.cpp
@@ -472,7 +472,7 @@ static const R_CallMethodDef CallEntries[] = {
     {"_duckdb_rapi_rel_distinct",            (DL_FUNC) &_duckdb_rapi_rel_distinct,            1},
     {"_duckdb_rapi_rel_explain",             (DL_FUNC) &_duckdb_rapi_rel_explain,             1},
     {"_duckdb_rapi_rel_filter",              (DL_FUNC) &_duckdb_rapi_rel_filter,              2},
-    {"_duckdb_rapi_rel_from_altrep_df",      (DL_FUNC) &_duckdb_rapi_rel_from_altrep_df,      4},
+    {"_duckdb_rapi_rel_from_altrep_df",      (DL_FUNC) &_duckdb_rapi_rel_from_altrep_df,      3},
     {"_duckdb_rapi_rel_from_df",             (DL_FUNC) &_duckdb_rapi_rel_from_df,             3},
     {"_duckdb_rapi_rel_from_sql",            (DL_FUNC) &_duckdb_rapi_rel_from_sql,            2},
     {"_duckdb_rapi_rel_from_table",          (DL_FUNC) &_duckdb_rapi_rel_from_table,          3},

src/reltoaltrep.cpp

@@ -385,7 +385,7 @@ static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) {
 	return data_frame;
 }
 
-[[cpp11::register]] SEXP rapi_rel_from_altrep_df(SEXP df, bool strict, bool allow_materialized, bool enable_materialization) {
+[[cpp11::register]] SEXP rapi_rel_from_altrep_df(SEXP df, bool strict, bool allow_materialized) {
 	if (!Rf_inherits(df, "data.frame")) {
 		if (strict) {
 			cpp11::stop("rapi_rel_from_altrep_df: Not a data.frame");
@@ -439,12 +439,6 @@ static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) {
 		}
 	}
 
-	// Side effect comes last
-	// FIXME: Add separate rapi_() function for this
-	if (enable_materialization) {
-		wrapper->rel->allow_materialization = true;
-	}
-
 	return res;
 }
 

tests/testthat/test-relational.R

@@ -809,6 +809,7 @@ test_that("rel_project does not automatically quote upper-case column names", {
   ref <- expr_reference(names(df))
   exprs <- list(ref)
   proj <- rel_project(rel, exprs)
+  # FIXME: Change to rel_to_altrep() in 1.1.3
   ans <- rapi_rel_to_altrep(proj)
   expect_equal(df, ans)
 })
@@ -860,6 +861,7 @@ test_that("we don't crash with evaluation errors", {
     )
   )
 
+  # FIXME: Change to rel_to_altrep() in 1.1.3
   ans <- rapi_rel_to_altrep(rel2)
 
   # This query is supposed to throw a runtime error.
@@ -891,6 +893,7 @@ test_that("we don't crash with evaluation errors", {
     )
   )
 
+  # FIXME: Change to rel_to_altrep() in 1.1.3
   ans <- rapi_rel_to_altrep(rel2)
 
   # This query is supposed to throw a runtime error.