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

is.na() : handle FLAT and CONSTANT #91

Merged
merged 4 commits into from
May 16, 2024
Merged

is.na() : handle FLAT and CONSTANT #91

merged 4 commits into from
May 16, 2024

Conversation

romainfrancois
Copy link
Collaborator

@romainfrancois romainfrancois commented May 7, 2024

closes #89

@romainfrancois romainfrancois changed the title handle FLAT and CONSTANT is.na() : handle FLAT and CONSTANT May 7, 2024
expect_false(is_na_constant(FALSE))

skip("I don't know why it returns NA: will investigate")
expect_true(is_na_constant(NA))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to investigate what happens here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what's happening: duckdb/duckdb-r#161 we get a SQLNULL and that perhaps does not go though isna_any

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After some investigation, it appears we don't get to call the function at all when value is a SQLNULL, even if we add a bind step.

So I believe we should instead allow creation of a LOGICAL NULL constant in duckdb/duckdb-r#161 and keep skip() in the meantime

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hannes: How does propagation of NULL values work? The is.na() function is special in that it needs to return TRUE if passed an SQLNULL, do we need to define this function in a special way?

We need a function here because is.na === is.na | is.nan for doubles and is.na for all other types.

@romainfrancois romainfrancois requested a review from krlmlr May 8, 2024 06:51
@romainfrancois
Copy link
Collaborator Author

Added some code for the !FLAT & ! CONSTANT case, which turned out to be easy enough after all. I don't know how to test it though. How would I get a vector that is neither FLAT not CONSTANT ?

@romainfrancois romainfrancois merged commit eecf9dd into main May 16, 2024
46 of 50 checks passed
@romainfrancois romainfrancois deleted the is_na_non_flat branch May 16, 2024 08:34
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.

r_base::is.na(): INTERNAL Error: Operation requires a flat vector but a non-flat vector was encountered
2 participants