[matthewpeterkort]

Summary

Closes #196.

BuildGraphPhysicalPlan prepared each graph in four full-tree walks plus two recursive deep-copy helpers: default-match normalization, semantic.ValidateSemanticGraph, a traversal-count walk, a graph-only validation walk, a second deep copy for the required-prefix semijoin view, a recomputed traversal count for pathSets capacity, and a separate required-prefix presence walk. The render walk also repeated the empty-mode defaulting a third time inline.

This PR replaces the two recursive copy helpers and the two standalone walks with one private preparation traversal (prepareGraphPlanViews / prepareGraphView) that derives, in a single walk:

• the default-normalized tree (empty child match modes → REQUIRED),
• the required-prefix semijoin view (descendants beyond an optional branch marked OPTIONAL),
• the traversal declaration count (used for both the 32 cap and pathSets capacity), and
• whether any required-prefix traversal exists (path_root inclusion).

semantic.ValidateSemanticGraph and graph-specific validateGraphNode stay independent validators; their contracts differ (catalog/alias/edge validation vs. graph-only refusal of dataframe shaping fields), so they were not merged.

Reduced reasoning burden (not line count)

• One place to reason about match-mode semantics. Before: empty-mode defaulting appeared in three sites (normalizer, required-prefix walk, render walk) and the semijoin view was derived from the normalized tree by a second recursion whose correctness argument required knowing the input was already normalized (MatchMode.Required() is strict equality, so a raw "" would have silently flipped prefix propagation). Now defaulting, view derivation, counting, and prefix presence share one recursion with one matchMode decision and one childAncestorsRequired propagation rule.
• The two views are structurally coupled by construction. The semijoin view can no longer drift from the normalized tree — there is exactly one node-construction site producing both.
• Callers of the preparation step see one invariant: after prepareGraphPlanViews, the tree has no empty child modes, the count is final, and the prefix bit agrees with the view. The old code re-derived these ad hoc at three later sites (cap check, pathSets capacity, path_root inclusion), each a separate walk with its own defaulting logic to re-verify.
• Net production diff: −64/+41 lines, with five functions replaced by two.

Behavior preserved (pinned by tests)

• Exact errors and first-failure precedence: semantic validation → 32-declaration cap (still reported before plan construction) → graph-only shaping validation. New tests pin all three precedence crossings, including cap-vs-shaping and semantic-vs-cap.
• Path-sensitive semijoin derivation retained (not a flat "has required" bit): required→required, required→optional, optional→required, and branching-prefix cases assert exact PathSets and exact required_*_result bind sets; required under optional still keeps the path_root fallback and emits no root semi-join, while required chains before optional still drop the fallback.
• Omitted ("") and whitespace-only child modes compile byte-identical to explicit REQUIRED (reflect.DeepEqual on full plans); root match-mode handling unchanged (ValidateSemanticGraph root refusal asserted verbatim).
• Caller-provided graph never mutated; identical input compiles to DeepEqual plans (determinism contract).
• Render walk's inline empty-mode fallback removed: provably unreachable after the prepared normalization + validateGraphNode, which leaves only REQUIRED/OPTIONAL child modes before the walk.
• No behavior, ordering, naming, or feature changes; no new abstractions or registries; zero handwritten code comments (scripts/check_no_code_comments.py: scanned=2 violations=0).

Verification (matched/passed/skipped recorded)

Ticket checks[]:

• git diff --check → clean.
• go test -json ./internal/dataframe/compiler/lower -run TestBuildGraphPhysicalPlan -count=1 → matched 12, passed 12, failed 0, skipped 0 (all 12 listed; existing 3 tests unchanged, 9 new cases added purely additively — no existing expectation was modified; new expectations were validated against the pre-change baseline, and they caught one real regression during development before it could ship).

Focused suites (issue verification section + consumers of BuildGraphPhysicalPlan):

• ./internal/dataframe/compiler/lower (full): matched 320, passed 320, skipped 0.
• ./internal/dataframe/compiler (graph conformance/contract paths incl. CompileResolvedGraphQuery): matched 538, failed 0, skipped 73 (all *AgainstArango live Arango tests).
• ./internal/dataframe/compiler/render/aql: matched 109, failed 0, skipped 0.
• ./internal/dataframe/semantic: matched 55, failed 0, skipped 0.
• ./internal/api/graphql/graph/query (production caller): matched 51, failed 0, skipped 7 (live PostgreSQL tests).
• ./internal/dataframe/compiler/render/postgres (PostgreSQL graph render consumers): matched 1260, failed 0, skipped 192 (live-DB gated). TestGraph20 offline set: 7 passed / 2 skipped (LOOM_TEST_POSTGRES_URL unset).
• go build ./... and go vet ./internal/dataframe/compiler/... clean.

