Conversation
WalkthroughAdds interface→object caching and a parser precalc step; introduces walker IDs via NewWalkerWithID and updates callers; refactors datasource node collection into per-data-source visitors with a nodesCollector and newFieldRefs propagation; multiple visitors, builders, and tests updated to the new APIs and flows. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.35)
participant Parser
participant P as parser.precalculate
participant Doc as Document
participant IF as InterfaceTypeDefinition
end
Parser->>P: after Parse()/ParseWithLimits
alt no parse errors
P->>Doc: PopulateInterfaceTypeDefinitionImplementedByObjects()
Doc->>IF: set ImplementedByObjectDefinitions
end
sequenceDiagram
rect rgba(240,250,230,0.35)
participant Builder as NodeSelectionBuilder
participant Visitor as nodeSelectionVisitor
participant Filter as DataSourceFilter
participant Collector as nodesCollector
participant DS1 as DataSource#1
participant DSn as DataSource#N
end
Builder->>Visitor: SelectNodes()
Visitor->>Visitor: init newFieldRefs
Visitor->>Filter: NewDataSourceFilter(..., dataSources, newFieldRefs)
Filter->>Collector: init per-DS visitors
par per-DS visits
Collector->>DS1: collect via collectNodesDSVisitor
Collector->>DSn: collect via collectNodesDSVisitor
end
Collector-->>Filter: aggregate per-DS keys/reports
Filter-->>Visitor: used data sources & NodeSuggestions
Visitor->>Visitor: merge discovered newFieldRefs (may trigger rerun)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
devsergiy
force-pushed
the
sergiy/eng-8285-improve-query-planning-time
branch
from
85594c9 to
8cb0c3c
Compare
ysmolski
left a comment
ysmolski
left a comment
There was a problem hiding this comment.
First half of review...
devsergiy
requested review from
Noroth,
StarpTech and
jensneuse
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
v2/pkg/engine/plan/visitor.go (1)
1023-1061: Clean up disabled code.The TODO comment indicates that the field alias override logic has been disabled and cleanup is needed. The
resolveFieldPathfunction (lines 1024-1061) is no longer called fromEnterFieldand should be removed to avoid confusion.Consider removing the unused
resolveFieldPathfunction and the TODO comment once you've verified that the simplified path construction (line 394) works correctly across all use cases.v2/pkg/engine/plan/datasource_filter_visitor.go (2)
109-114: Replace panic with a report error in applyLandedToA missing node can occur if landedTo references were pruned/rewritten. Panicking will crash planning.
Apply this diff:
- if !ok { - panic("node not found") - } + if !ok { + f.report.AddInternalError(fmt.Errorf("applyLandedTo: node not found for fieldRef=%d (treeID=%d, path=%q)", fieldRef, treeNodeID, f.nodes.items[fieldRef].ParentPath)) + continue + }
905-915: Guard against empty children in childsHasOnlyTypenamechild.GetData() can be empty; accessing itemIds[0] risks panic.
Apply this diff:
- for _, child := range children { - itemIds := child.GetData() - firstItem := itemIds[0] + for _, child := range children { + itemIds := child.GetData() + if len(itemIds) == 0 { + // no suggestions for this child; skip + continue + } + firstItem := itemIds[0]
🧹 Nitpick comments (6)
v2/pkg/astvisitor/visitor.go (1)
1383-1387: Consider removing or documenting commented profiling code.The commented pprof code appears to be intentionally disabled. Either:
- Remove it if it's not needed, or
- Add a comment explaining when/how to enable it for profiling
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
1072-1075: Use a stable sort (or a tie-breaker) to avoid flaky test orderYou sort by Path only using an unstable sort. When multiple suggestions share the same Path, relative order is undefined and can cause sporadic failures.
Apply this diff:
- slices.SortFunc(actualItems, func(a, b *NodeSuggestion) int { - return strings.Compare(a.Path, b.Path) - }) + // Stable sort preserves insertion order for equal paths, avoiding flakiness. + slices.SortStableFunc(actualItems, func(a, b *NodeSuggestion) int { + return strings.Compare(a.Path, b.Path) + })v2/pkg/engine/plan/datasource_filter_visitor.go (2)
50-54: Honor the max concurrency settingWithMaxDataSourceCollectorsConcurrency sets a limit, but nodesCollector currently spawns one goroutine per data source unconditionally. Respect the cap to prevent oversubscription on large graphs.
Example adjustment (gate inside collector):
- for i, ds := range f.dataSources { ... go func(...) { ... }(...) } + // inside nodesCollector.collectNodes(): + semCap := int(c.maxConcurrency) + var sem chan struct{} + if semCap > 0 { + sem = make(chan struct{}, semCap) + } + for i, visitor := range c.dsVisitors { + if sem != nil { sem <- struct{}{} } + go func(visitor *collectNodesDSVisitor, report *operationreport.Report, nodesToVisit []nodeVisitTask) { + if sem != nil { defer func() { <-sem }() } + // existing body ... + }(visitor, c.dsVisitorsReports[i], nodesToVisit) + }Also applies to: 143-195
775-779: Deterministic tie-breaking for duplicate selection orderSorting by jumpCount with slices.SortFunc is unstable; ties can reorder duplicates nondeterministically, affecting “first available” behavior.
Apply this diff to use a stable sort:
- slices.SortFunc(jumpsCounts, func(a, b nodeJump) int { + slices.SortStableFunc(jumpsCounts, func(a, b nodeJump) int { // acs order from 0 to n return a.jumpCount - b.jumpCount })Optionally add a secondary key (e.g., data source order or hash) if stricter determinism is needed.
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (2)
259-263: Reset should also clear localSuggestionLookup and providesEntriesPersisting these across runs can suppress legitimate re-suggestions after rewrites and grow memory.
Apply this diff:
func (f *collectNodesDSVisitor) reset() { - f.localSuggestions = f.localSuggestions[:0] - f.keys = f.keys[:0] + f.localSuggestions = f.localSuggestions[:0] + f.keys = f.keys[:0] + // clear per-run caches + if len(f.localSuggestionLookup) > 0 { + for k := range f.localSuggestionLookup { + delete(f.localSuggestionLookup, k) + } + } + if len(f.providesEntries) > 0 { + f.providesEntries = f.providesEntries[:0] + } }Optionally clear notExternalKeyPaths if key composition can change between runs.
122-148: Respect maxConcurrency when dispatching per-DS visitorscollectNodes launches one goroutine per DS without honoring c.maxConcurrency. Gate execution to prevent resource spikes on large fleets.
Apply this diff:
- wg := &sync.WaitGroup{} - wg.Add(len(c.dsVisitors)) + wg := &sync.WaitGroup{} + wg.Add(len(c.dsVisitors)) + var sem chan struct{} + if c.maxConcurrency > 0 { + sem = make(chan struct{}, c.maxConcurrency) + } - for i, visitor := range c.dsVisitors { - go func(visitor *collectNodesDSVisitor, report *operationreport.Report, nodesToVisit []nodeVisitTask) { + for i, visitor := range c.dsVisitors { + if sem != nil { sem <- struct{}{} } + go func(visitor *collectNodesDSVisitor, report *operationreport.Report, nodesToVisit []nodeVisitTask) { defer func() { // recover from panic and add it to the report if r := recover(); r != nil { report.AddInternalError(fmt.Errorf("panic: %v stack: %s", r, debug.Stack())) } }() defer func() { - wg.Done() + if sem != nil { <-sem } + wg.Done() }() // cleanup data from previous runs, but preserve indexes visitor.reset() for _, node := range nodesToVisit { if err := visitor.EnterField(node.fieldRef, node.treeNodeData, node.treeNodeId); err != nil { report.AddInternalError(fmt.Errorf("data source %s: %v", visitor.dataSource.Name(), err)) // stop processing on error return } } - }(visitor, c.dsVisitorsReports[i], nodesToVisit) + }(visitor, c.dsVisitorsReports[i], nodesToVisit) }Also applies to: 150-163
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
v2/pkg/ast/ast_interface_type_definition.go(4 hunks)v2/pkg/astnormalization/abstract_field_normalizer.go(1 hunks)v2/pkg/astnormalization/astnormalization.go(6 hunks)v2/pkg/astnormalization/variables_mapper.go(1 hunks)v2/pkg/astparser/parser.go(2 hunks)v2/pkg/asttransform/typename_visitor.go(1 hunks)v2/pkg/astvalidation/operation_validation.go(2 hunks)v2/pkg/astvisitor/visitor.go(4 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go(7 hunks)v2/pkg/engine/plan/abstract_selection_rewriter.go(1 hunks)v2/pkg/engine/plan/analyze_plan_kind.go(1 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go(19 hunks)v2/pkg/engine/plan/datasource_filter_node_suggestions.go(2 hunks)v2/pkg/engine/plan/datasource_filter_visitor.go(5 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(3 hunks)v2/pkg/engine/plan/key_fields_visitor.go(3 hunks)v2/pkg/engine/plan/key_fields_visitor_test.go(2 hunks)v2/pkg/engine/plan/node_selection_builder.go(4 hunks)v2/pkg/engine/plan/node_selection_visitor.go(5 hunks)v2/pkg/engine/plan/path_builder.go(1 hunks)v2/pkg/engine/plan/planner.go(1 hunks)v2/pkg/engine/plan/provides_fields_visitor.go(2 hunks)v2/pkg/engine/plan/required_fields_visitor.go(1 hunks)v2/pkg/engine/plan/visitor.go(2 hunks)v2/pkg/variablesvalidation/variablesvalidation.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (19)
v2/pkg/engine/plan/abstract_selection_rewriter.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/engine/plan/analyze_plan_kind.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/engine/plan/provides_fields_visitor.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/asttransform/typename_visitor.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/astnormalization/astnormalization.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/astnormalization/variables_mapper.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewDefaultWalkerWithID(125-127)
v2/pkg/variablesvalidation/variablesvalidation.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
v2/pkg/engine/plan/datasource_filter_visitor.go (1)
NewDataSourceFilter(35-44)
v2/pkg/engine/plan/key_fields_visitor.go (4)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (1)
SeenKeyPath(43-47)v2/pkg/engine/plan/datasource_configuration.go (1)
DSHash(15-15)v2/pkg/engine/datasource/graphql_datasource/configuration.go (1)
FederationConfiguration(130-133)v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/astvalidation/operation_validation.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/engine/plan/path_builder.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/astnormalization/abstract_field_normalizer.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/engine/plan/datasource_filter_visitor.go (6)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSource(274-287)DSHash(15-15)v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (2)
KeyIndex(38-41)SeenKeyPath(43-47)v2/pkg/engine/plan/source_connection_graph.go (1)
DataSourceJumpsGraph(34-37)v2/pkg/ast/ast.go (1)
Document(10-56)v2/pkg/operationreport/operationreport.go (1)
Report(9-12)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (3)
NewNodeSuggestions(122-124)NodeSuggestions(93-98)TraverseBFS(100-120)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (5)
v2/pkg/operationreport/operationreport.go (2)
Report(9-12)NewReport(14-16)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (2)
TraverseBFS(100-120)NodeSuggestion(14-40)v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(274-287)v2/pkg/engine/datasource/graphql_datasource/configuration.go (1)
FederationConfiguration(130-133)
v2/pkg/engine/plan/planner.go (2)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/astnormalization/inline_fragment_add_on_type.go (1)
InlineFragmentAddOnType(11-17)
v2/pkg/engine/plan/node_selection_builder.go (2)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/engine/plan/datasource_filter_visitor.go (1)
NewDataSourceFilter(35-44)
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go (1)
v2/pkg/engine/plan/type_field.go (1)
TypeField(3-8)
v2/pkg/astvisitor/visitor.go (2)
v2/pkg/engine/resolve/node.go (1)
Node(20-26)v2/pkg/ast/path.go (2)
Path(34-34)PathItem(27-32)
v2/pkg/engine/plan/required_fields_visitor.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (22)
v2/pkg/astnormalization/variables_mapper.go (1)
15-15: LGTM! Walker identification added for better traceability.The change from
NewDefaultWalker()toNewDefaultWalkerWithID("VariablesMapper")adds a meaningful identifier for debugging and tracing purposes without modifying behavior. The ID appropriately matches the struct name and aligns with the project-wide standardization effort.v2/pkg/engine/plan/configuration.go (1)
118-118: The review comment is incorrect—the field is actively used in the codebase.The script confirms that
DisableDefaultMappingis read inv2/pkg/engine/plan/visitor.go:1046, where it controls conditional logic:if v.Config.Fields[i].DisableDefaultMapping { return nil }The field clearly has an effect: it gates whether default mapping is applied. It's also set in tests and documented with examples. The TODO comment suggesting it "has no effect" is misleading and should be removed or corrected rather than following through on the suggested deletion.
Likely an incorrect or invalid review comment.
v2/pkg/asttransform/typename_visitor.go (1)
21-21: LGTM: Walker identifier added for traceability.The walker initialization has been updated to include an identifier string for debugging/tracing purposes, consistent with the project-wide refactor.
v2/pkg/engine/plan/path_builder.go (1)
20-20: LGTM: Walker identifier added for traceability.Consistent with the project-wide refactor to add identifiers to walkers for debugging/tracing.
v2/pkg/engine/plan/planner.go (1)
56-56: LGTM: Walker identifiers added for traceability.Both walker initializations have been updated with descriptive identifiers ("PrepareOperationWalker" and "PlanningWalker") for debugging/tracing, consistent with the project-wide refactor.
Also applies to: 61-61
v2/pkg/astvisitor/visitor.go (2)
66-66: LGTM: New walker constructors with ID support.The additions of
walkerIDfield and the new constructorsNewWalkerWithIDandNewDefaultWalkerWithIDprovide a clean way to identify walkers for debugging/tracing without breaking existing functionality.Also applies to: 100-108, 125-127
114-114: The Path capacity change to a fixed value of 64 is safe and doesn't impact existing callers.The code review concern can be resolved. Analysis shows:
- All calls to
NewWalker()useancestorSizevalues of 4, 8, or 24—all less than the fixed capacity of 64- No code validates or depends on
Pathcapacity matchingancestorSize; they are independent allocationsPathusage only involves slicing operations (w.Path[:len(w.Path)-1]) and length checks, which work correctly with the larger capacity- No performance-sensitive deep nesting scenarios detected that would exceed the 64-item capacity
v2/pkg/engine/plan/analyze_plan_kind.go (1)
10-10: LGTM: Walker identifier added for traceability.Consistent with the project-wide refactor to add identifiers to walkers.
v2/pkg/engine/plan/provides_fields_visitor.go (1)
42-42: LGTM: Walker identifiers added for traceability.Both walker initializations have been updated with descriptive identifiers ("AddTypenamesVisitor" and "ProvidesVisitor") consistent with the project-wide refactor.
Also applies to: 69-69
v2/pkg/astvalidation/operation_validation.go (1)
33-33: LGTM: Walker identifiers added for traceability.Both walker initializations in
DefaultOperationValidatorandNewOperationValidatorhave been updated with the identifier "OperationValidator", consistent with the project-wide refactor.Also applies to: 67-67
v2/pkg/engine/plan/abstract_selection_rewriter.go (1)
695-695: LGTM! Walker identification improves observability.The addition of the "AbstractFieldPathCollector" identifier to the walker initialization enhances debugging and tracing capabilities without affecting functionality.
v2/pkg/engine/plan/required_fields_visitor.go (1)
67-67: LGTM! Walker identification improves observability.The addition of the "RequiredFieldsVisitor" identifier to the walker initialization enhances debugging and tracing capabilities without affecting functionality.
v2/pkg/engine/plan/key_fields_visitor.go (2)
90-172: LGTM! Error handling improvement enhances robustness.The method signature change adding error return capability is well-implemented:
- All early return paths correctly return
nil(lines 98, 103, 111, 171)- Error condition properly returns the report (line 133)
- Consistent with error propagation patterns in the codebase
175-175: LGTM! Walker identification improves observability.The addition of the "KeyInfoVisitor" identifier to the walker initialization enhances debugging and tracing capabilities without affecting functionality.
v2/pkg/variablesvalidation/variablesvalidation.go (1)
81-81: Walker ID addition LGTMUsing NewWalkerWithID with a descriptive ID improves tracing; no behavior change.
v2/pkg/engine/plan/key_fields_visitor_test.go (1)
486-494: Tests updated appropriately for error-returning collectorSwitch to collectNodesDSVisitor and assert.NoError around collectKeysForPath are correct and improve signal. LGTM.
Also applies to: 495-495, 497-497
v2/pkg/astparser/parser.go (1)
94-95: Post-parse precalc integration looks goodCalling precalculate() only when report has no errors is safe and keeps Parse/ParseWithLimits behavior stable while populating interface implementation caches. LGTM.
Please confirm that documents created without parser (e.g., via Import... helpers) either invoke PopulateInterfaceTypeDefinitionImplementedByObjects or rely on getters that fallback, to avoid regressions.
Also applies to: 111-112, 114-121
v2/pkg/astnormalization/astnormalization.go (1)
206-215: Walker IDs across normalization stages LGTMConsistent NewWalkerWithID usage improves traceability without altering semantics.
Also applies to: 217-217, 221-221, 239-239, 248-248, 256-256, 264-264, 272-272, 286-286, 359-359, 367-367, 370-370, 374-374
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
1042-1047: Ctor/signature migration looks goodNewDataSourceFilter(DataSources, newFieldRefs) and findBestDataSourceSet(nil) usage aligns with the refactor. Please confirm nil newFieldRefs is intended for these tests and that landedTo=nil is a supported path.
v2/pkg/engine/plan/node_selection_builder.go (1)
53-59: Walker IDs and newFieldRefs wiring LGTM
- Using astvisitor.NewWalkerWithID for both walkers is consistent with the new instrumentation.
- newFieldRefs initialization and reset are correct.
- dsFilter constructed with config.DataSources and newFieldRefs, and subsequent FilterDataSources calls match the new API.
Also applies to: 65-79, 85-88, 91-91, 106-106, 141-141
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (2)
100-121: Nice: BFS over responseTree and newFieldRefs filteringBuilding a visit list via TraverseBFS and gating by newFieldRefs minimizes work on secondary runs. LGTM.
178-186: LGTM: Walker identity for tree buildingUsing NewWalkerWithID("TreeBuilderVisitor") and threading fieldInfo improves traceability without affecting behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (2)
72-95: Remove commented-out code.The initialization logic correctly sets up per-datasource visitors with isolated local state. However, the commented-out walker initialization at line 82 should be removed.
Apply this diff to clean up the commented code:
visitor := &collectNodesDSVisitor{ operation: c.operation, definition: c.definition, - // walker: walker, nodes: c.nodes,
251-271: Remove commented-out walker field.The per-datasource visitor structure is well-designed for parallel execution. However, the commented-out walker field at line 252 should be removed if it's no longer needed.
Apply this diff to clean up:
type collectNodesDSVisitor struct { - // walker *astvisitor.Walker operation *ast.Document definition *ast.Documentv2/pkg/ast/ast_interface_type_definition.go (3)
167-178: Nil-check cache to avoid redundant fallback; preallocate result sliceUse impl == nil (not len==0) to detect “not populated”; otherwise you re-enumerate even when the cache is a valid empty slice, hurting hot-path perf. Also preallocate typeNames.
- implementedByObjectRefs := d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions - if len(implementedByObjectRefs) == 0 { + impl := d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions + if impl == nil { // fallback for documents not precalculated (e.g., built programmatically) for _, n := range d.InterfaceTypeDefinitionImplementedByRootNodes(interfaceDefRef) { if n.Kind == NodeKindObjectTypeDefinition { typeNames = append(typeNames, d.ObjectTypeDefinitionNameString(n.Ref)) } } } else { - for _, implementedByObjectRef := range d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions { - typeNames = append(typeNames, d.ObjectTypeDefinitionNameString(implementedByObjectRef)) - } + typeNames = make([]string, 0, len(impl)) + for _, ref := range impl { + typeNames = append(typeNames, d.ObjectTypeDefinitionNameString(ref)) + } }
190-201: Mirror nil-check and prealloc in bytes variantSame rationale as above: distinguish “not populated” via nil and preallocate.
- implementedByObjectRefs := d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions - if len(implementedByObjectRefs) == 0 { + impl := d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions + if impl == nil { // fallback for documents not precalculated (e.g., built programmatically) for _, n := range d.InterfaceTypeDefinitionImplementedByRootNodes(interfaceDefRef) { if n.Kind == NodeKindObjectTypeDefinition { typeNames = append(typeNames, d.ObjectTypeDefinitionNameBytes(n.Ref)) } } } else { - for _, implementedByObjectRef := range d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions { - typeNames = append(typeNames, d.ObjectTypeDefinitionNameBytes(implementedByObjectRef)) - } + typeNames = make([][]byte, 0, len(impl)) + for _, ref := range impl { + typeNames = append(typeNames, d.ObjectTypeDefinitionNameBytes(ref)) + } }
212-225: Clarify cache lifecycle and usage with a doc commentAdd a doc comment explaining when to call PopulateInterfaceTypeDefinitionImplementedByObjects (parser precalc and after programmatic AST changes). This reduces misuse and reliance on slower fallbacks.
+// PopulateInterfaceTypeDefinitionImplementedByObjects precomputes the +// ImplementedByObjectDefinitions cache for all interfaces. +// Call this after parsing (parser precalc) and after programmatic AST mutations +// that change interface implementations (e.g., Import* helpers). func (d *Document) PopulateInterfaceTypeDefinitionImplementedByObjects() { for ref := 0; ref < len(d.InterfaceTypeDefinitions); ref++ { implementedByNodes := d.InterfaceTypeDefinitionImplementedByRootNodes(ref) implementedByObjectRefs := make([]int, 0, len(implementedByNodes)) for _, implementedByNode := range implementedByNodes { if implementedByNode.Kind != NodeKindObjectTypeDefinition { continue } implementedByObjectRefs = append(implementedByObjectRefs, implementedByNode.Ref) } d.InterfaceTypeDefinitions[ref].ImplementedByObjectDefinitions = implementedByObjectRefs } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/ast/ast_interface_type_definition.go(4 hunks)v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/ast/ast_interface_type_definition.go (5)
v2/pkg/ast/ast_description.go (1)
Description(12-17)v2/pkg/engine/resolve/node_object.go (1)
Position(173-176)v2/pkg/ast/ast_object_type_definition.go (1)
TypeList(10-12)v2/pkg/ast/ast_directive.go (1)
DirectiveList(11-13)v2/pkg/ast/ast_field_definition.go (1)
FieldDefinitionList(11-15)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (3)
v2/pkg/operationreport/operationreport.go (2)
Report(9-12)NewReport(14-16)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (2)
TraverseBFS(100-120)NodeSuggestion(14-40)v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (18)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (17)
24-28: LGTM: Well-structured additions for per-datasource visitor pattern.The new fields support efficient parallel node collection with per-datasource state isolation and incremental field processing via
newFieldRefsfiltering.
66-70: LGTM: Clean abstraction for work distribution.The
nodeVisitTaskstruct appropriately encapsulates the data needed for each visitor task with value types that are safe for concurrent access.
97-120: LGTM: Efficient task building with incremental filtering.The BFS traversal with
newFieldRefsfiltering (lines 106-111) enables efficient incremental processing by skipping fields that were processed in previous iterations. The task list is built before goroutine dispatch, ensuring safe concurrent access.
122-162: LGTM: Well-implemented concurrent visitor execution.The concurrency pattern is solid:
- Goroutines receive visitor/report/tasks as parameters, avoiding closure pitfalls
- Proper defer ordering ensures cleanup (lines 136-143)
- Panic recovery with stack traces aids debugging (lines 144-149)
- Per-visitor state isolation prevents data races
- Optional semaphore provides backpressure control
164-189: LGTM: Correct error aggregation and safe post-processing.The implementation correctly waits for all goroutines (line 164), aggregates errors from all reports (lines 166-176), and only modifies shared state after synchronization (lines 180-188). The comment at lines 178-179 is valuable documentation of the safety invariant.
191-204: LGTM: Walker identification enhances debuggability.Using
NewWalkerWithIDwith the identifier "TreeBuilderVisitor" (line 192) enables tracing and debugging across the visitor infrastructure.
219-236: LGTM: Efficient field info collection.The logic correctly collects field information only for newly seen fields (line 235), avoiding redundant work while maintaining the deduplication invariant with
IsFieldSeen(line 220).
273-276: Verify whether maps should be reset.The
reset()method clears slices but doesn't resetlocalSuggestionLookuporlocalSeenKeysmaps. Please confirm this is intentional—if these maps should persist across iterations, consider adding a clarifying comment.
278-330: LGTM: Methods correctly updated for per-datasource visitor pattern.The helper methods (
hasSuggestionForFieldOnCurrentDataSource,hasProvidesConfiguration,isEntityInterface, etc.) are appropriately updated to work with thecollectNodesDSVisitorreceiver without changing their core logic.
331-377: LGTM: Improved error handling with explicit returns.Converting from side-effect error logging to explicit error returns (lines 353, 358, 372) enables proper error propagation in the concurrent visitor execution model. The early validation checks are well-placed.
379-402: LGTM: Correctly adapted to use fieldInfo.The method properly uses
info.enclosingTypeDefinition(line 384) from the fieldInfo map instead of walker state, aligning with the stateless visitor design.
409-518: LGTM: Well-refactored with proper error propagation.The method is correctly updated to:
- Accept
treeNodeIdas a parameter for tree correlation (line 409)- Return errors explicitly rather than using report side effects (lines 415-417, 422-424, 431-433)
- Use
fieldInfomap instead of walker state (line 410)- Store
treeNodeIdin node suggestions for proper tree node correlation (line 510)The error handling is thorough and the logic flow is cleaner than side-effect-based reporting.
520-530: LGTM: Safe shared state updates.The method correctly mutates shared state (
f.nodes) after all concurrent visitors complete. The tree node data updates (lines 525-528) properly correlate suggestions with their tree locations.
539-541: LGTM: Correct inverse mapping helper.The function properly inverts
TreeNodeIDby subtracting the offset, enabling bidirectional tree node ↔ field reference mapping.
552-559: LGTM: Enhanced fieldInfo for stateless visitors.Adding
enclosingTypeDefinition(line 558) enables visitors to access type context without walker state, supporting the stateless per-datasource visitor architecture.
561-596: LGTM: Key enabler of stateless visitor pattern.The method precomputes comprehensive field metadata, including
enclosingTypeDefinition(line 594), during tree building. This enables per-datasource visitors to operate without walker state, supporting efficient parallel execution.
49-64: Verification complete: All callers updated for signature change.The search found exactly one call to
CollectNodes()atv2/pkg/engine/plan/datasource_filter_visitor.go:160. The caller correctly assigns the result to a single variable (keysInfo := f.nodesCollector.CollectNodes()), which matches the new signature that returns only[]DSKeyInfo. No legacy code using the old two-value return pattern was found.v2/pkg/ast/ast_interface_type_definition.go (1)
18-26: ****Verification confirms the implementation is sound:
- No external mutations: Codebase search found zero writes to
ImplementedByObjectDefinitionsoutside the definition file.- Proper lifecycle:
PopulateInterfaceTypeDefinitionImplementedByObjects()is wired into the parser precalc phase (parser.go:119).- Correct fallback logic: The
len() == 0check intentionally handles both nil and empty slices, enabling fallback for non-precalculated documents—a feature, not a flaw.- Preallocation already present: Line 215 uses
make([]int, 0, len(implementedByNodes)), avoiding unnecessary allocations.The exported field is managed safely within its intended lifecycle; the theoretical mutation risk does not manifest in practice.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v2/pkg/ast/ast_interface_type_definition.go (3)
167-182: Fallback logic correctly implemented; minor refactor suggested.The nil-check fallback successfully addresses the previous review concern about backward compatibility for documents not run through precalculate(). Based on learnings.
However, line 179 re-accesses the struct field instead of using the local variable
implementedByObjectRefscaptured at line 167.Apply this diff to use the local variable:
} else { typeNames = make([]string, 0, len(implementedByObjectRefs)) - for _, implementedByObjectRef := range d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions { + for _, implementedByObjectRef := range implementedByObjectRefs { typeNames = append(typeNames, d.ObjectTypeDefinitionNameString(implementedByObjectRef)) } }
193-208: Same refactor opportunity as lines 167-182.Line 205 should use the local variable
implementedByObjectRefsinstead of re-accessing the struct field.Apply this diff:
} else { typeNames = make([][]byte, 0, len(implementedByObjectRefs)) - for _, implementedByObjectRef := range d.InterfaceTypeDefinitions[interfaceDefRef].ImplementedByObjectDefinitions { + for _, implementedByObjectRef := range implementedByObjectRefs { typeNames = append(typeNames, d.ObjectTypeDefinitionNameBytes(implementedByObjectRef)) } }
218-231: Add doc comment for exported method.Public methods in Go should have doc comments explaining their purpose and usage.
Add a doc comment above the method:
// PopulateInterfaceTypeDefinitionImplementedByObjects populates the ImplementedByObjectDefinitions // cache for all interface type definitions in the document. This optimization is typically called // during parsing to improve query planning performance. func (d *Document) PopulateInterfaceTypeDefinitionImplementedByObjects() {Otherwise, the implementation correctly filters and caches object type implementors.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/ast/ast_interface_type_definition.go(4 hunks)v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/ast/ast_interface_type_definition.go (5)
v2/pkg/ast/ast_description.go (1)
Description(12-17)v2/pkg/engine/resolve/node_object.go (1)
Position(173-176)v2/pkg/ast/ast_object_type_definition.go (1)
TypeList(10-12)v2/pkg/ast/ast_directive.go (1)
DirectiveList(11-13)v2/pkg/ast/ast_field_definition.go (1)
FieldDefinitionList(11-15)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (4)
v2/pkg/operationreport/operationreport.go (2)
Report(9-12)NewReport(14-16)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (2)
TraverseBFS(100-120)NodeSuggestion(14-40)v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(274-287)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (15)
v2/pkg/ast/ast_interface_type_definition.go (1)
18-27: LGTM! Cache field added appropriately.The
ImplementedByObjectDefinitionsfield provides an optional cache that enables performance optimization while maintaining backward compatibility through nil initialization.v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (14)
66-94: LGTM! Well-structured per-DS initialization.The
nodeVisitTaskstruct andinitVisitorsmethod cleanly prepare per-data-source visitors with independent state (local keys, suggestions, reports). This design prevents cross-DS contamination and enables parallel processing.
96-188: LGTM! Solid parallelization with proper error handling.The refactored
collectNodesmethod introduces robust per-DS parallel processing:
- Tree traversal with BFS and task queuing (lines 98-119).
- Goroutine-per-DS-visitor pattern with optional concurrency limiting via semaphore (lines 121-161).
- Panic recovery with stack traces for debugging (lines 143-148).
- Error propagation via per-DS reports, aggregated after all visitors complete (lines 165-175).
- Post-goroutine suggestion application prevents data races (lines 179-187).
The pattern is well-implemented and should improve query planning performance.
190-203: LGTM! Walker identification added.The
buildTreemethod now usesNewWalkerWithID(line 191) for walker identification and passesfieldInfoto the visitor (line 197) to enable field metadata collection.
205-248: LGTM! Tree builder now captures field metadata.The
treeBuilderVisitornow collects field information (line 234) including enclosing type definitions, enabling downstream per-DS visitors to access this context.
250-274: LGTM! Per-DS visitor state management.The
collectNodesDSVisitormaintains per-DS state (localSuggestions,localSuggestionLookup,providesEntries) and theresetmethod correctly clears state between runs while preserving capacity.
276-327: LGTM! Helper methods correctly scoped to per-DS visitor.These helper methods (
hasSuggestionForFieldOnCurrentDataSource,hasProvidesConfiguration,isEntityInterface,isInterfaceObject,allKeysHasDisabledEntityResolver) are correctly scoped to the per-DS visitor, enabling independent DS-specific checks.
329-375: LGTM! Provides handling with proper error propagation.The
handleProvidesSuggestionsmethod now returns errors, enabling proper propagation to the per-DS report. The checks for object type definitions (line 339) and field definitions (line 343) correctly use the newenclosingTypeDefinitionfromfieldInfo. Error messages include helpful context.
377-405: LGTM! Union typename and external key checks.Both helper methods correctly use the
enclosingTypeDefinitionfromfieldInfo(line 382) and provide clear boolean checks for union typename suggestions and external key paths.
407-516: LGTM! EnterField refactored with error propagation.The
EnterFieldmethod signature now includesitemIds []intandtreeNodeId uintparameters and returns anerror, enabling proper error propagation from provides handling (lines 413-415) and key collection (lines 420-431). The method correctly:
- Checks both global and local suggestions to prevent duplicates (lines 437-445).
- Propagates
treeNodeIdto node suggestions (line 508).- Tracks local suggestions for deduplication (line 512).
518-528: LGTM! Safe suggestion application after parallel collection.The
applySuggestionsmethod correctly applies per-DS local suggestions to the globalNodeSuggestionsstructure. Called after all goroutines complete (line 180), this avoids data races.
530-539: LGTM! Tree node ID mapping functions.The newly exported
TreeNodeFieldReffunction (lines 537-539) correctly reverses theTreeNodeIDcalculation, enabling mapping from tree node IDs back to field references. The offset of 100 ensures tree node ID 0 remains reserved for the root.
550-594: LGTM! Field info now includes enclosing type definition.The
fieldInfostruct now includesenclosingTypeDefinition ast.Node(line 556), andcollectFieldInfocorrectly captures this from the walker (line 592). This enables per-DS visitors to access type context for provides handling and other type-specific logic.
49-64: Verification confirmed: signature change fully updated.The codebase contains exactly one caller of
CollectNodesatv2/pkg/engine/plan/datasource_filter_visitor.go:160, which correctly assigns the result to a single variable matching the new signature. No breaking patterns found.
14-29: ****The
newFieldRefsmap is not dead code. It's an input parameter populated by the caller before being passed toNewDataSourceFilter. The map is populated bynodeSelectionVisitor.addNewFieldRefs()in node_selection_builder.go and passed toDataSourceFilterat construction time. The gating logic at line 105-109 correctly implements incremental field collection—whenlen(c.newFieldRefs) > 0, only fields present in the map are visited. This is a designed feature, not dead code.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (2)
329-375: Verify enclosingTypeDefinition is always valid.At line 339,
enclosingTypeDefinition.Kindis accessed directly. While thecollectFieldInfomethod (line 595) sets this fromf.walker.EnclosingTypeDefinition, there's no explicit validation that it's a valid non-zero node before accessing itsKindfield.If
fieldInfois missing for afieldRef, line 410 returnsnilearly, so this should be safe. However, consider adding a defensive check at line 339 to ensureenclosingTypeDefinition.Refis valid if the ast.Node type supports zero values.
105-110: Add a comment explainingnewFieldRefslifecycle and the incremental filtering design.The verification confirms the filtering logic is correct. The
newFieldRefsmap is passed fromnodeSelectionVisitor(where it's populated byaddNewFieldRefs()), through the visitor hierarchy tonodesCollector. The guard clauseif len(c.newFieldRefs) > 0properly handles empty maps (processes all fields) versus populated maps (incremental filtering). However, add a brief comment at lines 105-109 explaining this is intentional incremental processing, clarifying that an empty map means first iteration (process all) and a populated map means subsequent iteration (process only new fields).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/engine/plan/datasource_configuration.go(1 hunks)v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go(19 hunks)v2/pkg/engine/plan/federation_metadata.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
v2/pkg/engine/plan/federation_metadata.go (1)
FederationMetaData(10-18)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (4)
v2/pkg/operationreport/operationreport.go (2)
Report(9-12)NewReport(14-16)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (2)
TraverseBFS(100-120)NodeSuggestion(14-40)v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(277-290)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (11)
v2/pkg/engine/plan/datasource_configuration.go (1)
90-101: LGTM! Efficient entity type name indexing.The map initialization and population logic is correct. Building the
entityTypeNamesindex during initialization enables O(1) lookups inHasEntity(), which should improve query planning performance as intended by this PR.v2/pkg/engine/plan/federation_metadata.go (2)
10-18: LGTM! Appropriate field for caching entity type names.The
entityTypeNamesfield provides an efficient lookup structure for entity type membership checks. The private visibility is correct since this is an internal optimization detail.
38-41: LGTM! Clean O(1) entity lookup.The direct map lookup is efficient and correct. The nil-map safety is implicit (returns false for missing keys), which matches the expected behavior when the map is uninitialized.
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (8)
121-188: LGTM: Parallel execution is correctly synchronized.The parallelization is well-designed:
- Each DS visitor operates on its own local state during the parallel phase
- Shared state modifications (
applySuggestions, key aggregation) occur only afterwg.Wait()- Per-DS error handling and panic recovery are properly implemented
- The semaphore-based concurrency limiting (lines 126-128, 139-142) is correctly applied
190-203: LGTM: Walker initialization updated correctly.The change to
NewWalkerWithIDand the addition offieldInfoto the visitor align with the broader refactoring to support per-data-source processing.
205-248: LGTM: Field info collection integrated properly.The
treeBuilderVisitornow collects field metadata during tree construction, which is then used by per-DS visitors. The integration is clean and maintains separation of concerns.
407-519: LGTM: Per-DS visitor logic is well-structured.The
EnterFieldmethod correctly:
- Handles provides suggestions with error propagation
- Collects keys for entities and possible types
- Checks for duplicate suggestions (global and local)
- Creates node suggestions with appropriate metadata
- Uses local state to prevent cross-DS contamination during parallel execution
533-542: LGTM: Tree node ID mapping is correct.The
TreeNodeFieldReffunction correctly invertsTreeNodeIDby subtracting the same offset (100) that was added. This ensures field refs and tree node IDs don't collide with the root node ID.
553-597: LGTM: Field info collection is comprehensive.The
fieldInfostruct andcollectFieldInfomethod properly capture all necessary metadata for per-DS processing, including theenclosingTypeDefinitionwhich enables proper type-based validation downstream.
72-94: The original review concern is incorrect.The verification confirms that
initVisitors()is called at line 157 indatasource_filter_visitor.go, immediately beforeCollectNodes()is invoked at line 160. The method initialization order is correct, and there is no risk of incomplete key collection due to missing initialization.Likely an incorrect or invalid review comment.
49-64: No issues found. Error handling is properly implemented via shared report.The caller at line 79-82 correctly checks
f.report.HasErrors()immediately after callingf.collectNodes(). SincenodesCollectorandDataSourceFiltershare the same report instance, errors fromCollectNodes()are properly propagated and checked before the result is used.
collect fields info along with building tree
This reverts commit a83ecbe.
add a fallback if interface implemented by is not precalculated
devsergiy
force-pushed
the
sergiy/eng-8285-improve-query-planning-time
branch
from
dc50526 to
0305727
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/plan/key_fields_visitor.go (1)
269-270: Fix critical stack bug in currentKeyPath initializationThe bug is confirmed. Line 269 incorrectly appends to
v.keyPathsinstead ofv.currentKeyPath, breaking the stack invariant needed for nested field traversal. This causes:
currentKeyPathto contain duplicated entries from the entirekeyPathsaccumulatorLeaveFieldpops incorrectly since the stack was never properly builtisRootKeyPathExternal()to check wrong indicesThe suggested fix is correct:
- v.currentKeyPath = append(v.keyPaths, fieldKeyPath) + v.currentKeyPath = append(v.currentKeyPath, fieldKeyPath)
🧹 Nitpick comments (9)
v2/pkg/engine/plan/configuration.go (1)
118-118: Consider removing the field now or tracking its deprecation in an issue.The TODO comment helpfully documents that
DisableDefaultMappinghas no effect. Since the PR is already refactoring related code and the enriched summary indicates other files are moving away from this field, consider addressing this now rather than deferring it:
- Option 1 (preferred if truly unused): Remove the field in this PR if it's confirmed to have no effect and no external consumers rely on it.
- Option 2 (if breaking change needs coordination): Open an issue to track its deprecation and removal, then reference that issue in the comment (e.g.,
// TODO(#ISSUE): deprecated, remove in v3).Leaving untracked TODO comments can accumulate technical debt.
Would you like me to search the codebase to verify whether
DisableDefaultMappingis still referenced elsewhere, or help draft an issue to track its removal?v2/pkg/engine/plan/key_fields_visitor_test.go (1)
495-498: Good: assert errors from collectKeysForPath and dedupe behaviorConsider adding one negative test case where a key field is missing (to exercise the TODO error path in getKeyPaths).
v2/pkg/astvisitor/visitor.go (3)
66-108: Expose a setter and update doc to reflect NewWalkerWithID
- Add Walker.SetID(id string) so pooled walkers can be labeled post‑construction.
- Update the “Always use NewWalker” comment to mention NewWalkerWithID/NewDefaultWalkerWithID.
type Walker struct { - walkerID string + walkerID string ... } +// SetID sets a human-readable identifier for profiling/debug. +func (w *Walker) SetID(id string) { w.walkerID = id }Also applies to: 125-128
114-114: Path capacity constantYou changed Path capacity to 64. Extract a package‑level const (e.g., defaultPathCap = 64) to avoid magic numbers in both constructors.
1383-1388: pprof labeling stubConsider guarding with a build tag or debug flag so this can be enabled without editing source.
v2/pkg/engine/plan/key_fields_visitor.go (2)
131-134: Error propagation shapeYou return the local report as error. Ensure operationreport.Report implements error (Error() string) and that callers don’t also rely on input.report side effects. If not guaranteed, return fmt.Errorf with details.
212-215: TODO: missing-node should surface as an errorReplace the TODO with a proper external error on input.report so collectKeysForPath returns a failure when a key field isn’t present in any DS.
v2/pkg/engine/plan/datasource_filter_node_suggestions.go (1)
100-120: Minor: guard nil root and avoid enqueuing nilFunction is correct, but an early return avoids pushing nil and one PopFront.
func TraverseBFS(data tree.Tree[[]int]) iter.Seq2[uint, tree.Node[[]int]] { return func(yield func(uint, tree.Node[[]int]) bool) { - q := queue.New() - q.PushBack(data.Root()) + root := data.Root() + if root == nil { + return + } + q := queue.New() + q.PushBack(root) for { current := q.PopFront() switch node := current.(type) {v2/pkg/engine/plan/node_selection_visitor.go (1)
136-168: Ensure newFieldRefs map is always initializednewFieldRefs is written in addNewFieldRefs; please confirm it’s allocated by the builder before any visit, or initialize/reset it here to be defensive.
func (c *nodeSelectionVisitor) EnterDocument(operation, definition *ast.Document) { c.hasNewFields = false ... if c.skipFieldsRefs == nil { c.skipFieldsRefs = make([]int, 0, 8) } + if c.newFieldRefs == nil { + c.newFieldRefs = make(map[int]struct{}, 16) + } else { + for k := range c.newFieldRefs { + delete(c.newFieldRefs, k) + } + } ... }If builder already guarantees this, consider a brief comment here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
v2/pkg/ast/ast_interface_type_definition.go(4 hunks)v2/pkg/astnormalization/abstract_field_normalizer.go(1 hunks)v2/pkg/astnormalization/astnormalization.go(6 hunks)v2/pkg/astnormalization/variables_mapper.go(1 hunks)v2/pkg/astparser/parser.go(2 hunks)v2/pkg/asttransform/typename_visitor.go(1 hunks)v2/pkg/astvalidation/operation_validation.go(2 hunks)v2/pkg/astvisitor/visitor.go(4 hunks)v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go(7 hunks)v2/pkg/engine/plan/abstract_selection_rewriter.go(1 hunks)v2/pkg/engine/plan/analyze_plan_kind.go(1 hunks)v2/pkg/engine/plan/configuration.go(1 hunks)v2/pkg/engine/plan/datasource_configuration.go(1 hunks)v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go(19 hunks)v2/pkg/engine/plan/datasource_filter_node_suggestions.go(2 hunks)v2/pkg/engine/plan/datasource_filter_visitor.go(5 hunks)v2/pkg/engine/plan/datasource_filter_visitor_test.go(3 hunks)v2/pkg/engine/plan/federation_metadata.go(2 hunks)v2/pkg/engine/plan/key_fields_visitor.go(3 hunks)v2/pkg/engine/plan/key_fields_visitor_test.go(2 hunks)v2/pkg/engine/plan/node_selection_builder.go(4 hunks)v2/pkg/engine/plan/node_selection_visitor.go(5 hunks)v2/pkg/engine/plan/path_builder.go(1 hunks)v2/pkg/engine/plan/planner.go(1 hunks)v2/pkg/engine/plan/provides_fields_visitor.go(2 hunks)v2/pkg/engine/plan/required_fields_visitor.go(1 hunks)v2/pkg/engine/plan/visitor.go(2 hunks)v2/pkg/variablesvalidation/variablesvalidation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- v2/pkg/ast/ast_interface_type_definition.go
- v2/pkg/astvalidation/operation_validation.go
- v2/pkg/engine/plan/abstract_selection_rewriter.go
- v2/pkg/engine/plan/datasource_configuration.go
- v2/pkg/engine/plan/analyze_plan_kind.go
- v2/pkg/astnormalization/variables_mapper.go
- v2/pkg/engine/plan/provides_fields_visitor.go
- v2/pkg/engine/plan/path_builder.go
- v2/pkg/engine/plan/visitor.go
- v2/pkg/variablesvalidation/variablesvalidation.go
- v2/pkg/astnormalization/abstract_field_normalizer.go
- v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
- v2/pkg/engine/plan/required_fields_visitor.go
- v2/pkg/astparser/parser.go
- v2/pkg/asttransform/typename_visitor.go
🧰 Additional context used
🧬 Code graph analysis (8)
v2/pkg/engine/plan/key_fields_visitor.go (2)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (1)
SeenKeyPath(43-47)v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/astnormalization/astnormalization.go (1)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
v2/pkg/engine/plan/datasource_filter_visitor.go (1)
NewDataSourceFilter(35-44)
v2/pkg/engine/plan/planner.go (2)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/astnormalization/inline_fragment_add_on_type.go (1)
InlineFragmentAddOnType(11-17)
v2/pkg/astvisitor/visitor.go (1)
v2/pkg/ast/path.go (2)
Path(34-34)PathItem(27-32)
v2/pkg/engine/plan/datasource_filter_visitor.go (3)
v2/pkg/engine/plan/datasource_configuration.go (2)
DataSource(277-290)DSHash(15-15)v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (2)
KeyIndex(38-41)SeenKeyPath(43-47)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (3)
NewNodeSuggestions(122-124)NodeSuggestions(93-98)TraverseBFS(100-120)
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (4)
v2/pkg/operationreport/operationreport.go (2)
Report(9-12)NewReport(14-16)v2/pkg/engine/plan/datasource_filter_node_suggestions.go (2)
TraverseBFS(100-120)NodeSuggestion(14-40)v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/engine/plan/datasource_configuration.go (1)
DataSource(277-290)
v2/pkg/engine/plan/node_selection_builder.go (2)
v2/pkg/astvisitor/visitor.go (1)
NewWalkerWithID(100-108)v2/pkg/engine/plan/datasource_filter_visitor.go (1)
NewDataSourceFilter(35-44)
🔇 Additional comments (27)
v2/pkg/engine/plan/federation_metadata.go (2)
16-17: No action needed — initialization is correct and complete.The
entityTypeNamesmap is properly initialized and populated:
- Allocated at line 91 in
InitKeys()withmake(map[string]struct{})- Populated at line 97 by iterating through
Keysand adding eachTypeName- Called during initialization (line 87), before
HasEntity()is usedThe optimization correctly replaces O(n) key lookup with O(1) map access while preserving semantic equivalence.
38-41: Code implementation is correct; review comment mischaracterizes the change.The
HasEntitymethod implementation shown (lines 38-41) is identical to the previous version inHEAD~1. It already uses O(1) map lookup and is correctly populated from the Keys configuration inInitKeys(). All callers properly use the boolean result to check entity existence.However, the review comment incorrectly frames this as a "refactoring optimization" when git history shows no actual code change here. The method wasn't refactored—it was already optimized.
Likely an incorrect or invalid review comment.
v2/pkg/engine/plan/key_fields_visitor_test.go (1)
486-493: Rename to collectNodesDSVisitor looks rightConstruction aligns with per-DS visitor pattern; maps preallocated appropriately.
v2/pkg/engine/plan/key_fields_visitor.go (1)
175-176: Nice: labeled walker for profilingAdopting NewWalkerWithID("KeyInfoVisitor") improves traceability.
v2/pkg/engine/plan/planner.go (1)
56-57: Adopt labeled walkersClear IDs (“PrepareOperationWalker”, “PlanningWalker”) aid profiling; no behavior change.
Also applies to: 61-67, 69-73
v2/pkg/astnormalization/astnormalization.go (1)
206-215: Consistent walker IDs across normalization stagesGood instrumentation pass; descriptive IDs make traces readable.
Also applies to: 217-235, 221-223, 239-244, 247-253, 255-261, 263-269, 271-278, 285-295, 356-383
v2/pkg/engine/plan/node_selection_visitor.go (2)
57-67: Helper centralization for skip/new field refsGood consolidation; ensures new refs are tracked whenever skips are added.
500-501: Good: route all additions through addSkipFieldRefs/addNewFieldRefsThis keeps revisit logic consistent after adds/rewrites.
Also applies to: 590-591, 673-674, 717-719
v2/pkg/engine/plan/datasource_filter_visitor_test.go (1)
1042-1043: Tests updated to new DataSourceFilter APIConstructor and method arity changes reflected correctly; enabling selection reasons improves debuggability.
Also applies to: 1047-1048
v2/pkg/engine/plan/node_selection_builder.go (4)
53-53: LGTM: Walker initialization updated to use identifiable walkers.The transition to
NewWalkerWithIDwith string identifiers improves debugging and tracing capabilities.Also applies to: 65-65
57-57: LGTM: New field reference tracking properly initialized and reset.The
newFieldRefsmap enables tracking newly discovered fields across iterations, which is correctly initialized during construction and reset between runs.Also applies to: 87-87
91-91: LGTM: DataSourceFilter construction updated with per-iteration state.The constructor now receives
dataSourcesandnewFieldRefsupfront, aligning with the new internal state management pattern.
106-106: LGTM: FilterDataSources calls updated to match new signature.The method signature simplification (removing
dataSourcesandnodeSuggestionsparameters) reflects the architectural shift to internal state management withinDataSourceFilter.Also applies to: 141-141
v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go (9)
24-28: LGTM: New fields support per-datasource visitor architecture.The added fields (
fieldInfo,newFieldRefs,dsVisitors,dsVisitorsReports) enable proper isolation of per-datasource state and incremental field processing.
49-64: LGTM: CollectNodes simplified to return only keys.The return signature change reflects that
nodes(NodeSuggestions) are now managed internally rather than returned. The explicit key reset on line 56 ensures clean state between iterations.
72-94: LGTM: Per-datasource visitor initialization with proper state isolation.Each datasource visitor receives isolated local state (
keys,localSeenKeys,localSuggestionLookup) while sharing read-only references to common structures. This design supports safe concurrent execution.
96-163: LGTM: Concurrent per-datasource node collection with proper synchronization.The goroutine-based collection uses:
- Semaphore for controlled concurrency
- WaitGroup for completion tracking
- Per-DS isolated state to avoid races
- Panic recovery with error reporting (lines 143-148)
The
newFieldRefsfiltering (lines 105-110) correctly skips fields not added in the current iteration.
165-188: LGTM: Sequential result aggregation after concurrent collection.Error checking and result aggregation (suggestions, keys, seenKeys) occur sequentially after all goroutines complete, avoiding any race conditions. The comment on line 177 correctly notes the safety requirement.
190-203: LGTM: Tree builder updated with identifiable walker and field info.The tree builder now uses
NewWalkerWithIDand receivesfieldInfoto enable enriched field metadata collection includingenclosingTypeDefinition.
218-235: LGTM: Field info collection enriched with enclosing type definition.The
collectFieldInfomethod now capturesenclosingTypeDefinitionfrom the walker (line 598), which is used for provides directive handling (line 339) and type checking (line 382).Also applies to: 565-599
250-522: LGTM: Per-datasource visitor with isolated state and improved error handling.The
collectNodesDSVisitor:
- Maintains isolated per-DS state (
localSuggestions,localSuggestionLookup,localSeenKeys)- Returns errors from
EnterFieldandhandleProvidesSuggestionsfor proper propagation- Uses
localSuggestionLookup(line 448) to prevent duplicate suggestions within an iteration- Properly resets state via
reset()(lines 271-274)
543-545: LGTM: Helper function for bidirectional tree node ID mapping.
TreeNodeFieldRefcorrectly invertsTreeNodeID(line 536-541), enabling mapping from tree node IDs back to field references.v2/pkg/engine/plan/datasource_filter_visitor.go (5)
25-26: LGTM: DataSourceFilter refactored with internal state management.The addition of
newFieldRefs(line 25),dataSources(line 26), andnodesCollector(line 32) fields supports the new architecture where datasources and iteration state are managed internally rather than passed as parameters.Also applies to: 32-33, 35-43
56-76: LGTM: FilterDataSources signature simplified with internal datasource management.The method signature reduction from 4 to 2 parameters reflects the architectural shift:
dataSourcesnow managed internally (line 67)nodeSuggestionsmanaged internally viaf.nodes- Only per-iteration parameters (
landedTo,fieldDependsOn) passed
78-99: LGTM: findBestDataSourceSet updated to use internal node collection.The method now calls
f.collectNodes()(line 79) which manages node collection internally usingf.nodesCollector.
143-195: LGTM: Internal nodesCollector with lazy initialization and proper lifecycle.The
collectNodesmethod now:
- Lazily initializes
nodesCollectoron first call (line 144)- Calls
initVisitors()once during initialization (line 157)- Maintains
seenKeysacross iterations for deduplication (line 152)- Preserves deterministic datasource ordering (lines 186-190)
384-627: LGTM: Duplicate node selection updated to use BFS traversal helper.The traversal mechanism changed to use
TraverseBFS(f.nodes.responseTree)(line 384), which provides an iterator-based BFS traversal. The selection logic remains unchanged.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.235](v2.0.0-rc.234...v2.0.0-rc.235) (2025-10-29) ### Bug Fixes * improve query planning time ([#1326](#1326)) ([373cf2a](373cf2a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved query planning time performance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary by CodeRabbit
Performance Improvements
Bug Fixes & Improvements
Tests
Checklist