Repository navigation
Conversation
ggevay
changed the title
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
October 7, 2026 13:13
4bf8c78 to
5843322
Compare
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
October 8, 2026 13:18
5843322 to
39ec177
Compare
ggevay
marked this pull request as ready for review
October 8, 2026 14:17
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
October 8, 2026 14:30
39ec177 to
1cf8bf8
Compare
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
October 12, 2026 12:23
1cf8bf8 to
c49ce29
Compare
aljoscha
approved these changes
Oct 12, 2026
aljoscha
left a comment
aljoscha
left a comment
Contributor
There was a problem hiding this comment.
Looks good, I had an inline comment about an outdated comment.
Also, I think the code currently is fine, but we're still duplicating some "peek" code in the coordinator now. Maybe a sketch of a fully unified implementation is that the frontend only does a "create dataflow" command or sth, and handle all the rest of peeking, same as it does for fast-path peeks?
Comment on lines
+1111
to
+1114
| // NOTE: Using the cursor creates Futures that are not Sync, | ||
| // so we can't drive them on the main Coordinator loop. | ||
| // Spawning a task has the additional benefit that we get to | ||
| // delete batches once we're done. |
Contributor
There was a problem hiding this comment.
This comment is now outdated, we're not on the coordinator main loop anyways, at this point, no?
Contributor
Author
There was a problem hiding this comment.
Right, fixed. The spawned task is still needed, but because ExecuteResponse::SendingRowsStreaming holds a Sync stream and the cursor's futures aren't Sync, and the comment now says that.
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
October 12, 2026 20:13
c49ce29 to
d495c0a
Compare
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
October 12, 2026 20:31
d495c0a to
b1d5551
Compare
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
November 10, 2026 15:53
b1d5551 to
e0accd4
Compare
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
November 10, 2026 16:51
e0accd4 to
a251be9
Compare
Contributor
Author
The frontend implements fast-path and constant peeks itself, so the coordinator's `implement_peek_plan` only ever received slow-path plans from `implement_slow_path_peek`. Fold its slow-path arm into `implement_slow_path_peek`, delete its fast-path and constant arms and `PlannedPeek`, and create the statement-logging guard only once the peek is registered, so that no error path has to defuse it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It is a free function that both the frontend's fast path and the coordinator's slow path use, and it touches no coordinator state. Move it to `peek_client`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the coordinator no longer sequencing SUBSCRIBE, `implement_subscribe` only runs for the frontend's `ExecuteSubscribe` command, so it no longer needs to take an `ExecuteContext`'s guard by reference. Create the guard where the `Subscribing` response is assembled, as `implement_slow_path_peek` does for peeks, so that the `ExecuteSubscribe` handler has no error path that has to defuse it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The comment on reading a stashed peek's batches still said the cursor's non-Sync futures can't run on the coordinator loop, but the response stream is now built on the session task. What requires the spawned task is the Sync stream that `ExecuteResponse::SendingRowsStreaming` holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ggevay
force-pushed
the
gabor/peek-seq-05-merge-slow-path-peek
branch
from
November 11, 2026 14:48
a251be9 to
5c5ec97
Compare
This branch has not been deployed
This file contains hidden or 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
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.
implement_peek_planintoimplement_slow_path_peek. The frontend implements fast-path and constant peeks itself, so the coordinator'simplement_peek_planonly ever received slow-path plans. Its slow-path arm moves intoimplement_slow_path_peek. Its fast-path and constant arms andPlannedPeekare deleted. The statement-logging guard is created only once the peek is registered, so no error path has to defuse it.create_peek_response_streamtopeek_client. It is a free function used by both the frontend fast path and the coordinator slow path, and touches no coordinator state. Pure move (git show --color-moved).implement_subscribeonly runs for the frontend'sExecuteSubscribecommand now, so it creates its statement-logging guard only for theSubscribingresponse, and the handler no longer defuses one on errors.No behavior change intended.
🤖 Generated with Claude Code