-
-
Notifications
You must be signed in to change notification settings - Fork 578
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
PgSelect now yields an object, not an array #2335
Merged
Merged
Conversation
This file contains 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
🦋 Changeset detectedLatest commit: 13a1219 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
benjie
force-pushed
the
pg-select-object
branch
2 times, most recently
from
January 28, 2025 19:29
19f6127
to
76fc9d2
Compare
connection capable steps.
…ld an async iterator
It breaks `grafast/grafast/__tests__/errorHandling-test.ts`
…ist positions to return a _different_ step to actually return the list - useful for returning connection-capable steps in list positions.
…ute() now handles both stream() and defer() concerns.
…, which has meant that stream info is no longer passed at planning time - instead execute() can evaluate if it is being streamed or not and make decisions based on that.
…ather than arrays/streams; thanks to the new Grafast .items() method and these classes being "opaque" steps this is _mostly_ a non-breaking change.
…latest logic in Grafast.
benjie
force-pushed
the
pg-select-object
branch
from
February 4, 2025 15:41
76fc9d2
to
72b300b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
hasMore
used to be hacked onto the returned array which was always a disgusting hack but it worked and I couldn't justify spending the time to fix it. But it turns out we need more stuff like that now that we're eradicating eval, for example we need to know what the indicies are that were used to select the orderBy values so that we can populate the cursors with them. Since we're likely to need more stuff like this in future, I've changed PgSelect to return an object, where one of the entries in the object isitems
which is the previous array; then I've added PgSelectRowsStep and PgUnionAllRowsStep to extract these items whilst still honouring stream/defer.I've also updated Grafast to have a new convention method,
ExecutableStep::items()
, which will be called when a step is to be used in a list position. This gives the step a chance to return an alternative step to yield the list itself, this enables the PgSelect to be used directly in list positions (without having to.items()
yourself manually), and to feed it to things likelistTransform
(each/filter/groupBy/etc). This minimizes the amount of rewriting necessary.I've also merged the
stream()
andexecute()
methods - these are no longer separate methods, in factstream()
is no longer supported. The return signature ofexecute()
has changed to accomodate this, but no change is needed on existing step classes unless they implement astream()
method.I've also rewritten how the
@stream
directive is interpreted and moved it entirely to runtime, including in the output generation.All of this is breaking.
Performance impact
Work towards:
MAJOR CHANGE: I've deleted the optimizations of PgSelectStep, so SQL inlining will no longer occur. We'll restore this in a (much) later PR, but for now we need to focus on solving the above issue.
Security impact
Working towards eradicating step eval, so positive.
Checklist
yarn lint:fix
passes.yarn test
passes.RELEASE_NOTES.md
file (if one exists).