Draft
Conversation
The two dataloaders (linked_to_editions_source and reverse_linked_to_editions_source) were doing basically the same thing, but with slightly different sql for direct and reverse links. We can combine the SQL for both into a single giant four way UNION ALL if we take the direction of the links as query input. This is a slight performance win because of the way that the dataloaders batch up requests. If the same dataloader supports both direct and reverse links, sometimes that can save us a few SQL queries. For example, for /government/ministers/lord-in-waiting-government-whip--13 before this change we had to make a separate query to look up the reverse role link to find the role_appointment. After this change, we can combine this with the request to get the direct links (ordered_parent_organisations, organisations, taxons), making one fewer SQL query. In practice, this doesn't seem to be much of a performance win or loss, and it trades off reducing a bit of duplicated ruby and SQL code with creating one giant 163 line SQL query. The secret agenda behind it is that the resulting SQL query is pretty much what we'd need if we were to use a recursive CTE instead of using the dataloaders to batch editions. This could reduce the number of queries we're making by ~4 or 5, depending on how deep the links tree is.
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
1 participant
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 two dataloaders (linked_to_editions_source and reverse_linked_to_editions_source) were doing basically the same thing, but with slightly different sql for direct and reverse links.
We can combine the SQL for both into a single giant four way UNION ALL if we take the direction of the links as query input.
This is a slight performance win because of the way that the dataloaders batch up requests. If the same dataloader supports both direct and reverse links, sometimes that can save us a few SQL queries.
For example, for /government/ministers/lord-in-waiting-government-whip--13 before this change we had to make a separate query to look up the reverse role link to find the role_appointment. After this change, we can combine this with the request to get the direct links (ordered_parent_organisations, organisations, taxons), making one fewer SQL query.
In practice, this doesn't seem to be much of a performance win or loss, and it trades off reducing a bit of duplicated ruby and SQL code with creating one giant 163 line SQL query.
The secret agenda behind it is that the resulting SQL query is pretty much what we'd need if we were to use a recursive CTE instead of using the dataloaders to batch editions. This could reduce the number of queries we're making by ~4 or 5, depending on how deep the links tree is.
Draft for now because I'm not convinced this is a good idea.