Allow path stitcher to work on SQLiteReader
#291
Merged
Conversation
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
Performance SummaryComparing base acb0fab with head ee936e1 on microsoft/[email protected]. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
hendrikvanantwerpen
force-pushed
the
sql-based-stitching
branch
from
a552756 to
2a5b45f
Compare
Performance SummaryComparing base acb0fab with head a552756 on microsoft/[email protected]. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
Performance SummaryComparing base acb0fab with head 2a5b45f on microsoft/[email protected]. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
Performance SummaryComparing base acb0fab with head 8a3834c on microsoft/[email protected]. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
Performance SummaryComparing base acb0fab with head b3bc09a on microsoft/[email protected]. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
hendrikvanantwerpen
force-pushed
the
sql-based-stitching
branch
from
b3bc09a to
2a5b45f
Compare
hendrikvanantwerpen
force-pushed
the
sql-based-stitching
branch
from
2a5b45f to
db92a36
Compare
Performance SummaryComparing base f8477c5 with head db92a36 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
dcreager
approved these changes
Sep 13, 2023
| graph: &StackGraph, | ||
| partials: &mut PartialPaths, | ||
| db: &mut Db, | ||
| candidates: &mut C, |
There was a problem hiding this comment.
Only API suggestion I can think of is to consider if this could/should be mut candidates: C instead. I think you're always passing in a mut ref to a temporary, in which case you can just pass in that temporary directly. It probably doesn't matter from a codegen perspective, though, so if you consider this shape to be more understandable I'm also good with keeping it as-is.
There was a problem hiding this comment.
I tried this, but the SQLiteReader implements Candidates directly, and would be moved (and lost) by this change. So I'm going to leave it as-is.
This is to keep consistent with the `ForwardPartialPathStitcher` and it keeps to option open to introduce `Backward` versions in the future.
These changes add support for using `ForwardPartialPathStitcher` directly on a `SQLiteReader` (besides the already supported graph edges and `Database`). This required pushing the `StackGraph` + `PartialPaths` + `ToAppendable` values into the `ForwardCandidates` trait, to allow the candidates implementation to lazily load data during the stitching process.
hendrikvanantwerpen
force-pushed
the
sql-based-stitching
branch
from
db92a36 to
233e827
Compare
Performance SummaryComparing base 0fcb846 with head 233e827 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in BeforeAfter |
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.
This PR makes changes to
ForwardPartialPathStitcherand related types to add support for running the path stitcher directly on aSQLiteReader. With this change, theForwardPartialPathStitcheris the one and only path building implementation.Some changes that were made as part of this:
CandidatestoForwardCandidates, which is in line with theForwardPartialPathStitcherand would make it easier in the future if we ever get backward algorithms.SQLReader::clearmethod for easier reuse of instances. It clears the cached data in the reader.ForwardCandidatestrait with the reponsibility to manage access to the graph and partial path arena. As a result, theForwardCandidatesinstance is the only thing going into the stitcher, and replaces the explicitStackGraphandPartialPathsarguments. This was necessary to allow the candidates implementation to lazy load data during stitching.ForwardCandidateserrors throughForwardPartialPathStitcher, so that loading errors can be reported to the caller.