Incomplete evidence (honest record): the live PostgreSQL 18 and Arango integration suites (the 2 TestGraph20Live* tests, 73 *AgainstArango, 7 graphql live PG tests, and the render/postgres live delta audits) could not run in this sandbox: no docker binary and LOOM_TEST_POSTGRES_URL/Arango URLs unset. They skip by design, not by tool error. All offline tests that consume BuildGraphPhysicalPlan passed.

Scope

Files changed: internal/dataframe/compiler/lower/graph_plan.go, internal/dataframe/compiler/lower/graph_plan_test.go (the ticket's owned set only). Base: feature/postgres-backend @ 80e88771.

Review status

Native AO review not started (coordinator-owned per batch process).

[matthewpeterkort]

Verdict: APPROVED — ready; no changes required. (Posted as COMMENT because GitHub rejects APPROVE/REQUEST_CHANGES on a PR from the author account; the machine-readable verdict is submitted to AO separately.)

Scope. One commit refactors internal/dataframe/compiler/lower/graph_plan.go so a single traversal (prepareGraphPlanViews/prepareGraphView) prepares the normalized graph, the required-prefix (semi-join) view, the traversal declaration count, and the required-prefix flag, replacing normalizeGraphMatchModes, graphRequiredSemiJoinRoot/Node, countGraphTraversals, and graphHasRequiredTraversal; plus 220 lines of new table tests in graph_plan_test.go.

Equivalence analysis vs base 80e8877 (all checks pass).

• Normalized view: non-root nodes get whitespace-only MatchMode defaulted to REQUIRED before ValidateSemanticGraph, matching the removed normalizeGraphMatchModes; the root MatchMode stays untouched in both versions, so the root-mode rejection and its error text are preserved.
• requiredPrefixView is structurally identical to the old graphRequiredSemiJoinRoot output: nodes outside the all-REQUIRED ancestor chain are forced to OPTIONAL while requiredness propagation uses the pre-override normalized mode via ancestorsRequired && matchMode == REQUIRED, exactly like graphRequiredSemiJoinNode. Consumers (appendRequiredTraversalMatchFilters, requiredSemanticRouteKey, buildRequiredTraversalExists) only read MatchMode, Children, ResourceType, EdgeLabel, Filters, and OccurrenceID; the views replace only MatchMode/Children and share remaining slices without mutating them.
• hasRequiredPrefix matches graphHasRequiredTraversal: true iff some non-root node is REQUIRED with all ancestors REQUIRED (per-node hasRequiredPrefix = ancestorsRequired && node.required, OR-folded).
• traversalCount matches countGraphTraversals, and the validation order (ValidateSemanticGraph, then the 32-declaration cap, then validateGraphNode) is preserved; the new boundary tests pin that ordering.
• The removed local MatchMode default in walk is safe: walk now iterates the normalized tree, where trimmed-empty modes can no longer appear, so PathExtend.MatchMode values are unchanged.
• Children slices are freshly allocated at every level, so the caller graph is never mutated (also pinned by TestBuildGraphPhysicalPlanLeavesCallerGraphUnmodified).

Other checks. No remaining references to the removed helpers anywhere in the repo; test helper names are unique within package lower; all referenced IR constants (PhysicalPathSeedOp, PhysicalFilterOp, PhysicalExistsPredicate), semantic types, and schema routes (Condition focus_Condition Observation, Patient subject_Patient edges) exist, including the proven-route check in resolveStorageRoute. The new tests lock omitted-match-mode defaulting, required-prefix path sensitivity (including the branching case), the 32/33 declaration limit boundary, validation-vs-cap precedence, and caller immutability. All CI check runs, including Tests / Go unit tests, are green on head 11a6d2e.

Optional (no change required). (1) Consider bundling the four return values of prepareGraphView into a small view struct if it grows further. (2) The reason prepareGraphPlanViews must stay separate from prepareGraphView is subtle — normalizing the root MatchMode would trip the ValidateSemanticGraph root check — worth recording in the PR description. (3) Several tests assert complete error strings; good as contract pins but brittle to message rewording.

Review basis. Static diff review against base 80e8877 plus consumer/source tracing; the reviewer ran no builds or tests locally (review-only), so CI on head is the test evidence.