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

RAS-1400 use native sql #450

Merged
merged 4 commits into from
Jan 9, 2025
Merged

RAS-1400 use native sql #450

merged 4 commits into from
Jan 9, 2025

Conversation

LJBabbage
Copy link
Contributor

@LJBabbage LJBabbage commented Jan 6, 2025

What and why?

The code in the model https://github.com/ONSdigital/ras-party/blob/main/ras_party/models/models.py#L40 will force join the attributes table, due to how we create business attributes for every individual sample entry, means there are literally thousands for some businesses. Because of that if we attempt any join using the Business model it will overwork and produce an excessive performance hitting amount of joins. This PR sides steps the model and creates a pure SQL of exactly what we want

How to test?

Create enrolments for different CE's and make sure when you log in the correct information is displayed on the todo list. You can use acceptance-tests, but I would add another enrolled to them and possible another CE

Jira

@LJBabbage LJBabbage requested a review from a team as a code owner January 6, 2025 13:54
@SteveScorfield
Copy link
Contributor

/deploy scorfs

@ras-rm-pr-bot
Copy link
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-450

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

Copy link
Contributor

@SteveScorfield SteveScorfield left a comment

Choose a reason for hiding this comment

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

I have tested the code and works as expected. The acceptance tests pass. I like this approach. It's more streamlined. Approved.

@pricem14pc
Copy link
Contributor

pricem14pc commented Jan 8, 2025

  • reviewed code
  • tested locally in docker and in dev environments(run time and build) and successfully tested multiple enrollment permutations
  • checked cross-app usage of v1/enrolments/respondent is for a limited number of frontstage use cases only
  • tested a number of preprod use cases for large enrolments
  • native query correctly returning significantly less rows than the inefficient SQLAlchemy query (i.e. no longer outer join to respondent/respondent_business returning rogue rows for other respondents)
  • tested the query explain plan and it no longer needs to create expensive hash tables or nested outer joins

Copy link
Contributor

@pricem14pc pricem14pc left a comment

Choose a reason for hiding this comment

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

Happy with the current improvements and have discussed progressing the outstanding comments at a later date.

@LJBabbage LJBabbage merged commit d799464 into main Jan 9, 2025
1 check passed
@LJBabbage LJBabbage deleted the update-enrolment-query branch January 9, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants