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

[ALS-4947] Add anyRecordOfMulti field to the query object #77

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

ramari16
Copy link
Contributor

@ramari16 ramari16 commented Aug 28, 2023

This new field supports separate lists of OR filters where the result of each is ANDed with each other. I also removed some obsolete/redundant java generics

Comment on lines -263 to +276
private void addIdSetsForAnyRecordOf(Query query, ArrayList<Set<Integer>> filteredIdSets) {
if(!query.getAnyRecordOf().isEmpty()) {
Set<Integer> patientsInScope = new ConcurrentSkipListSet<Integer>();
VariantBucketHolder<VariantMasks> bucketCache = new VariantBucketHolder<VariantMasks>();
query.getAnyRecordOf().parallelStream().forEach(path->{
if(patientsInScope.size()<Math.max(
phenotypeMetaStore.getPatientIds().size(),
variantService.getPatientIds().length)) {
if(VariantUtils.pathIsVariantSpec(path)) {
addIdSetsForVariantSpecCategoryFilters(new String[]{"0/1","1/1"}, path, patientsInScope, bucketCache);
} else {
patientsInScope.addAll(getCube(path).keyBasedIndex());
}
private void addIdSetsForAnyRecordOf(List<String> anyRecordOfFilters, ArrayList<Set<Integer>> filteredIdSets) {
if(!anyRecordOfFilters.isEmpty()) {
VariantBucketHolder<VariantMasks> bucketCache = new VariantBucketHolder<>();
Set<Integer> anyRecordOfPatientSet = anyRecordOfFilters.parallelStream().flatMap(path -> {
if (VariantUtils.pathIsVariantSpec(path)) {
TreeSet<Integer> patientsInScope = new TreeSet<>();
addIdSetsForVariantSpecCategoryFilters(new String[]{"0/1", "1/1"}, path, patientsInScope, bucketCache);
return patientsInScope.stream();
} else {
return (Stream<Integer>) getCube(path).keyBasedIndex().stream();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to add all patients matching any path to patientsInScope which is a hacky way of doing OR.

The new logic creates a set with all patients matching any of the paths (i.e., they have any record of that path) and adds it to filteredIdSets, which is a list of sets that eventually get ANDed with each other. We can then call this method multiple times with multiple different lists of anyRecordOf filters to support AND/OR functionality in this very specific way.

Copy link
Contributor

@srpiatt srpiatt left a comment

Choose a reason for hiding this comment

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

I think what you're trying to accomplish would make more sense to me if there were unit tests. So, aside from just proofreading for any obvious errors, I don't know if any of this works how you're intending.

@ramari16 ramari16 merged commit 3dc96b5 into release/0002 Aug 30, 2023
@ramari16 ramari16 deleted the feature/ALS-4947 branch August 30, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants