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.

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

Conversation

romainfrancois
Copy link
Contributor

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 a SQLNULL and let expr_constant(NA) to construct a Value(LogicalType::BOOLEAN) ?

expr_constant(NULL) is currently an error

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 ?

@@ -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

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, Romain!

exprs <- list(
y = expr_constant(NA, TRUE)
)
expect_equal(rel_to_altrep(rel_project(rel1, exprs))[, ], NA)
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?

@@ -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
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.

@romainfrancois romainfrancois mentioned this pull request May 8, 2024
@romainfrancois romainfrancois marked this pull request as draft May 16, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants