feat: dominance-based hybrid query plan scheduler (opt-in) - #1491
Closed
jensneuse wants to merge 3 commits into
Closed
feat: dominance-based hybrid query plan scheduler (opt-in)#1491jensneuse wants to merge 3 commits into
jensneuse wants to merge 3 commits into
Conversation
Replace the level-based parallel grouping in postprocess with an opt-in hybrid scheduler that picks the better of two complementary algorithms using a provably correct symbolic dominance check. The level-based postprocessor synchronises every dependency level even when independent chains share no edges, which forces unrelated fetches to wait at every barrier. The new scheduler emits Parallel(Sequence(...), Sequence(...)) shapes when two chains can run fully in parallel, cutting critical-path latency on the dominant federation pattern (independent entity hops joining at a downstream subgraph). The hybrid runs both scheduleSP (component-aware eager-inline with multi-parent merge intersection) and scheduleLevel (component-aware WCC + ASAP-roots) and returns the SP tree only when symbolic path-set dominance proves it is no worse than the Level tree under any non-negative duration vector. Otherwise it returns the Level tree. This guarantees no regression against the existing level-based output and is a strict improvement on every DAG where SP dominates Level. Executor refactor: the loader's resolveParallel now supports nested Sequence and Parallel children via a three-phase leaf protocol (prepare under lock, load without lock, merge under lock). A useMergeMu bit computed at fetch tree entry keeps flat plans on the existing fast path with zero mutex overhead. The race detector is clean across postprocess and resolve test suites. Wiring: the new processor is opt-in via WithBuildScheduleTree(). The legacy orderSequenceByDependencies + createParallelNodes pair remains the default. A follow-up PR will flip the default and update fixtures. Coverage: 23 scenario tests covering independent components, chains, diamonds, fan-out/fan-in, @requires chains, batch entity fetches, nested entities, interface expansion, @provides, mutations, composite key fan-in, response-path validator stress, asymmetric chain merges, deep multi-parent fan-in, the non-SP N-shape representation limit, and the dominance-fallback case for incomparable trees. Property tests exhaustively enumerate DAGs up to n=6, sample random DAGs at n=3..15 and n=50/200, and stress every fixture under skewed duration distributions. ADR docs/adr/0001-query-plan-scheduler.md captures the algorithm, the dominance theorem and proof, the executor split, and the acknowledged residual gaps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…d error The first commit refactored the loader for nested Sequence / Parallel support but left the new code paths (resolveParallelNested, three-phase prepare/load/merge split, mergeMu mutex bypass, fetchTreeHasNestedParallel walk) without direct test coverage. Reviewer correctly flagged this. Adds focused executor tests that exercise every new path: - TestLoaderNestedParallelCorrectness: hand-rolled Parallel(Sequence(A,B), Single(C)) with a controlled mock data source. Verifies B sees A's merged data through the new resolveSerialWithCtx → resolveSingleWithCtx flow inside a parallel branch. - TestLoaderNestedParallelRaceMutexDiscipline: forces interleaving by making A's load wait on D's done channel. Branch 2's mergePhase runs while branch 1 is still in loadPhase, exercising the prepare/load/merge serialisation invariant. Clean under -race. - TestLoaderFlatParallelKeepsFastPathWithoutMergeMutex: pins the flat fast path with an assertion on Loader.useMergeMu being false. - TestFetchTreeHasNestedParallel: pure unit test covering nil, leaf, flat parallel, sequence, deep nesting, and large flat trees. - TestResolveParallelNestedDoesNotCancelSiblings: when one nested branch fails its load, the sibling branch completes normally. The response carries the failing branch's subgraph error AND the sibling's data. Asserted with require.Equal on the full response body. - TestResolveParallelNestedSequenceStopsAfterFailedDependency: when A fails inside Sequence(A, B), B is in the same sequence and depends on A. C is in a parallel sibling branch. B's data source is not called and B emits no secondary error in the response. C runs normally. - TestResolveSerialStopsTransitivelyOnFailedDependency: A → B → C all in one sequence with explicit DependsOnFetchIDs. When A fails, neither B nor C run, and only A's subgraph error is in the response. To make the last two tests pass cleanly the loader now skips a fetch whose DependsOnFetchIDs intersects the set of fetches that already failed their load. The check happens inside preparePhase under the existing merge mutex; load errors record their fetch ID after executeSourceLoad returns. This skip applies only to the new prepare/load/merge path and does not affect the flat-parallel fast path which still calls loadFetch directly. The skip prevents wasted network calls and avoids emitting redundant secondary errors for fetches whose upstream dependency already failed, which matches the review feedback that errors should stop only the children that depend on the failing fetch. Verified clean: - go build ./pkg/engine/resolve/ - go test ./pkg/engine/resolve/ - go test -race ./pkg/engine/resolve/ - go test ./pkg/engine/postprocess/ (and -race) - go test -run TestQueryOrderBaseline ./pkg/engine/datasource/graphql_datasource/ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the four-paragraph Worked Examples section with five detailed walkthroughs that each show: - the dependency graph drawn in ASCII - the candidate trees produced by scheduleSP and scheduleLevel - the root-to-leaf path sets used by the dominance check - the dominance verdict and which tree the hybrid returns - a numerical makespan comparison under uniform and skewed durations The five examples cover the four interesting decision outcomes: 1. Independent components — both schedulers identical, SP trivially dominates, illustrates the baseline win over the legacy shape. 2. Two chains joining at a third subgraph — SP path-set-dominates Level, hybrid takes the SP win that grows under skewed durations. 3. Asymmetric chain merge with a leaf side-branch — SP does NOT dominate, regresses against Level even under uniform durations, hybrid correctly falls back to Level. Canonical motivation for the dominance check. 4. Independent leaf alongside a chain (codex round-9 case) — SP loses only under skewed durations, hybrid catches it via path-set containment, also illustrates why scheduleSP runs WCC at every recursion level. 5. Incomparable trees (codex round-10 case) — neither tree dominates, each wins under different duration vectors, hybrid takes the conservative Level fallback. Documents Gap B at runtime. Closes with a four-line decision table summarising which of the five outcomes maps to which hybrid pick. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Resolved by #1594 |
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.
Summary
WithBuildScheduleTree()) that emitsParallel(Sequence(...), Sequence(...))shapes when independent dependency chains can run fully in parallel — cutting critical-path latency on the dominant federation pattern.scheduleSPeager-inline,scheduleLevelWCC + ASAP-roots) using a provably correct symbolic path-set dominance check. Guarantees no regression vs the level-based output under any duration distribution.Sequence/Parallelfetch trees with a three-phase leaf protocol (prepare / load / merge); flat plans keep the existing fast path with zero mutex overhead.Why
Today's level-based grouping (
orderSequenceByDependencies+createParallelNodes) synchronises every dependency level. When two independent entity chains co-exist (e.g.me → currentPracticeplus an unrelatedorganisationsfetch), the second chain has to wait for the first chain's join even though no dependency requires it.The optimal plan is
Parallel(Sequence(user, practice), organisations), which cuts critical path frommax(user, organisations) + practicetomax(user + practice, organisations).Algorithm — TL;DR
scheduleSP(component-aware eager-inline with multi-parent merge intersection) andscheduleLevel(component-aware level-based) on the same DAG.dominates(SP, Level)is true, where dominance is set-containment of root-to-leaf paths: every path in A is contained in some path in B. This impliesmakespan_A(d) ≤ makespan_B(d)for every non-negative duration vector — a finite, polynomial-time, provably correct check.Full theorem and proof in docs/adr/0001-query-plan-scheduler.md.
Wiring
The new processor is opt-in via
WithBuildScheduleTree(). The default postprocess pipeline is unchanged — existing fixtures and behaviour are stable. A follow-up PR will flip the default and audit fixture diffs.Scope
v2/pkg/engine/resolve/loader.go— three-phase split (prepare/load/merge) insideresolveSingleforSingleFetch/EntityFetch/BatchEntityFetch.mergeMu sync.MutexanduseMergeMu boolonLoader.fetchTreeHasNestedParallelwalk at fetch-tree entry.resolveParallelbranches: existing fast path for all-Singlechildren (no lock),resolveParallelNestedfor the new shapes (lock-aware viamaybeLock).v2/pkg/engine/postprocess/build_schedule_tree.go—scheduleSP,scheduleLevel,dominates,buildScheduleTree,validateSchedulewith leaf-flattening and fetch-kind-awareprovidedPathfor SingleFetch/EntityFetch/BatchEntityFetch.v2/pkg/engine/postprocess/postprocess.go—WithBuildScheduleTree()opt-in option.v2/pkg/engine/postprocess/build_schedule_tree_scenarios_test.go— 23 hand-built scenarios + option-wiring + scenario-17 validator stress test.v2/pkg/engine/postprocess/build_schedule_tree_property_test.go— exhaustive enumerationn≤6, randomn=3..15(fixed seed), smoken=50/200, fixture skew-stress.v2/pkg/engine/datasource/graphql_datasource/query_order_baseline_test.go— planner-direct baseline test (pins today's level-based shape; the new processor is opt-in so this still passes).docs/adr/0001-query-plan-scheduler.md— ADR with algorithms, dominance theorem + proof, executor split, residuals, worked examples.docs/superpowers/specs/2026-05-08-*.md— design specs that drove the implementation.Acknowledged residuals
Documented in the ADR and in scenario fixtures:
Test plan
go test ./pkg/engine/postprocess/— all 23 scenarios + 4 property tests passgo test ./pkg/engine/resolve/— cleango test -race ./pkg/engine/postprocess/ ./pkg/engine/resolve/— cleango test -run TestQueryOrderBaseline ./pkg/engine/datasource/graphql_datasource/— passes (default behaviour unchanged)FetchTreeProcessorinterface to return error (open question)Notes
This is a draft. No fixture flips, no default behaviour changes, no executor regressions — only new opt-in code paths plus a behaviour-preserving executor refactor.
🤖 Generated with Claude Code