TranQL query optimization, loading error/state improvements, variable view color fix#325
TranQL query optimization, loading error/state improvements, variable view color fix#325frostyfan109 wants to merge 6 commits intodevelopfrom
Conversation
frostyfan109
commented
Oct 28, 2024
frostyfan109
commented
- Related concept & related variable queries are now performed as a single TranQL query per CDE. Before, related studies required 1 query and related concepts required 5 queries for a total of 6 queries per CDE.
- Adds loading/error states to the studies and knowledge graph tabs in the concept model. Previously, it seemed they were relying on their API requests completing fairly quickly and not failing.
- Fixes coloring of study sources in the variable view for certain searches.
- Fixes context to support capitalized "CDE" study/data sources (currently "cde" for both)
…source tags, small bug fixes
gaurav
left a comment
gaurav
left a comment
There was a problem hiding this comment.
I don't have the technical depth to evaluate these changes, but I was able to run the code and confirm that things seem to be working well (and very fast!) and only have two minor comments. Good job!
| if (e.name !== "CanceledError") throw e | ||
| if (e.name !== "CanceledError") { | ||
| console.warning(e) | ||
| setStudies(null) |
There was a problem hiding this comment.
Would it be useful to propagate the exception?
There was a problem hiding this comment.
Not in this scenario. The exception is "implicitly" handled by setting the state to null, indicating an error state.
| { | ||
| headers: { 'Content-Type': 'text/plain' }, | ||
| method: 'POST', | ||
| body: `SELECT publication->named_thing FROM "redis:" WHERE publication="${cdeId}"`, |
There was a problem hiding this comment.
Be careful that this could pick up (1) things that aren't concepts, or (2) concepts that we haven't loaded. For example, when I tried running this locally, I found the related concept "Rheum", which we haven't actually loaded, so trying to select it (via "Open") resulted in a synonymousConcepts is undefined error.
I prefer the approach we were using previously, which is to limit this query to certain types (['disease', 'anatomical_entity', 'phenotypic_feature', 'biological_process'] in https://github.com/helxplatform/helx-ui/pull/325/files#diff-e11890aa96a2d27536af10ab83b21362d569d6467f13afd9f6792b8196bd65afL2400). I'm not sure if this will fix this problem entirely, but I think it would be helpful.
There was a problem hiding this comment.
Hmm, good point. I considered that it could possibly lead to unindexed concepts from unrestricting the types, but I didn't find in an example of such a thing happening during my testing.
There was a problem hiding this comment.
I'll note that querying against these specific types in tranql isn't going to fix the issue of getting results back that aren't indexed in dug. This can be done by checking the concept returned by tranql is linked to a study variable, but this also isn't particularly practical. There was a bug with the concept lookup mechanism (used here) which I've fixed now, so you shouldn't be seeing that error anymore.
Really, what needs to happen is the addition of a new Dug endpoint that let's you pull any documents out of the concept index by ID. Then we could do a subsequent lookup against the results returned by tranql and filter out any that aren't indexed. It's just that this feature isn't particularly prominent so the work hasn't been done.
2 participants
