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

[Instruments] Instrument data QueryEngine #8216

Merged
merged 15 commits into from
Apr 26, 2023

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Nov 4, 2022

This adds a QueryEngine for the instruments module. The Instrument
QueryEngine bulk loads the data so that it can work with either JSON
or SQL instruments, and then compares the data that was loaded
against the criteria given.

This is intended to be an example of a QueryEngine for PR#8215.

It could use more automated tests but tests for instruments are difficult
since they're so highly customized per project.

@driusan driusan changed the title Instruments qe [Instruments] Instrument data QueryEngine Nov 4, 2022
@driusan driusan added the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label Nov 7, 2022
@driusan driusan force-pushed the InstrumentsQE branch 4 times, most recently from e7cccae to 4f8b9b0 Compare November 7, 2022 18:07
@driusan driusan force-pushed the InstrumentsQE branch 2 times, most recently from 088cf8a to cf9f16d Compare December 7, 2022 15:45
@driusan driusan added Feature PR or issue introducing/requiring at least one new feature and removed Blocked PR awaiting the merge, resolution or rejection of an other Pull Request labels Dec 7, 2022
@driusan
Copy link
Collaborator Author

driusan commented Dec 7, 2022

Blocked by #8260, which adds the getQueryEngine function and removes getDataDictionary from the module descriptor class.

@driusan driusan added the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label Dec 7, 2022
This adds a Module->getQueryEngine() function to the Module class to
get the QueryEngine to the module. The default is a NullQueryEngine
which does nothing and matches nothing.

Update the dictionary module to use the new interface instead of
Module->getDataDictionary().
This adds a QueryEngine for the instruments module. The Instrument
QueryEngine bulk loads the data so that it can work with either JSON
or SQL instruments, and then compares the data that was loaded
against the criteria given.
@driusan driusan removed the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label Jan 24, 2023
@laemtl laemtl added this to the 25.0.0 milestone Mar 7, 2023
@ridz1208 ridz1208 self-assigned this Mar 21, 2023
@driusan driusan changed the base branch from main to 25.0-release March 31, 2023 17:06
@laemtl laemtl added Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch and removed Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch labels Apr 4, 2023
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

partial review

modules/instruments/php/instrumentqueryengine.class.inc Outdated Show resolved Hide resolved
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

I havn't manage to make it populated the dictionary module. Should it?

@xlecours
Copy link
Contributor

xlecours commented Apr 5, 2023

More on a design perspective, I have doubts regarding the Instantiation of an instrument for each row in the flag table and keeping them in memory then applying the filtering using Criteria*.
Could the bulkLoadInstrument yield lightweight objects that could be pass to Criteria and Instantiate Instruments after that if necessary? Something like FilterIterator with an accept function.

@driusan
Copy link
Collaborator Author

driusan commented Apr 5, 2023

  1. Yes, the dictionary module should be populated. Any instruments you have in your test_names table should be there.
  2. In my testing, the bulkLoadInstanceData wasn't heavy. The overhead comes from repeated calls to \NDB_Factory (and all the SQL calls it makes) which bulkLoad avoids, not the memory requirements of holding the instrument's data in memory. We can revisit later if it proves to be a problem, but I was able to handle fairly large instruments without memory problems when I wrote/tested this. We need to handle both JSON data and SQL table-based instruments, so we always need the instantiated instruments and the overhead of having to call NDB_Factory shadows the overhead of doing it once and keeping the data in memory.

@xlecours xlecours added the Critical to release PR or issue is key for the release to which it has been assigned label Apr 11, 2023
Copy link
Contributor

@xlecours xlecours left a comment

Choose a reason for hiding this comment

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

I had a project/modules/instruments ... my bad.
This is working just right.

@driusan driusan merged commit b6bb6a7 into aces:25.0-release Apr 26, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical to release PR or issue is key for the release to which it has been assigned Feature PR or issue introducing/requiring at least one new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants