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

Move more input planning to runtime #2376

Draft
wants to merge 90 commits into
base: runtime-orderby
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Feb 19, 2025

Description

Yet more related to

This introduces the new applyInput and bakedInput steps to replace applyPlan and inputPlan needs at runtime; and then uses them to move the following from plantime to runtime:

It also gets rid of ModifierStep and replaces it with Modifier (which runs at runtime, as part of the new applyInput) and implements necessary APIs to make transition a little softer (e.g. $foo.placeholder(value.getRaw(), codec) is no longer available at runtime, so instead you do sqlValueWithCodec(value, codec)).

FieldArgs is now only used for the field itself, and a new FieldArg type is used for argument applyPlan; no other inputs support applyPlan or inputPlan any more. There's no need for the autoApplyAfter... fields any more either.

Performance impact

Unknown, not really thinking about performance right now. Probably some trade-offs, but also the SQL that's generated can be more efficient so even if it's slower in Node it should be faster in Postgres.

Security impact

Less load on the DB (hopefully) and less ability to have drastically long planning time in Grafast - win.

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

Copy link

changeset-bot bot commented Feb 19, 2025

⚠️ No Changeset found

Latest commit: 8f08788

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🌳 Triage
Development

Successfully merging this pull request may close these issues.

1 participant