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] Bulk load SessionID and VisitLabel #8855

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

driusan
Copy link
Collaborator

@driusan driusan commented Aug 8, 2023

When calling bulkLoadInstanceData, currently only instrument data is bulk loaded from the database. However, calling getSessionID or getVisitLabel is almost always required in order to use the data. This bulk loads the session id and visit label from the same query when calling bulkLoadInstanceData in order to further optimize it.

When calling bulkLoadInstanceData, currently only instrument data is
bulk loaded from the database. However, calling getSessionID or getVisitLabel
is almost always required in order to use the data. This bulk loads the
session id and visit label from the same query when calling bulkLoadInstanceData.
@driusan driusan added the php Pull requests that update Php code label Aug 8, 2023
@ridz1208 ridz1208 self-assigned this Oct 13, 2023
/**
* Gets the VisitLabel for the timepoint for this instrument.
*
* @return string The visit label
*/
function getVisitLabel(): string
{
return \TimePoint::singleton($this->getSessionID())->getVisitLabel();
if ($this->visitLabel === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just curious, why is this done in a lazy way instead of being instantiated in the factory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know, I didn't want to change the behaviour so I left if the way it was and just added caching.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense :)

@laemtl laemtl self-requested a review November 13, 2023 15:27
@driusan driusan merged commit 57379f2 into aces:main Nov 13, 2023
19 checks passed
@ridz1208 ridz1208 added this to the 26.0.0 milestone Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants