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

Implement boolean filter types in the Data Explorer #332

Merged
merged 4 commits into from
May 2, 2024

Conversation

jmcphers
Copy link
Contributor

This change adds support for the is_true and is_false filter types for the R data explorer backend. See posit-dev/positron#2949 for details.

@jmcphers jmcphers requested a review from DavisVaughan April 30, 2024 22:03
@@ -144,6 +144,14 @@
nzchar(col)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should I be expecting an update to filter_params in .ps.filter_rows()?

Even if no params, at least to show we are handling this case?

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 good question ... That mapping explicitly only contains the names of the operations that take parameters. Parameter free operations like "is null" are excluded.

https://github.com/posit-dev/amalthea/blob/171d64a8d619dd5511fbc8de0f37a4d40115c593/crates/ark/src/modules/positron/r_data_explorer.R#L67-L71

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I missed that bit thanks

@@ -144,6 +144,14 @@
nzchar(col)
}

.ps.filter_col.is_true <- function(col, params) {
col == TRUE
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is only applied on logical columns? i.e. could col ever not be a logical vector? (I would assume that since the frontend knows the column types, it should only present the user with applicable filter options)

Also, what do you want it to return in the NA case? NA == TRUE returns NA, so if the return value is allowed to be all 3 of true/false/na then that works I guess. I just immediately assumed it should be a vector of either TRUE or FALSE for what gets returned to the frontend.

If col is always logical and NAs are ok, a simplification for these would be to just return col and !col respectively.

If we want to remove NAs from the picture, col & !is.na(col) and !col & !is.na(col) would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output of this is ultimately fed to which, so NA is fine, but I agree removing them would be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(also, yes, the UI takes care of ensuring that these filter types only get applied to logical columns)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in cbf1030

@jmcphers jmcphers merged commit 833a617 into main May 2, 2024
1 check passed
@jmcphers jmcphers deleted the feature/boolean-filters branch May 2, 2024 19:17
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