From 0507f8e0cf9e29320473770f29c249e8d5aaebdb Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Wed, 21 Feb 2024 11:16:21 +0100 Subject: [PATCH 1/5] initial altlist support for LIST logical type --- src/include/reltoaltrep.hpp | 2 ++ src/reltoaltrep.cpp | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/include/reltoaltrep.hpp b/src/include/reltoaltrep.hpp index 8161324d5..d202a86c2 100644 --- a/src/include/reltoaltrep.hpp +++ b/src/include/reltoaltrep.hpp @@ -15,10 +15,12 @@ struct RelToAltrep { static Rboolean RelInspect(SEXP x, int pre, int deep, int pvec, void (*inspect_subtree)(SEXP, int, int, int)); static SEXP VectorStringElt(SEXP x, R_xlen_t i); + static SEXP VectorListElt(SEXP x, R_xlen_t i); static R_altrep_class_t rownames_class; static R_altrep_class_t logical_class; static R_altrep_class_t int_class; static R_altrep_class_t real_class; static R_altrep_class_t string_class; + static R_altrep_class_t list_class; }; diff --git a/src/reltoaltrep.cpp b/src/reltoaltrep.cpp index afbdd670b..34d2aa3da 100644 --- a/src/reltoaltrep.cpp +++ b/src/reltoaltrep.cpp @@ -10,6 +10,7 @@ R_altrep_class_t RelToAltrep::logical_class; R_altrep_class_t RelToAltrep::int_class; R_altrep_class_t RelToAltrep::real_class; R_altrep_class_t RelToAltrep::string_class; +R_altrep_class_t RelToAltrep::list_class; void RelToAltrep::Initialize(DllInfo *dll) { // this is a string so setting row names will not lead to materialization @@ -18,28 +19,33 @@ void RelToAltrep::Initialize(DllInfo *dll) { int_class = R_make_altinteger_class("reltoaltrep_int_class", "duckdb", dll); real_class = R_make_altreal_class("reltoaltrep_real_class", "duckdb", dll); string_class = R_make_altstring_class("reltoaltrep_string_class", "duckdb", dll); + list_class = R_make_altlist_class("reltoaltrep_list_class", "duckdb", dll); R_set_altrep_Inspect_method(rownames_class, RownamesInspect); R_set_altrep_Inspect_method(logical_class, RelInspect); R_set_altrep_Inspect_method(int_class, RelInspect); R_set_altrep_Inspect_method(real_class, RelInspect); R_set_altrep_Inspect_method(string_class, RelInspect); + R_set_altrep_Inspect_method(list_class, RelInspect); R_set_altrep_Length_method(rownames_class, RownamesLength); R_set_altrep_Length_method(logical_class, VectorLength); R_set_altrep_Length_method(int_class, VectorLength); R_set_altrep_Length_method(real_class, VectorLength); R_set_altrep_Length_method(string_class, VectorLength); + R_set_altrep_Length_method(list_class, VectorLength); R_set_altvec_Dataptr_method(rownames_class, RownamesDataptr); R_set_altvec_Dataptr_method(logical_class, VectorDataptr); R_set_altvec_Dataptr_method(int_class, VectorDataptr); R_set_altvec_Dataptr_method(real_class, VectorDataptr); R_set_altvec_Dataptr_method(string_class, VectorDataptr); + R_set_altvec_Dataptr_method(list_class, VectorDataptr); R_set_altvec_Dataptr_or_null_method(rownames_class, RownamesDataptrOrNull); R_set_altstring_Elt_method(string_class, VectorStringElt); + R_set_altlist_Elt_method(list_class, VectorListElt); } template @@ -231,6 +237,12 @@ SEXP RelToAltrep::VectorStringElt(SEXP x, R_xlen_t i) { END_CPP11 } +SEXP RelToAltrep::VectorListElt(SEXP x, R_xlen_t i) { + BEGIN_CPP11 + return VECTOR_ELT(AltrepVectorWrapper::Get(x)->Vector(), i); + END_CPP11 +} + static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) { switch (type.id()) { case LogicalTypeId::BOOLEAN: @@ -261,13 +273,15 @@ static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) { case LogicalTypeId::VARCHAR: case LogicalTypeId::UUID: return RelToAltrep::string_class; + case LogicalTypeId::LIST: + return RelToAltrep::list_class; default: cpp11::stop("rel_to_altrep: Unknown column type for altrep: %s", type.ToString().c_str()); } } [[cpp11::register]] SEXP rapi_rel_to_altrep(duckdb::rel_extptr_t rel) { - D_ASSERT(rel && rel->rel); + D_ASSERT(rel && rel->rel); auto drel = rel->rel; auto ncols = drel->Columns().size(); From ac7264b3114f6136b5c2246905b24fe43c07c448 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Thu, 22 Feb 2024 11:46:12 +0100 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=A7=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/reltoaltrep.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reltoaltrep.cpp b/src/reltoaltrep.cpp index 34d2aa3da..804d783d5 100644 --- a/src/reltoaltrep.cpp +++ b/src/reltoaltrep.cpp @@ -281,7 +281,7 @@ static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) { } [[cpp11::register]] SEXP rapi_rel_to_altrep(duckdb::rel_extptr_t rel) { - D_ASSERT(rel && rel->rel); + D_ASSERT(rel && rel->rel); auto drel = rel->rel; auto ncols = drel->Columns().size(); From f03a66302b24cc23ec496220e9775b6f905413b4 Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Thu, 22 Feb 2024 17:29:22 +0100 Subject: [PATCH 3/5] + test for rel_filter() with LIST column type --- tests/testthat/test_list.R | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/testthat/test_list.R b/tests/testthat/test_list.R index 7b7a042b0..249b42bf8 100644 --- a/tests/testthat/test_list.R +++ b/tests/testthat/test_list.R @@ -26,3 +26,16 @@ test_that("one-level lists can be read", { res <- dbGetQuery(con, "SELECT ['Hello', 'World'] a union all select ['There']")$a expect_equal(res, list(c("Hello", "World"), c("There"))) }) + +test_that("rel_filter() handles LIST logical type", { + con <- dbConnect(duckdb()) + on.exit(dbDisconnect(con, shutdown = TRUE)) + + df1 <- tibble::tibble(a = list(1, c(1,2))) + + rel1 <- rel_from_df(con, df1) + rel2 <- rel_filter(rel1, list(expr_constant(TRUE))) + + df2 <- rel_to_altrep(rel2) + expect_equal(df1$a, df2$a) +}) From 2402b76ff33bedf5cc8ee28d46ebc0e34b866d4d Mon Sep 17 00:00:00 2001 From: Romain Francois Date: Fri, 23 Feb 2024 15:25:37 +0100 Subject: [PATCH 4/5] condition compilation. ALREP for VECSXP was introduced in R 4.3.0 --- src/include/rapi.hpp | 5 +++++ src/include/reltoaltrep.hpp | 6 +++++- src/reltoaltrep.cpp | 21 +++++++++++++++++---- tests/testthat/test_list.R | 2 ++ 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/include/rapi.hpp b/src/include/rapi.hpp index 1e4052814..80b227c2b 100644 --- a/src/include/rapi.hpp +++ b/src/include/rapi.hpp @@ -4,6 +4,7 @@ #include #include +#include #include "duckdb.hpp" #include "duckdb/function/table_function.hpp" @@ -11,6 +12,10 @@ #include "duckdb/parser/tableref/table_function_ref.hpp" #include "duckdb/common/mutex.hpp" +#if defined(R_VERSION) && R_VERSION >= R_Version(4, 3, 0) +#define R_HAS_ALTLIST +#endif + namespace duckdb { typedef unordered_map arrow_scans_t; diff --git a/src/include/reltoaltrep.hpp b/src/include/reltoaltrep.hpp index d202a86c2..f6a1f2fec 100644 --- a/src/include/reltoaltrep.hpp +++ b/src/include/reltoaltrep.hpp @@ -15,12 +15,16 @@ struct RelToAltrep { static Rboolean RelInspect(SEXP x, int pre, int deep, int pvec, void (*inspect_subtree)(SEXP, int, int, int)); static SEXP VectorStringElt(SEXP x, R_xlen_t i); - static SEXP VectorListElt(SEXP x, R_xlen_t i); static R_altrep_class_t rownames_class; static R_altrep_class_t logical_class; static R_altrep_class_t int_class; static R_altrep_class_t real_class; static R_altrep_class_t string_class; + +#if defined(R_HAS_ALTLIST) + static SEXP VectorListElt(SEXP x, R_xlen_t i); static R_altrep_class_t list_class; +#endif + }; diff --git a/src/reltoaltrep.cpp b/src/reltoaltrep.cpp index 804d783d5..c19cfa0b3 100644 --- a/src/reltoaltrep.cpp +++ b/src/reltoaltrep.cpp @@ -10,7 +10,10 @@ R_altrep_class_t RelToAltrep::logical_class; R_altrep_class_t RelToAltrep::int_class; R_altrep_class_t RelToAltrep::real_class; R_altrep_class_t RelToAltrep::string_class; + +#if defined(R_HAS_ALTLIST) R_altrep_class_t RelToAltrep::list_class; +#endif void RelToAltrep::Initialize(DllInfo *dll) { // this is a string so setting row names will not lead to materialization @@ -19,33 +22,37 @@ void RelToAltrep::Initialize(DllInfo *dll) { int_class = R_make_altinteger_class("reltoaltrep_int_class", "duckdb", dll); real_class = R_make_altreal_class("reltoaltrep_real_class", "duckdb", dll); string_class = R_make_altstring_class("reltoaltrep_string_class", "duckdb", dll); - list_class = R_make_altlist_class("reltoaltrep_list_class", "duckdb", dll); R_set_altrep_Inspect_method(rownames_class, RownamesInspect); R_set_altrep_Inspect_method(logical_class, RelInspect); R_set_altrep_Inspect_method(int_class, RelInspect); R_set_altrep_Inspect_method(real_class, RelInspect); R_set_altrep_Inspect_method(string_class, RelInspect); - R_set_altrep_Inspect_method(list_class, RelInspect); R_set_altrep_Length_method(rownames_class, RownamesLength); R_set_altrep_Length_method(logical_class, VectorLength); R_set_altrep_Length_method(int_class, VectorLength); R_set_altrep_Length_method(real_class, VectorLength); R_set_altrep_Length_method(string_class, VectorLength); - R_set_altrep_Length_method(list_class, VectorLength); R_set_altvec_Dataptr_method(rownames_class, RownamesDataptr); R_set_altvec_Dataptr_method(logical_class, VectorDataptr); R_set_altvec_Dataptr_method(int_class, VectorDataptr); R_set_altvec_Dataptr_method(real_class, VectorDataptr); R_set_altvec_Dataptr_method(string_class, VectorDataptr); - R_set_altvec_Dataptr_method(list_class, VectorDataptr); R_set_altvec_Dataptr_or_null_method(rownames_class, RownamesDataptrOrNull); R_set_altstring_Elt_method(string_class, VectorStringElt); + +#if defined(R_HAS_ALTLIST) + list_class = R_make_altlist_class("reltoaltrep_list_class", "duckdb", dll); + R_set_altrep_Inspect_method(list_class, RelInspect); + R_set_altrep_Length_method(list_class, VectorLength); + R_set_altvec_Dataptr_method(list_class, VectorDataptr); R_set_altlist_Elt_method(list_class, VectorListElt); +#endif + } template @@ -237,11 +244,13 @@ SEXP RelToAltrep::VectorStringElt(SEXP x, R_xlen_t i) { END_CPP11 } +#if defined(R_HAS_ALTLIST) SEXP RelToAltrep::VectorListElt(SEXP x, R_xlen_t i) { BEGIN_CPP11 return VECTOR_ELT(AltrepVectorWrapper::Get(x)->Vector(), i); END_CPP11 } +#endif static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) { switch (type.id()) { @@ -273,8 +282,12 @@ static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) { case LogicalTypeId::VARCHAR: case LogicalTypeId::UUID: return RelToAltrep::string_class; + +#if defined(R_HAS_ALTLIST) case LogicalTypeId::LIST: return RelToAltrep::list_class; +#endif + default: cpp11::stop("rel_to_altrep: Unknown column type for altrep: %s", type.ToString().c_str()); } diff --git a/tests/testthat/test_list.R b/tests/testthat/test_list.R index 249b42bf8..c03b9fedc 100644 --- a/tests/testthat/test_list.R +++ b/tests/testthat/test_list.R @@ -28,6 +28,8 @@ test_that("one-level lists can be read", { }) test_that("rel_filter() handles LIST logical type", { + skip_if_not(getRversion() >= "4.3.0") + con <- dbConnect(duckdb()) on.exit(dbDisconnect(con, shutdown = TRUE)) From 167880eb2f15169b6a1c59d624de901b6a888ed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Fri, 23 Feb 2024 16:14:43 +0100 Subject: [PATCH 5/5] Update src/reltoaltrep.cpp --- src/reltoaltrep.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reltoaltrep.cpp b/src/reltoaltrep.cpp index c19cfa0b3..53ce94fda 100644 --- a/src/reltoaltrep.cpp +++ b/src/reltoaltrep.cpp @@ -294,7 +294,7 @@ static R_altrep_class_t LogicalTypeToAltrepType(const LogicalType &type) { } [[cpp11::register]] SEXP rapi_rel_to_altrep(duckdb::rel_extptr_t rel) { - D_ASSERT(rel && rel->rel); + D_ASSERT(rel && rel->rel); auto drel = rel->rel; auto ncols = drel->Columns().size();