Repository navigation
[dataquery] Fetch accessible instruments in a single query in the dictionary module - #11167
Conversation
667b993 to
429e3fc
Compare
429e3fc to
32fa8b6
Compare
|
@marandmart can you take a look at this when you have the time? |
|
@HenriRabalais can you please rebase this to 29.0-release |
Move the per-visit instrument lookup out of the dictionary module and into a batched Utility::getVisitsInstruments() helper that fetches the distinct instruments for all of the users visits in one query, instead of running Utility::getVisitInstruments() once per visit label.
32fa8b6 to
babdda2
Compare
| FROM session s | ||
| JOIN candidate c ON (c.ID=s.CandidateID) | ||
| JOIN flag f ON (f.sessionID=s.id) | ||
| JOIN test_names t ON (f.TestID=t.ID) |
There was a problem hiding this comment.
This table is not accurate for the full name. The source of truth for the full name is NDB_BVL_Instrument::getFullName(). (For instance, for LINST instruments it comes from parsing the file, and for a php instrument it could just return a hardcoded string.)
I think you need to:
- Not join this table (it's unnecessary)
- Call the
NDB_BVL_Instrument::factory(testname)->getFullName()for each
(This shouldn't be slower, unless there's too many instruments using LegacyInstrumentTrait instead of implementing getFullName() themselves..)
I also think this function should be moved into the dictionary module where it's used or inlined (not for efficiency, just because having both getVisitsInstruments and getVisitInstruments next to each other in the Utility class is confusing, and there is only one call site.. also you have access to the LorisInstance object there to get a database connection.)
There was a problem hiding this comment.
thanks for the review!
I've now realized the full name isn't used at this call site at all. getUserModuleCategories() only checks whether each instrument is in the list, and the category names already come from getFullName() in InstrumentQueryEngine::getDataDictionary(), so there's nothing to call getFullName() for. I've removed Full_name from the query. I kept the join to test_names only to get Test_name, since flag just has TestID, so let me know if you had another way of getting the name in mind.
I moved the query into getUserModuleCategories() and removed Utility::getVisitsInstruments(), as you asked.
Description
/dictionary/categories(and/dictionary/module/<name>) scaled linearly with the number of distinct visit labels in the database.getUserModuleCategories()built the set of instruments the user can access by running oneUtility::getVisitInstruments()query per visit label, and each of those queries joinssession,candidate,flagandtest_names.This replaces the per-visit loop with a single query that fetches the distinct instrument names across all of the user's visits. The query returns names only, since the result is only used to check whether an instrument is in the list.
Changes
foreachloop inLORIS\dictionary\Module::getUserModuleCategories()with a single query over all of the user's visitsTest_nameonly, soFull_nameis no longer fetchedTesting Instructions
session.Visit_labelvalues.GET /dictionary/categoriesdirectly.Related Issues
Closes #11166. The slow loop was introduced in #9903.