search

populationgenomics / metamist

Sample level metadata system
MIT License
1 stars 1 forks source link

Analysis query fixes #791

Closed dancoates closed 2 months ago

dancoates commented 2 months ago

Hopefully fixes #790

This changes from querying the cross product join of analyses and sequencing groups to using a group by and JSON_ARRAYAGG to reduce the number of rows that the large meta object is propagated to.

Also removes a broken and unused function and fixes a few places that were still referring to sample ids on analyses.

dancoates commented 2 months ago

I've updated to combine the queries, I found that GROUP_CONCAT actually works better than JSON_ARRAYAGG as it doesn't include nulls and makes it easier to split back out to ids without having to parse the json for each row

dancoates commented 2 months ago

Also probably worth noting that cohorts aren't actually in the graphql schema for analyses so even though this returns cohort ids you can only get them through the rest api, not sure if it would be worth adding to the schema, but probably out of scope of this change as it would require adding a new loader for cohorts

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 77.24%. Comparing base (d4b498c) to head (678d387). Report is 1 commits behind head on dev.

Files Patch % Lines
db/python/tables/analysis.py 78.57% 3 Missing :warning:
db/python/layers/analysis.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #791 +/- ## ========================================== + Coverage 76.98% 77.24% +0.26% ========================================== Files 157 157 Lines 13026 12967 -59 ========================================== - Hits 10028 10017 -11 + Misses 2998 2950 -48 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nevoodoo commented 2 months ago

I think this should also close #798 once it is merged into main :)