chore: improve debug formatting in path builder#1355
Conversation
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughUpdates internal debug and logging output across planning and node-selection code paths, adding run-aware headers and consolidated messages; changes a helper signature ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/plan/path_builder.go (1)
46-121: Run-aware planning-path logs look good; consider minor tweaks to run index and indentationThe new “CreatePlanningPaths” and per‑run headers, plus the structured planner sections (datasource, dependencies, required fields, paths), make the debug output much easier to scan and are properly gated on
PrintPlanningPaths.Two small, optional polish points:
- Run index after the revisit loop
After the secondary‑run loop,
ialready reflects the last executed planning run. Passingi + 1intoprintPlanningPathswill label the final dump as one run higher than was actually walked (e.g., 3 walks but “Run #4”). Usingikeeps the numbering consistent with the previous “Planned paths on run #%d” messages.
- Indenting individual path lines
Inside
printPlanningPaths, path entries are printed without indentation, whereas the surrounding section uses\t\tprefixes. IfpathConfiguration.String()does not itself indent, the current output will visually “break out” of the indented block.You could address both with a small tweak:
@@ func (p *PathBuilder) CreatePlanningPaths(operation, definition *ast.Document, report *operationreport.Report) []PlannerConfiguration { - if hasRemovedPaths && p.config.Debug.PrintPlanningPaths { - debugMessage("Paths after removing unnecessary fragment paths:") - p.printPlanningPaths(i + 1) - } + if hasRemovedPaths && p.config.Debug.PrintPlanningPaths { + debugMessage("Paths after removing unnecessary fragment paths:") + p.printPlanningPaths(i) + } @@ func (p *PathBuilder) printPlanningPaths(run int) { - fmt.Println("\t\tPaths:") + fmt.Println("\t\tPaths:") planner.ForEachPath(func(path *pathConfiguration) (shouldBreak bool) { - fmt.Println(path.String()) + fmt.Printf("\t\t%s\n", path.String()) return false })[note: if you intentionally treat “after removing unnecessary fragment paths” as an extra “run”, feel free to keep
i+1; in that case you might want to mention that explicitly in the debug header.]Also applies to: 137-186
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
execution/engine/execution_engine_test.go(1 hunks)v2/pkg/engine/plan/node_selection_builder.go(4 hunks)v2/pkg/engine/plan/path_builder.go(6 hunks)v2/pkg/engine/plan/path_builder_visitor.go(1 hunks)
⏰ 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 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (3)
execution/engine/execution_engine_test.go (1)
263-271: Test debug option comment is fine as-isThe added commented
PrintNodeSuggestionsflag keeps all planner debug toggles co-located and doesn’t affect runtime behavior. No changes needed.v2/pkg/engine/plan/path_builder_visitor.go (1)
1227-1266: Potentially-missing-path logging and tracking remain consistentThe updated
handleMissingPathnow logs “Found potentially missing path” only whenPrintPlanningPathsis enabled and directly records the path inpotentiallyMissingPathTracker, which is then consumed bypopulateMissingPaths(). This keeps behavior intact while standardizing debug output.v2/pkg/engine/plan/node_selection_builder.go (1)
91-165: Run-aware node-selection logging is clear and non-invasiveThe revised messages (“SelectNodes. Initial operation…”, “Selected nodes on run #1…”, and “Selected nodes on additional run #%d.” plus “Updated node suggestions”) improve trace readability while staying fully behind the existing debug flags. No behavioral change.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/plan/path_builder.go(6 hunks)
🔇 Additional comments (6)
v2/pkg/engine/plan/path_builder.go (6)
48-48: LGTM! Clear visual separation for debug output.The separator improves readability of the debug header.
74-76: LGTM! Run labeling for initial planning.The explicit "run #1" label and corresponding parameter improve traceability of planning iterations.
92-94: LGTM! Consistent run numbering for secondary runs.The run counter correctly labels secondary planning iterations.
137-149: LGTM! Cleaner conditional output.Printing only when conditions are true reduces noise and improves debug output clarity.
151-179: LGTM! Significantly improved debug structure.The run-aware output, conditional dependency printing, and enhanced formatting make the planning paths much easier to read and debug. The signature change is internal and well-integrated.
181-188: LGTM! Consistent indentation for missing paths.The improved formatting aligns well with the other debug output enhancements.
This produces format that is easier to read: ``` CreatePlanningPaths =================== Found potentially missing path query.accounts.complex Planned paths on initial run #1: should revisit has missing paths has fields waiting for dependency Missing paths: query.accounts.complex Run #1. Planner ID 0 Parent = query Datasource ID = id-1, name = id-1, hash = 11987355753863978781 Paths: {"ds":11987355753863978781,"path":"query","shouldWalkFields":true,"pathType":"parent"} {"ds":11987355753863978781,"path":"query.accounts","fieldRef": 2,"typeName":"Query","shouldWalkFields":true,"isRootNode":true,"pathType":"field"} {"ds":11987355753863978781,"path":"query.accounts.id","fieldRef": 0,"typeName":"User","shouldWalkFields":true,"isRootNode":true,"pathType":"field"} {"ds":11987355753863978781,"path":"query.accounts.__typename","fieldRef": 7,"typeName":"User","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} Run #1. Planner ID 1 Parent = query.accounts Datasource ID = id-2, name = id-2, hash = 14787246084284009732 Depends on planner IDs: [0] Required fields: key {"type_name":"User","selection_set":"id","RemappedPaths":null} Paths: {"ds":14787246084284009732,"path":"query.accounts","shouldWalkFields":true,"pathType":"parent"} {"ds":14787246084284009732,"path":"query.accounts.nested","fieldRef": 3,"typeName":"User","shouldWalkFields":true,"isRootNode":true,"pathType":"field"} {"ds":14787246084284009732,"path":"query.accounts.nested.__typename","fieldRef": 4,"typeName":"Nested","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} {"ds":14787246084284009732,"path":"query.accounts.nested.property","fieldRef": 5,"typeName":"Nested","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} {"ds":14787246084284009732,"path":"query.accounts.nested.name","fieldRef": 6,"typeName":"Nested","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} Planned paths on run #2: Run #2. Planner ID 0 Parent = query Datasource ID = id-1, name = id-1, hash = 11987355753863978781 Paths: {"ds":11987355753863978781,"path":"query","shouldWalkFields":true,"pathType":"parent"} {"ds":11987355753863978781,"path":"query.accounts","fieldRef": 2,"typeName":"Query","shouldWalkFields":true,"isRootNode":true,"pathType":"field"} {"ds":11987355753863978781,"path":"query.accounts.id","fieldRef": 0,"typeName":"User","shouldWalkFields":true,"isRootNode":true,"pathType":"field"} {"ds":11987355753863978781,"path":"query.accounts.__typename","fieldRef": 7,"typeName":"User","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} Run #2. Planner ID 1 Parent = query.accounts Datasource ID = id-2, name = id-2, hash = 14787246084284009732 Depends on planner IDs: [0] Required fields: key {"type_name":"User","selection_set":"id","RemappedPaths":null} Paths: {"ds":14787246084284009732,"path":"query.accounts","shouldWalkFields":true,"pathType":"parent"} {"ds":14787246084284009732,"path":"query.accounts.nested","fieldRef": 3,"typeName":"User","shouldWalkFields":true,"isRootNode":true,"pathType":"field"} {"ds":14787246084284009732,"path":"query.accounts.nested.__typename","fieldRef": 4,"typeName":"Nested","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} {"ds":14787246084284009732,"path":"query.accounts.nested.property","fieldRef": 5,"typeName":"Nested","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} {"ds":14787246084284009732,"path":"query.accounts.nested.name","fieldRef": 6,"typeName":"Nested","shouldWalkFields":true,"isRootNode":false,"pathType":"field"} Run #2. Planner ID 2 Parent = query.accounts Datasource ID = id-1, name = id-1, hash = 11987355753863978781 Depends on planner IDs: [0 1] Required fields: required by complex: {"type_name":"User","field_name":"complex","selection_set":"nested { property name }","RemappedPaths":{}} key {"type_name":"User","selection_set":"id","RemappedPaths":null} Paths: {"ds":11987355753863978781,"path":"query.accounts","shouldWalkFields":true,"pathType":"parent"} {"ds":11987355753863978781,"path":"query.accounts.complex","fieldRef": 1,"typeName":"User","shouldWalkFields":true,"isRootNode":true,"pathType":"field"} ```
ysmolski
force-pushed
the
yury/format_debugging_path_builder
branch
from
1faead2 to
a7377e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/path_builder.go (1)
151-179: Address previous feedback: Format inconsistencies remain.The run-aware signature change is good, but several formatting issues from previous review have not been addressed:
- Tab indentation (lines 153, 154, 156, 158, 163, 166, 169, 172): Use 2 spaces instead of tabs for consistent rendering.
- Line 154: Should use
ParentPath:instead ofParent =per previous feedback.- Line 156: Should use colons instead of equals for consistency with other labels.
Based on past review comments, apply this diff:
func (p *PathBuilder) printPlanningPaths(run int) { for i, planner := range p.visitor.planners { - fmt.Printf("\n\tRun #%d. Planner ID %d\n", run, i) - fmt.Printf("\t\tParent = %s\n", planner.ParentPath()) + fmt.Printf("\nRun #%d. Planner ID %d\n", run, i) + fmt.Printf(" ParentPath: %s\n", planner.ParentPath()) ds := planner.DataSourceConfiguration() - fmt.Printf("\t\tDatasource ID = %s, name = %s, hash = %d\n", ds.Id(), ds.Name(), ds.Hash()) + fmt.Printf(" Datasource ID: %s, name: %s, hash: %d\n", ds.Id(), ds.Name(), ds.Hash()) if len(planner.ObjectFetchConfiguration().dependsOnFetchIDs) > 0 { - fmt.Printf("\t\tDepends on planner IDs: %v\n", planner.ObjectFetchConfiguration().dependsOnFetchIDs) + fmt.Printf(" Depends on planner IDs: %v\n", planner.ObjectFetchConfiguration().dependsOnFetchIDs) } requiredFields := planner.RequiredFields() if requiredFields != nil && len(*requiredFields) > 0 { - fmt.Println("\t\tRequired fields:") + fmt.Println(" Required fields:") for _, field := range *requiredFields { if field.FieldName != "" { - fmt.Printf("\t\t\trequired by %s: %s\n", field.FieldName, field) + fmt.Printf(" required by %s: %s\n", field.FieldName, field) continue } - fmt.Println("\t\t\tkey:", field) + fmt.Println(" key:", field) } } - fmt.Println("\t\tPaths:") + fmt.Println(" Paths:") planner.ForEachPath(func(path *pathConfiguration) (shouldBreak bool) { fmt.Println(path.String()) return false }) fmt.Println() } }
♻️ Duplicate comments (1)
v2/pkg/engine/plan/path_builder.go (1)
182-188: Address previous feedback: Replace tabs with spaces in missing paths output.The tab indentation should be replaced with spaces for consistent rendering across environments.
Apply this diff:
func (p *PathBuilder) printMissingPaths() { if p.visitor.hasMissingPaths() { - fmt.Printf("\n\tMissing paths:\n") + fmt.Printf("\n Missing paths:\n") for path := range p.visitor.missingPathTracker { - fmt.Printf("\t\t%v\n", path) + fmt.Printf(" %v\n", path) } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
execution/engine/execution_engine_test.go(1 hunks)v2/pkg/engine/plan/node_selection_builder.go(4 hunks)v2/pkg/engine/plan/path_builder.go(6 hunks)v2/pkg/engine/plan/path_builder_visitor.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v2/pkg/engine/plan/path_builder_visitor.go
- execution/engine/execution_engine_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/plan/path_builder.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceConfiguration(272-275)v2/pkg/ast/path.go (1)
FieldName(23-23)
⏰ 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 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (9)
v2/pkg/engine/plan/node_selection_builder.go (5)
99-99: LGTM: Improved debug header clarity.The addition of function context and visual separator improves readability and aligns with the formatting style used in path_builder.go.
123-123: LGTM: Run-aware messaging improves clarity.The explicit run number makes the debug output more structured and easier to follow across multiple planning iterations.
149-149: LGTM: Clearer additional run terminology.The "additional run" terminology and run numbering (i+1) correctly distinguishes secondary runs from the initial run.
153-153: LGTM: Simplified wording."Updated" is more concise and user-friendly than "Recalculated" while conveying the same information.
162-162: LGTM: Message consolidation improves readability.Combining the two messages into one line with the boolean value in parentheses makes the debug output more compact without losing information.
v2/pkg/engine/plan/path_builder.go (4)
48-48: LGTM: Consistent header formatting.The header format matches the style introduced in node_selection_builder.go, providing clear visual separation for debug output.
74-76: LGTM: Run-aware output initialization.The explicit run #1 labeling and updated function signature improve debug output structure and consistency across planning iterations.
92-94: LGTM: Consistent secondary run labeling.The run numbering (i+1) correctly labels secondary runs starting from #2, maintaining consistency with the initial run #1 format.
116-117: LGTM: Clear final state labeling.The "Final paths" header clearly distinguishes the post-cleanup state, and using
i(the last actual run number) correctly represents when these paths were generated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/plan/path_builder.go (1)
172-176: Consider adding indentation to path output for consistency.The path JSON objects are printed without indentation, while other nested elements (e.g., required fields, missing paths) use 4-space indentation. This may be intentional for easier copy-paste of JSON, but if consistency is preferred:
fmt.Println(" Paths:") planner.ForEachPath(func(path *pathConfiguration) (shouldBreak bool) { - fmt.Println(path.String()) + fmt.Printf(" %s\n", path.String()) return false })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/plan/path_builder.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/plan/path_builder.go (2)
v2/pkg/engine/plan/datasource_configuration.go (1)
DataSourceConfiguration(272-275)v2/pkg/ast/path.go (1)
FieldName(23-23)
🔇 Additional comments (5)
v2/pkg/engine/plan/path_builder.go (5)
74-76: LGTM!The explicit "initial run #1" labeling provides clear context for the debug output, and the run number is correctly passed to
printPlanningPaths.
91-95: LGTM!The run numbering is consistent—initial run is #1, and secondary runs correctly start at #2 (
i+1whereistarts at 1).
115-118: Previous feedback addressed.The "Final paths after removing unnecessary fragment paths" header now clearly indicates this is post-cleanup output rather than a new run, addressing the earlier concern about misleading run numbers.
137-149: Previous feedback addressed: tabs replaced with 2-space indentation.The conditional status printing now uses consistent 2-space indentation as requested in prior reviews.
181-187: LGTM!The missing paths output uses consistent indentation (2-space for header, 4-space for items).
2 participants
This produces format that is easier to read:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.