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

Only select required columns from sql databases #15169

Open
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

adrinr
Copy link
Collaborator

@adrinr adrinr commented Dec 12, 2024

Description

Edit the SQL generation (for both external and sqs) in order to not include select *. Instead, pull only the required fields. This applies both on the requested table rows plus the linked relationships. This change will increase performance, not pulling non-needed data.

There is an exception for this when there is a formula field on the requested fields. Given that this formula might use other columns, we need to pull it all in order to compute them.

Launchcontrol

Smarter SQL select, requesting only the required data.

Copy link

linear bot commented Dec 12, 2024

Copy link

qa-wolf bot commented Dec 12, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/s labels Dec 12, 2024
@adrinr adrinr force-pushed the BUDI-8885/only-select-required-columns-from-sql-databases branch from 4fb5f8c to da92c3b Compare December 12, 2024 15:59
@adrinr adrinr changed the title Budi 8885/only select required columns from sql databases Only select required columns from sql databases Dec 13, 2024
@github-actions github-actions bot added size/m and removed size/s labels Dec 16, 2024
@adrinr adrinr force-pushed the BUDI-8885/only-select-required-columns-from-sql-databases branch from 2bfcb11 to 7f3b73f Compare December 17, 2024 10:31
@github-actions github-actions bot added size/l and removed size/m labels Dec 18, 2024
Base automatically changed from chore/guard-display-column-in-the-api to master December 19, 2024 11:30
@github-actions github-actions bot added size/xl and removed size/l labels Dec 19, 2024
@adrinr adrinr added the do not merge PR is not ready to be merged - generally the PR description should say why label Dec 19, 2024
@adrinr adrinr requested a review from mike12345567 December 19, 2024 14:42
@@ -86,7 +89,7 @@ export async function patch(ctx: UserCtx<PatchRowRequest, PatchRowResponse>) {
// The id might have been changed, so the refetching would fail. Recalculating the id just in case
const updatedId =
generateIdForRow({ ...beforeRow, ...dataToUpdate }, table) || _id
const row = await sdk.rows.external.getRow(table._id!, updatedId, {
const row = await sdk.rows.external.getRow(sourceId, updatedId, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice find!

fields = extractRealFields(table)
}

if (!isView || !helpers.views.isCalculationView(source)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For calculation views, I assume this ends up just leaving the fields empty (since its up to the SQL layer to generate the select statements for these)

Copy link
Collaborator

@mike12345567 mike12345567 left a comment

Choose a reason for hiding this comment

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

LGTM - the sqlQueryJson test cases are a bit of a nightmare to maintain - if they are being problematic we could consider getting rid of them - they circumvent the API which makes them a lot harder to keep up to date.

@adrinr adrinr marked this pull request as ready for review December 20, 2024 14:53
@adrinr adrinr requested a review from a team as a code owner December 20, 2024 14:53
@adrinr adrinr requested review from samwho and removed request for a team December 20, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PR is not ready to be merged - generally the PR description should say why firestorm Data/Infra/Revenue Team size/xl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants