Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
GianlucaFicarelli
approved these changes
Mar 19, 2026
Collaborator
GianlucaFicarelli
left a comment
There was a problem hiding this comment.
LGTM, my only concern is for performance:
- when ordering by an attribute in a nested model, the select query needs to join or left join with additional tables, and sort on that attribute to be able to select the desired page of results. If there are many records and there isn't a good index to use, the query might be not very efficient.
Example of queries:
- without nested ordering:
SELECT entity.type ... ORDER BY entity.creation_date DESC, cell_morphology.id - with nested ordering:
SELECT entity.type ... LEFT OUTER JOIN (entity AS entity_3 JOIN subject AS subject_2 ON entity_3.id = subject_2.id) ON scientific_artifact.subject_id = subject_2.id ... ORDER BY subject_2.name ASC, cell_morphology.id
- the additional joins are added also to the query that counts the records, although it's not needed. This is because the ordering attributes are handled together with the filtering attributes to determine the join to apply here.
Example of queries:
- without nested ordering:
SELECT count(distinct(cell_morphology.id)) ... - with nested ordering:
SELECT count(distinct(cell_morphology.id)) ... LEFT OUTER JOIN (entity AS entity_1 JOIN subject AS subject_1 ON entity_1.id = subject_1.id) ON scientific_artifact.subject_id = subject_1.id
We can check in staging if these queries are taking longer, and decide if it's worth to fix the count query by ignoring in that case the joins that are needed only for ordering and not for filtering.
Contributor
Author
|
Thank you for the feedback, that is what I was worried about. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The GUI is limited by the fact that many fields are not sortable with entity core REST API.
This is adding many fields for ordering.