fix: improve query planning time #2303
Conversation
devsergiy
requested review from
Noroth,
StarpTech,
endigma and
jensneuse
as code owners
WalkthroughUpdates dependencies (graphql-go-tools and cosmo modules), refactors planner flow by splitting parsing/preparation and introducing exported PlanPreparedOperation/PrepareOperation/ParseAndPrepareOperation helpers, and adds a planning benchmark suite (Taskfile, example config, tests, .gitignore). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
router-tests/go.mod(1 hunks)router/core/plan_generator.go(1 hunks)router/go.mod(1 hunks)router/internal/planningbenchmark/.gitignore(1 hunks)router/internal/planningbenchmark/Taskfile.yaml(1 hunks)router/internal/planningbenchmark/benchmark_config.json.example(1 hunks)router/internal/planningbenchmark/benchmark_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/go.mod
🧬 Code graph analysis (2)
router/core/plan_generator.go (1)
router/pkg/pubsub/datasource/planner.go (1)
Planner(14-21)
router/internal/planningbenchmark/benchmark_test.go (1)
router/core/plan_generator.go (1)
NewPlanGenerator(270-286)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
router/internal/planningbenchmark/README.md (1)
23-23: Minor: Inconsistent Taskfile reference.Line 23 references
Taskfile.ymlbut the actual file appears to beTaskfile.yaml(based on the context). Ensure consistency in documentation.router/core/plan_generator.go (1)
113-129: PrepareOperation provides clear preparation phase.The function appropriately separates normalization and validation into a discrete preparation step, enabling benchmarking of this phase independently.
Note that
PrepareOperationmodifies theoperationdocument in-place (vianormalizeOperation) and returns the same pointer. While this is consistent with typical AST manipulation patterns, consider adding a comment to clarify this behavior for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
router/core/plan_generator.go(5 hunks)router/core/plan_generator_test.go(1 hunks)router/internal/planningbenchmark/README.md(1 hunks)router/internal/planningbenchmark/benchmark_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/internal/planningbenchmark/benchmark_test.go
🧰 Additional context used
🪛 LanguageTool
router/internal/planningbenchmark/README.md
[style] ~24-~24: The double modal “needed Run” is nonstandard (only accepted in certain dialects). Consider “to be Run”.
Context: ...ningbenchmark/Taskfile.ymlif needed - Runtask bench-cpufromrouter/internal/...
(NEEDS_FIXED)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build-router
- GitHub Check: Analyze (go)
- GitHub Check: build_test
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
router/internal/planningbenchmark/README.md (1)
1-27: Documentation is clear and helpful.The README provides a good foundation for using the planning benchmark feature. The prerequisites and running instructions are straightforward.
Note: The LanguageTool style hint about "if needed - Run" is a false positive—the markdown bullet formatting is correct.
router/core/plan_generator_test.go (1)
32-32: Test correctly updated to use the new API.The change from the unexported
planOperationto the exportedPlanPreparedOperationaligns with the refactoring inplan_generator.go. The test continues to validate panic handling and error propagation correctly.router/core/plan_generator.go (5)
77-101: Well-structured refactoring with improved error messaging.The delegation to
ParseAndPrepareOperationandPlanPreparedOperationcreates a cleaner separation of concerns and enables granular benchmarking of individual planning phases. The error message improvement on line 100 ("invalid outputFormat specified") is more accurate than the previous version.
103-111: Clear API for parse and prepare workflow.This function provides a convenient entry point for parsing an operation from a file and preparing it for planning. The error wrapping with
PlannerOperationValidationErroris consistent with the error handling strategy.
162-194: Core planning function is well-structured.
PlanPreparedOperationcorrectly assumes the operation is normalized and validated, performs planning with appropriate error handling and panic recovery, and post-processes the plan. The switch statement properly handles both synchronous and subscription response plans.
404-404: Reasonable TODO for future enhancement.The TODO about assigning a static operation name for single anonymous operations is a valid future improvement. Currently, anonymous operations will cause
PrepareOperationto fail (line 116-118), which appears to be intentional behavior for now.
113-129: Addressing past review comments: wrapper functions serve the new API design.Previous review comments suggested inlining the normalization and validation calls. However, the current refactoring with
PrepareOperation,ParseAndPrepareOperation, andPlanPreparedOperationas exported functions serves a clear purpose: enabling granular control and benchmarking of individual planning phases. This design is more flexible and appropriate for the PR's goal of improving query planning time measurement.
Summary by CodeRabbit
New Features
Tests
Chores
Documentation
Checklist