expr_constant(, typed_logical_null = )
#161
Conversation
| exprs <- list( | ||
| y = expr_constant(NA, TRUE) | ||
| ) | ||
| expect_equal(rel_to_altrep(rel_project(rel1, exprs))[, ], NA) |
There was a problem hiding this comment.
Is there a more straightforward way to convert a constant to a SEXP rather that having to round trip through a data frame ?
There was a problem hiding this comment.
For our use case, this is best. You could add a wrapper function in this package?
There was a problem hiding this comment.
A function that turns a constant into an R value ?
| @@ -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) { | |||
There was a problem hiding this comment.
I believe this should instead not have the typed_logical_null argument, handle val == R_NilValue upfront, and use RApiTypes::SexpToValue(val, 0, true)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| exprs <- list( | ||
| y = expr_constant(NA, TRUE) | ||
| ) | ||
| expect_equal(rel_to_altrep(rel_project(rel1, exprs))[, ], NA) |
There was a problem hiding this comment.
For our use case, this is best. You could add a wrapper function in this package?
| @@ -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) { | |||
There was a problem hiding this comment.
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.
This is related to hannes/duckdb-rfuns#91 (comment)
This allows
expr_constant()to construct a NULL constant of type LOGICAL which wasn't possible before.Perhaps a better alternative would be to allow
expr_constant(NULL)to construct aSQLNULLand letexpr_constant(NA)to construct aValue(LogicalType::BOOLEAN)?expr_constant(NULL)is currently an error