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

SQL - use CTEs to avoid pagination issues with large tables #14557

Merged
merged 12 commits into from
Sep 11, 2024

Conversation

mike12345567
Copy link
Collaborator

Description

As the title - this updates our SQL generation to use a CTE, avoiding issues when attempting to jump to large page offsets.

This is required as the OFFSET parameter doesn't stop the rows before the offset from being selected, this includes our JSON aggregation and therefore this can perform poorly.

The CTE correctly picks out the rows that we are going to page on (including using our WHERE EXISTS step to filter the relationships) then selects the aggregations just for those rows.

This was not initially implemented as SQS could not support it yet.

@mike12345567 mike12345567 self-assigned this Sep 11, 2024
@mike12345567 mike12345567 requested a review from a team as a code owner September 11, 2024 12:28
@mike12345567 mike12345567 requested review from samwho and removed request for a team September 11, 2024 12:28
@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/m labels Sep 11, 2024
Copy link
Collaborator

@PClmnt PClmnt left a comment

Choose a reason for hiding this comment

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

Code LGTM, although I have not so much context on the overall goal

@mike12345567 mike12345567 merged commit e1d750b into master Sep 11, 2024
12 checks passed
@mike12345567 mike12345567 deleted the fix/sql-use-ctes branch September 11, 2024 15:05
@github-actions github-actions bot locked and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
firestorm Data/Infra/Revenue Team size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants