-
Notifications
You must be signed in to change notification settings - Fork 132
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
Query optimization for the Briefcase viewset #2142
Conversation
489a4a6
to
2b4efe0
Compare
Avoid multiple calls to `.count()` which case the application to re-query the database
Minor optimization for the `SELECT` query as we do not utilize any of the other columns within the instance table in the BriefcaseViewSet
…ueryset Not using the `num_of_submissions` attribute since it counts submissions which have not yet received all media files yet.
b4c3def
to
031fa06
Compare
deleted_at__isnull=True) | ||
instances = Instance.objects.filter( | ||
xform=xform, deleted_at__isnull=True).values( | ||
'pk', 'uuid') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using the uuid
anywhere? We could just only return the pk
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we're using it on the list
endpoint https://github.com/onaio/onadata/blob/b406899b61/onadata/apps/logger/templates/submissionList.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, forgot about that.
deleted_at__isnull=True) | ||
instances = Instance.objects.filter( | ||
xform=xform, deleted_at__isnull=True).values( | ||
'pk', 'uuid') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, forgot about that.
Changes / Features implemented
num_of_submissions
instead of trying to count all the instances whennumEntries
is not specifiedinstances
usinglen()
instead ofcount()
to avoid an additional Database query & preload theinstances
before the renderpk
&uuid
columns from the database instead of all the instance table columns; The briefcase viewset does not utilize any of the other columnsSteps taken to verify this change does what is intended
Side effects of implementing this change
Improved performance on the briefcase viewset. Performance gain
Tests were done on a form with 4601 submissions
With changes & numEntries query param
Without changes & numEntries query param
With changes & No query param
Without changes & No query param
Before submitting this PR for review, please make sure you have:
Closes #