feat: merge entity fetches and then schedule optimally - #3146
Conversation
Use the scheduling improvement in the engine's post-planner stage. scheduleFetches is a dependency-aware scheduler that emits nested Sequence/Parallel trees, collapsing independent chains onto their own branches instead of synchronizing them at wave barriers.
…579-reduce-federated-query-latency-by-improving-fetch-grouping # Conflicts: # router-tests/go.mod # router-tests/go.sum # router/go.mod # router/go.sum
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe router adds disabled-by-default configuration flags for multi-fetch and dependency-aware fetch scheduling. It propagates these flags into executor and planner postprocessing, updates the GraphQL tools dependency, and adds integration tests with query-plan fixtures. ChangesFetch scheduling controls
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The feature flags default to disabled, and no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3146 +/- ##
==========================================
- Coverage 62.40% 62.37% -0.03%
==========================================
Files 263 263
Lines 31048 31070 +22
==========================================
+ Hits 19375 19381 +6
+ Misses 10163 10159 -4
- Partials 1510 1530 +20
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router-tests/modules/query_stats_test.go (1)
171-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the same order-independent assertion to the simple-query case.
This case now treats
SubgraphRootFieldsorder as an implementation detail. The sibling case at lines 51-70 still compares the same field withassert.Equal, which is order-sensitive. If the schedule tree shape changes traversal order for that query too, that assertion becomes flaky.♻️ Proposed change at lines 51-70
- assert.Equal(t, []core.SubgraphRootField{ + // The list is deduplicated in fetch-tree traversal order, which is + // an implementation detail of the schedule tree shape: assert as a set. + assert.ElementsMatch(t, []core.SubgraphRootField{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router-tests/modules/query_stats_test.go` around lines 171 - 173, Update the sibling simple-query test’s assertion for SubgraphRootFields to use the same order-independent ElementsMatch assertion as the query-stats case, while preserving the existing expected values and test coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/plan_generator.go`:
- Around line 292-294: Update PlanPreparedOperation’s planner setup to derive
postprocessor options from the configured execution settings instead of
unconditionally calling postprocess.EnableMultiFetch(). Pass those effective
options into postprocess.NewProcessor, preserving both DisableScheduleFetches
and DisableMultiFetch behavior, and add regression coverage for each setting.
---
Nitpick comments:
In `@router-tests/modules/query_stats_test.go`:
- Around line 171-173: Update the sibling simple-query test’s assertion for
SubgraphRootFields to use the same order-independent ElementsMatch assertion as
the query-stats case, while preserving the existing expected values and test
coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87f69643-4ba7-4e29-96a8-ce79ed1d3cb0
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
router-tests/go.modrouter-tests/modules/query_stats_test.gorouter-tests/operations/testdata/fixtures/query_plans/only_query_plan.jsonrouter-tests/operations/testdata/fixtures/query_plans/query_plan_with_trace_no_data.jsonrouter-tests/operations/testdata/fixtures/query_plans/response_with_query_plan.jsonrouter-tests/operations/testdata/fixtures/query_plans/response_with_query_plan_operation_name.jsonrouter-tests/operations/testdata/fixtures/query_plans/response_with_query_plan_operation_name_sanitized_no_data.jsonrouter-tests/protocol/testdata/tracing.jsonrouter/core/executor.gorouter/core/factoryresolver.gorouter/core/operation_planner.gorouter/core/plan_generator.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router-tests/operations/multi_fetch_test.go (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
DisableScheduleFetches.The PR adds two execution flags:
disable_multi_fetchanddisable_schedule_fetches. This test covers onlyDisableMultiFetch.DisableScheduleFetchesswitches the planner back to the legacy wave-based fetch organizers, which is the escape hatch for this change. That path has no integration coverage in the provided files.Add a third subtest that sets
cfg.DisableScheduleFetches = trueand asserts the same response body. The response must stay identical regardless of the scheduling strategy.🧪 Proposed additional subtest
t.Run("returns the same result when fetch scheduling is disabled", func(t *testing.T) { t.Parallel() testenv.Run(t, &testenv.Config{ ModifyEngineExecutionConfiguration: func(cfg *config.EngineExecutionConfiguration) { cfg.DisableScheduleFetches = true }, }, func(t *testing.T, xEnv *testenv.Environment) { res := xEnv.MakeGraphQLRequestOK(testenv.GraphQLRequest{Query: multiFetchQuery}) require.JSONEq(t, multiFetchExpectedResponse, res.Body) }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router-tests/operations/multi_fetch_test.go` at line 13, Add a third parallel subtest in TestMultiFetch that sets DisableScheduleFetches to true through ModifyEngineExecutionConfiguration, executes multiFetchQuery, and asserts the response matches multiFetchExpectedResponse with JSON equality, preserving the existing test setup and result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@router-tests/operations/multi_fetch_test.go`:
- Line 13: Add a third parallel subtest in TestMultiFetch that sets
DisableScheduleFetches to true through ModifyEngineExecutionConfiguration,
executes multiFetchQuery, and asserts the response matches
multiFetchExpectedResponse with JSON equality, preserving the existing test
setup and result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fca06bdc-6397-4212-a300-d7ec341ac034
📒 Files selected for processing (9)
router-tests/modules/query_stats_test.gorouter-tests/operations/multi_fetch_test.gorouter-tests/operations/testdata/fixtures/query_plans/only_query_plan.jsonrouter-tests/operations/testdata/fixtures/query_plans/query_plan_with_trace_no_data.jsonrouter-tests/operations/testdata/fixtures/query_plans/response_with_query_plan.jsonrouter-tests/operations/testdata/fixtures/query_plans/response_with_query_plan_operation_name.jsonrouter-tests/operations/testdata/fixtures/query_plans/response_with_query_plan_operation_name_sanitized_no_data.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/config/testdata/config_full.json
- router/pkg/config/testdata/config_defaults.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router-tests/protocol/integration_test.go`:
- Around line 691-693: Update the assertion message in the integration test
around the matched operation check to format the expectEmployeeOps []string with
%v instead of %s, so failures display the expected operation names correctly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5775ad7f-81a0-47a3-8d8b-9709334743dc
📒 Files selected for processing (1)
router-tests/protocol/integration_test.go
…re-schedule # Conflicts: # router-tests/go.mod # router-tests/go.sum # router/go.mod # router/go.sum
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/config/config.schema.json (1)
2777-2787: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDescribe authorization-server URLs as discovery metadata only.
authorization_server_urlsdoes not establish token trust or validate the JWTissclaim. The statement that this field can “trust more than one authorization server” can cause operators to rely on an issuer restriction that the router does not enforce. State that configured JWKS signing keys are the trust boundary, and remove the per-issuer validation implication.Based on learnings:
AuthorizationServerURLandAuthorizationServerURLsprovide RFC 9728 discovery metadata only; token validation is anchored in configured JWKS keys and does not validate JWTissagainst these fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/config/config.schema.json` around lines 2777 - 2787, Update the description for authorization_server_urls to describe AuthorizationServerURL and AuthorizationServerURLs as RFC 9728 discovery metadata only. Remove the claims that these fields establish trust, restrict issuers, or imply JWT iss validation, and state that token trust is determined by configured JWKS signing keys.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@router/pkg/config/config.schema.json`:
- Around line 2777-2787: Update the description for authorization_server_urls to
describe AuthorizationServerURL and AuthorizationServerURLs as RFC 9728
discovery metadata only. Remove the claims that these fields establish trust,
restrict issuers, or imply JWT iss validation, and state that token trust is
determined by configured JWKS signing keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 066530e9-b2f1-4698-b803-58a2ca751932
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
router-tests/go.modrouter-tests/operations/multi_fetch_test.gorouter/core/executor.gorouter/core/factoryresolver.gorouter/core/plan_generator.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/config/testdata/config_full.json
- router-tests/operations/multi_fetch_test.go
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
Code itself is ok so approving.
However my main concern is that we have the "query-plan-skip" tag for query plan changes for the customer (which is what the query planner tests are for), I would verify if at least the error is the same as the baseline error before merging (You could open a no-op PR with a comment change and add the "query-plan" label, and compare the results from there and this PR).
|
I have tested QPlanner here: https://github.com/wundergraph/cosmo-celestial/pull/2094. There is a slight performance regression detected which comes from Audit FIxes released in the engine some time ago. |
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
2 participants
This PR adds two feature flags for the Cosmo's Query Planner:
engine.enable_multi_fetchThis option merges entity fetches that target the same subgraph and
execute at the same point in the query plan
into a single request with aliased
_entitiesfields.This reduces the number of requests sent to subgraphs.
env: ENGINE_ENABLE_MULTI_FETCH
engine.enable_schedule_fetchesThis feature replaces the wave-based fetch execution
with a dependency-aware fetch scheduler.
Independent fetch chains progress as soon as
their own dependencies complete,
instead of waiting for the slowest fetch in each wave.
env: ENGINE_ENABLE_SCHEDULE_FETCHES
By default, these features are disabled:
Built on top of wundergraph/graphql-go-tools#1612
Summary by CodeRabbit
New Features
Documentation
Tests