Add include/exclude logic for database service #262
Conversation
aswallace
requested review from
SeanLeRoy,
BrianWhitneyAI,
kmitcham and
pgarrison
and removed request for
SeanLeRoy
| @@ -202,7 +202,12 @@ export default class FileSet { | |||
| Object.entries(filterValuesByAnnotation).forEach(([annotation, filterValues]) => { | |||
| // If a filter value is `null` then we need to modify the way we approach filtering | |||
| // it in SQL | |||
| if (filterValues.length === 0) { | |||
| if (!!this.excludeFilters?.some((filter) => filter.name === annotation)) { | |||
| sqlBuilder.where(`"${annotation}" IS NULL`); | |||
There was a problem hiding this comment.
I think I'm missing some context for this change. If this PR is about adding the include/exclude filters, why does FileSet already have the excludeFilters and includeFilters variables? And why weren't they used in toQuerySQLBuilder before?
There was a problem hiding this comment.
The main logic for include, exclude, and fuzzy filters was added in #238 and remains the same. However, the emphasis for that ticket was to integrate with FES, which we query via URLs and not through our database service (which is how we query other data sources, e.g., CSVs).
The toQuerySQLBuilder is ONLY used by our database service, not by FES, so to prevent #238 from getting more bloated than it already was, I left it to be a separate ticket.
There was a problem hiding this comment.
Okay, thanks. So it looks like toQueryString is for FES and toQuerySQLBuilder is for database services?
On line 176, FileSet.toQueryString delegates to filter.toQueryString: why not follow the same pattern for toQuerySQLBuilder and leave it up to the filter class to decide the correct SQL? If I am reading correctly, this would remove the need for the excludeFilters and includeFilters variables in FileSet
There was a problem hiding this comment.
fwiw I'm quite sure I had a justification for not delegating to the class which I no longer remember. I think it had to do with the way we're mapping the filters to a grouped list for sql that made it seem like it would be complicated... I'll try it though and see if it comes back to me
There was a problem hiding this comment.
Whatever you end up doing, I think my feedback for this PR would be to just use a comment to explain the decision
There was a problem hiding this comment.
I tried to reach a middle ground on this in 11f2e05. Not 100% sure I fully addressed your concerns though, so lmk if you have more questions
packages/core/services/AnnotationService/DatabaseAnnotationService/index.ts
Outdated
Show resolved
Hide resolved
| }); | ||
| const filter = new FileFilter("bar", "barValue", FilterType.ANY); | ||
| const values = await annotationService.fetchRootHierarchyValues(["foo"], [filter]); | ||
| expect(values).to.deep.equal(["Cell Line0", "Is Split Scene1", "Whatever2"]); |
There was a problem hiding this comment.
This is more an issue with the overall test structure than this PR, but I can't really evaluate this test from the diff alone. I think I'd need to know where to find the data source named "e"? Without looking at that, I'm trusting that you've picked values on this line that make sense.
There was a problem hiding this comment.
These tests were essentially copy-pastes of the existing tests in this suite. Will update to clarify values
.../services/AnnotationService/DatabaseAnnotationService/test/DatabaseAnnotationService.test.ts
Outdated
Show resolved
Hide resolved
packages/core/services/AnnotationService/DatabaseAnnotationService/index.ts
Outdated
Show resolved
Hide resolved
packages/core/services/AnnotationService/DatabaseAnnotationService/index.ts
Outdated
Show resolved
Hide resolved
This adds include/exclude logic for external files that DON'T use FES (e.g., use database service with SQL queries).
Edit for context: the logic for FES has already been merged via #238
This does NOT implement fuzzy vs. exact search toggling yet for non-FES sources. Because of this, I'm conditionally hiding/disabling the fuzzy toggle in the UI element for non-AICS data sources.
Testing
Tested manually for
any valuefilters andno valuefilters using a test csv that contains null values.There's also some very simple unit tests to make sure things still run with typed filters