Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expr_constant(, typed_logical_null = ) #161

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

expr_constant(, typed_logical_null = ) #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions R/cpp11.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ rapi_expr_reference <- function(rnames) {
.Call(`_duckdb_rapi_expr_reference`, rnames)
}

rapi_expr_constant <- function(val) {
.Call(`_duckdb_rapi_expr_constant`, val)
rapi_expr_constant <- function(val, typed_logical_null) {
.Call(`_duckdb_rapi_expr_constant`, val, typed_logical_null)
}

rapi_expr_function <- function(name, args, order_bys, filter_bys) {
Expand Down
7 changes: 6 additions & 1 deletion R/relational.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,17 @@ expr_reference <- function(names, table = NULL) {

#' Create a constant expression
#' @param val the constant value
#' @param typed_logical_null Whether NA is converted to a LOGICAL NULL or a SQLNULL
#' @return a constant expression
#' @noRd
#' @examples
#' const_int_expr <- expr_constant(42)
#' const_str_expr <- expr_constant("Hello, World")
expr_constant <- rapi_expr_constant
#' const_sql_null_expr <- expr_constant(NA)
#' const_lgl_null_expr <- expr_constant(NA, TRUE)
expr_constant <- function(val, typed_logical_null = FALSE) {
rapi_expr_constant(val, typed_logical_null)
}

#' Create a function call expression
#' @param name the function name
Expand Down
2 changes: 1 addition & 1 deletion man/duckdb.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions src/cpp11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,10 @@ extern "C" SEXP _duckdb_rapi_expr_reference(SEXP rnames) {
END_CPP11
}
// relational.cpp
SEXP rapi_expr_constant(sexp val);
extern "C" SEXP _duckdb_rapi_expr_constant(SEXP val) {
SEXP rapi_expr_constant(sexp val, bool typed_logical_null);
extern "C" SEXP _duckdb_rapi_expr_constant(SEXP val, SEXP typed_logical_null) {
BEGIN_CPP11
return cpp11::as_sexp(rapi_expr_constant(cpp11::as_cpp<cpp11::decay_t<sexp>>(val)));
return cpp11::as_sexp(rapi_expr_constant(cpp11::as_cpp<cpp11::decay_t<sexp>>(val), cpp11::as_cpp<cpp11::decay_t<bool>>(typed_logical_null)));
END_CPP11
}
// relational.cpp
Expand Down Expand Up @@ -424,7 +424,7 @@ static const R_CallMethodDef CallEntries[] = {
{"_duckdb_rapi_disconnect", (DL_FUNC) &_duckdb_rapi_disconnect, 1},
{"_duckdb_rapi_execute", (DL_FUNC) &_duckdb_rapi_execute, 3},
{"_duckdb_rapi_execute_arrow", (DL_FUNC) &_duckdb_rapi_execute_arrow, 2},
{"_duckdb_rapi_expr_constant", (DL_FUNC) &_duckdb_rapi_expr_constant, 1},
{"_duckdb_rapi_expr_constant", (DL_FUNC) &_duckdb_rapi_expr_constant, 2},
{"_duckdb_rapi_expr_function", (DL_FUNC) &_duckdb_rapi_expr_function, 4},
{"_duckdb_rapi_expr_reference", (DL_FUNC) &_duckdb_rapi_expr_reference, 1},
{"_duckdb_rapi_expr_set_alias", (DL_FUNC) &_duckdb_rapi_expr_set_alias, 2},
Expand Down
4 changes: 2 additions & 2 deletions src/relational.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ external_pointer<T> make_external_prot(const string &rclass, SEXP prot, ARGS &&.
return make_external<ColumnRefExpression>("duckdb_expr", names);
}

[[cpp11::register]] SEXP rapi_expr_constant(sexp val) {
[[cpp11::register]] SEXP rapi_expr_constant(sexp val, bool typed_logical_null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should instead not have the typed_logical_null argument, handle val == R_NilValue upfront, and use RApiTypes::SexpToValue(val, 0, true)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? I just changed this to typed_logical_null := false to support a special case in duckdb. I need to better understand the original motivation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe expr_constant(NA) should make a constant that is a NULL LOGICAL, not a NULL SQL, the same way expr_constant(NA_integer_) makes a NULL INTEGER constant.

And then we could tweak expr_constant() so that expr_constant(NULL) would return a SQLNULL

With this PR, if we wanted to construct a LOGICAL NULL we need to expr_constant(NA, TRUE).

I perhaps miss background about the special case that warranted for using typed_logical_null := false

if (LENGTH(val) != 1) {
stop("expr_constant: Need value of length one");
}
return make_external<ConstantExpression>("duckdb_expr", RApiTypes::SexpToValue(val, 0, false));
return make_external<ConstantExpression>("duckdb_expr", RApiTypes::SexpToValue(val, 0, typed_logical_null));
}

[[cpp11::register]] SEXP rapi_expr_function(std::string name, list args, list order_bys, list filter_bys) {
Expand Down
1 change: 1 addition & 0 deletions tests/testthat/test-list.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,4 @@ test_that("rel_filter() handles LIST logical type", {
df2 <- rel_to_altrep(rel2)
expect_equal(df1$a, df2$a)
})

7 changes: 7 additions & 0 deletions tests/testthat/test-relational.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,13 @@ test_that("we can create various expressions and don't crash", {
expect_true(TRUE)
})

test_that("we can create a constant NA with LOGICAL type", {
rel1 <- rel_from_df(con, data.frame(x = 42))
exprs <- list(
y = expr_constant(NA, TRUE)
)
expect_equal(rel_to_altrep(rel_project(rel1, exprs))[, ], NA)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more straightforward way to convert a constant to a SEXP rather that having to round trip through a data frame ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For our use case, this is best. You could add a wrapper function in this package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A function that turns a constant into an R value ?

})

# TODO should maybe be a different file, test_enum_strings.R

Expand Down
Loading