From 11a6d2e548d8806e377340b5439d336006032e53 Mon Sep 17 00:00:00 2001 From: Matthew Peterkort <33436238+matthewpeterkort@users.noreply.github.com> Date: Fri, 13 Nov 2026 18:43:53 +0000 Subject: [PATCH] refactor(compiler): prepare graph normalized and required-prefix views in one traversal Collapse graph plan preparation from four full-tree walks plus two recursive deep-copy helpers into one private traversal that derives the default-normalized tree, its required-prefix semijoin view, the traversal declaration count, and required-prefix presence together. Behavior preserved: empty child match modes still normalize to REQUIRED, root match mode handling is unchanged, semijoin derivation stays path-sensitive, the 32-declaration cap still reports before plan construction, validator precedence (semantic validation, cap, graph-only shaping checks) is unchanged, and the caller-provided graph is never mutated. The render walk's duplicated empty-mode fallback is removed because the prepared tree is now the single defaulting point. Adds regression cases pinning omitted/default/whitespace match modes, required->required, optional->required, required->optional, branching required prefixes, PathSets root-fallback inclusion, required semi-join binding sets and ordering, the declaration-limit boundary with first-failure precedence across all three validators, and caller-graph immutability plus determinism. Covers https://github.com/calypr/loom/issues/196 --- .../dataframe/compiler/lower/graph_plan.go | 105 ++++----- .../compiler/lower/graph_plan_test.go | 220 ++++++++++++++++++ 2 files changed, 261 insertions(+), 64 deletions(-) diff --git a/internal/dataframe/compiler/lower/graph_plan.go b/internal/dataframe/compiler/lower/graph_plan.go index 4438c3916..adca37a34 100644 --- a/internal/dataframe/compiler/lower/graph_plan.go +++ b/internal/dataframe/compiler/lower/graph_plan.go @@ -21,11 +21,12 @@ func BuildGraphPhysicalPlan(output semantic.OutputPlan, context semantic.Executi if strings.TrimSpace(context.Project) == "" { return ir.PhysicalPlan{}, fmt.Errorf("semantic plan project is required") } - output.Root = normalizeGraphMatchModes(output.Root) + normalizedRoot, requiredPrefixView, traversalCount, hasRequiredPrefix := prepareGraphPlanViews(output.Root) + output.Root = normalizedRoot if err := semantic.ValidateSemanticGraph(output.Root); err != nil { return ir.PhysicalPlan{}, err } - if countGraphTraversals(output.Root) > maxGraphTraversalDeclarations { + if traversalCount > maxGraphTraversalDeclarations { return ir.PhysicalPlan{}, fmt.Errorf("graph traversal declaration count exceeds %d", maxGraphTraversalDeclarations) } if err := validateGraphNode(output.Root, true); err != nil { @@ -57,7 +58,7 @@ func BuildGraphPhysicalPlan(output semantic.OutputPlan, context semantic.Executi if err := appendRootPhysicalFilters(&physical, output.Root); err != nil { return ir.PhysicalPlan{}, err } - if err := appendRequiredTraversalMatchFilters(&physical, graphRequiredSemiJoinRoot(output.Root)); err != nil { + if err := appendRequiredTraversalMatchFilters(&physical, requiredPrefixView); err != nil { return ir.PhysicalPlan{}, err } @@ -70,8 +71,8 @@ func BuildGraphPhysicalPlan(output semantic.OutputPlan, context semantic.Executi }}, }) - pathSets := make([]string, 0, countGraphTraversals(output.Root)+1) - if !graphHasRequiredTraversal(output.Root) { + pathSets := make([]string, 0, traversalCount+1) + if !hasRequiredPrefix { pathSets = append(pathSets, "path_root") } routeOrder := 1 @@ -97,10 +98,6 @@ func BuildGraphPhysicalPlan(output semantic.OutputPlan, context semantic.Executi traversal := traversalResult.Traversal traversal.SourceVariable = sourceVariable setVariable := fmt.Sprintf("path_%d", index) - matchMode := child.MatchMode - if strings.TrimSpace(string(matchMode)) == "" { - matchMode = spec.TraversalMatchRequired - } pathSets = append(pathSets, setVariable) scope := make([]ir.PhysicalOperation, 0, 8+len(child.Filters)) scope = appendProjectScope(scope, []string{edgeVariable, targetVariable}, child.EdgeLabel, child) @@ -116,7 +113,7 @@ func BuildGraphPhysicalPlan(output semantic.OutputPlan, context semantic.Executi Variable: setVariable, SourceVariable: sourcePathSet, SourcePath: nil, Traversal: traversal, Node: ir.PhysicalPathNode{Alias: graphAlias(child, child.Alias), ResourceType: child.ResourceType, Value: ir.PhysicalValue{Variable: targetVariable}}, Relationship: ir.PhysicalPathRelationship{Alias: child.Alias, LabelBindKey: traversal.EdgeLabelBindKey, FromResourceType: parent.ResourceType, ToResourceType: child.ResourceType}, - MatchMode: string(matchMode), RouteOrder: index, Scope: scope, + MatchMode: string(child.MatchMode), RouteOrder: index, Scope: scope, }, }) if err := walk(child, setVariable); err != nil { @@ -135,39 +132,48 @@ func BuildGraphPhysicalPlan(output semantic.OutputPlan, context semantic.Executi return physical, nil } -func normalizeGraphMatchModes(node semantic.SemanticNode) semantic.SemanticNode { - copy := node - copy.Children = make([]semantic.SemanticNode, len(node.Children)) - for index, child := range node.Children { - childCopy := normalizeGraphMatchModes(child) - if strings.TrimSpace(string(childCopy.MatchMode)) == "" { - childCopy.MatchMode = spec.TraversalMatchRequired - } - copy.Children[index] = childCopy +func prepareGraphPlanViews(root semantic.SemanticNode) (semantic.SemanticNode, semantic.SemanticNode, int, bool) { + normalized := root + requiredPrefixView := root + normalized.Children = make([]semantic.SemanticNode, len(root.Children)) + requiredPrefixView.Children = make([]semantic.SemanticNode, len(root.Children)) + traversalCount := len(root.Children) + hasRequiredPrefix := false + for index, child := range root.Children { + childNormalized, childRequiredPrefixView, childCount, childHasRequiredPrefix := prepareGraphView(child, true) + normalized.Children[index] = childNormalized + requiredPrefixView.Children[index] = childRequiredPrefixView + traversalCount += childCount + hasRequiredPrefix = hasRequiredPrefix || childHasRequiredPrefix } - return copy + return normalized, requiredPrefixView, traversalCount, hasRequiredPrefix } -func graphRequiredSemiJoinRoot(node semantic.SemanticNode) semantic.SemanticNode { - copy := node - copy.Children = make([]semantic.SemanticNode, len(node.Children)) - for index, child := range node.Children { - copy.Children[index] = graphRequiredSemiJoinNode(child, true) +func prepareGraphView(node semantic.SemanticNode, ancestorsRequired bool) (semantic.SemanticNode, semantic.SemanticNode, int, bool) { + matchMode := node.MatchMode + if strings.TrimSpace(string(matchMode)) == "" { + matchMode = spec.TraversalMatchRequired } - return copy -} - -func graphRequiredSemiJoinNode(node semantic.SemanticNode, ancestorsRequired bool) semantic.SemanticNode { - copy := node + childAncestorsRequired := ancestorsRequired && matchMode == spec.TraversalMatchRequired + normalized := node + normalized.MatchMode = matchMode + requiredPrefixView := node + requiredPrefixView.MatchMode = matchMode if !ancestorsRequired { - copy.MatchMode = spec.TraversalMatchOptional + requiredPrefixView.MatchMode = spec.TraversalMatchOptional } - childAncestorsRequired := ancestorsRequired && node.MatchMode.Required() - copy.Children = make([]semantic.SemanticNode, len(node.Children)) + normalized.Children = make([]semantic.SemanticNode, len(node.Children)) + requiredPrefixView.Children = make([]semantic.SemanticNode, len(node.Children)) + traversalCount := len(node.Children) + hasRequiredPrefix := childAncestorsRequired for index, child := range node.Children { - copy.Children[index] = graphRequiredSemiJoinNode(child, childAncestorsRequired) + childNormalized, childRequiredPrefixView, childCount, childHasRequiredPrefix := prepareGraphView(child, childAncestorsRequired) + normalized.Children[index] = childNormalized + requiredPrefixView.Children[index] = childRequiredPrefixView + traversalCount += childCount + hasRequiredPrefix = hasRequiredPrefix || childHasRequiredPrefix } - return copy + return normalized, requiredPrefixView, traversalCount, hasRequiredPrefix } func CompileResolvedGraphPlan(resolved semantic.ResolvedRecipePlan, limit int, policy ir.PhysicalOptimizationPolicy) (ir.PhysicalPlan, error) { @@ -191,35 +197,6 @@ func graphAlias(node semantic.SemanticNode, fallback string) string { return fallback } -func countGraphTraversals(node semantic.SemanticNode) int { - count := len(node.Children) - for _, child := range node.Children { - count += countGraphTraversals(child) - } - return count -} - -func graphHasRequiredTraversal(node semantic.SemanticNode) bool { - var walk func(semantic.SemanticNode, bool) bool - walk = func(parent semantic.SemanticNode, requiredPrefix bool) bool { - for _, child := range parent.Children { - mode := child.MatchMode - if strings.TrimSpace(string(mode)) == "" { - mode = spec.TraversalMatchRequired - } - childRequired := mode == spec.TraversalMatchRequired - if requiredPrefix && childRequired { - return true - } - if walk(child, requiredPrefix && childRequired) { - return true - } - } - return false - } - return walk(node, true) -} - func validateGraphNode(node semantic.SemanticNode, root bool) error { if !root && node.MatchMode != spec.TraversalMatchRequired && node.MatchMode != spec.TraversalMatchOptional && node.MatchMode != "" { return fmt.Errorf("graph traversal %q has invalid match mode %q", node.Alias, node.MatchMode) diff --git a/internal/dataframe/compiler/lower/graph_plan_test.go b/internal/dataframe/compiler/lower/graph_plan_test.go index 8890e9711..d27be0814 100644 --- a/internal/dataframe/compiler/lower/graph_plan_test.go +++ b/internal/dataframe/compiler/lower/graph_plan_test.go @@ -1,6 +1,9 @@ package lower import ( + "fmt" + "reflect" + "sort" "strings" "testing" @@ -58,3 +61,220 @@ func TestBuildGraphPhysicalPlanRootFallbackOnlyForOptionalBranches(t *testing.T) } } } + +func graphPlanTestRoot(children ...semantic.SemanticNode) semantic.SemanticNode { + return semantic.SemanticNode{Alias: "root", ResourceType: "Patient", Children: children} +} + +func graphPlanTestCondition(alias string, mode spec.TraversalMatchMode, children ...semantic.SemanticNode) semantic.SemanticNode { + return semantic.SemanticNode{Alias: alias, ResourceType: "Condition", EdgeLabel: "subject_Patient", MatchMode: mode, Children: children} +} + +func graphPlanTestObservation(alias string, mode spec.TraversalMatchMode) semantic.SemanticNode { + return semantic.SemanticNode{Alias: alias, ResourceType: "Observation", EdgeLabel: "subject_Patient", MatchMode: mode} +} + +func graphPlanTestFocusedObservation(alias string, mode spec.TraversalMatchMode) semantic.SemanticNode { + return semantic.SemanticNode{Alias: alias, ResourceType: "Observation", EdgeLabel: "focus_Condition", MatchMode: mode} +} + +func graphPlanTestObservationSiblings(count int, mode spec.TraversalMatchMode) []semantic.SemanticNode { + children := make([]semantic.SemanticNode, 0, count) + for index := 0; index < count; index++ { + children = append(children, graphPlanTestObservation(fmt.Sprintf("observation_%d", index), mode)) + } + return children +} + +func graphPlanTestBuild(t *testing.T, root semantic.SemanticNode) ir.PhysicalPlan { + t.Helper() + plan, err := BuildGraphPhysicalPlan(semantic.OutputPlan{Root: root}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err != nil { + t.Fatalf("BuildGraphPhysicalPlan() error = %v", err) + } + return plan +} + +func graphPlanTestPathSets(t *testing.T, plan ir.PhysicalPlan) []string { + t.Helper() + last := plan.Operations[len(plan.Operations)-1] + if last.GraphReturn == nil { + t.Fatalf("final operation = %#v, want graph return", last) + } + return last.GraphReturn.PathSets +} + +func graphPlanTestRequiredMatchKeys(plan ir.PhysicalPlan) []string { + keys := []string{} + for key := range plan.BindVars { + if strings.HasPrefix(key, "required_") && strings.HasSuffix(key, "_result") { + keys = append(keys, key) + } + } + sort.Strings(keys) + return keys +} + +func graphPlanTestRequiredFiltersPrecedePathSeed(t *testing.T, plan ir.PhysicalPlan) { + t.Helper() + seed := -1 + for index, operation := range plan.Operations { + if operation.Kind == ir.PhysicalPathSeedOp { + seed = index + break + } + } + if seed < 0 { + t.Fatal("plan has no path seed operation") + } + for index := seed; index < len(plan.Operations); index++ { + operation := plan.Operations[index] + if operation.Kind == ir.PhysicalFilterOp && operation.Filter != nil && operation.Filter.Expression != nil && operation.Filter.Expression.Kind == ir.PhysicalExistsPredicate { + t.Fatalf("required semi-join filter at %d must precede the path seed at %d", index, seed) + } + } +} + +func TestBuildGraphPhysicalPlanOmittedMatchModeDefaultsToRequired(t *testing.T) { + explicit := graphPlanTestBuild(t, graphPlanTestRoot(graphPlanTestCondition("condition", spec.TraversalMatchRequired))) + for _, omitted := range []spec.TraversalMatchMode{"", " "} { + plan := graphPlanTestBuild(t, graphPlanTestRoot(graphPlanTestCondition("condition", omitted))) + if !reflect.DeepEqual(plan, explicit) { + t.Fatalf("omitted match mode %q compiled differently from REQUIRED", omitted) + } + } + if got := graphPlanTestPathSets(t, explicit); !reflect.DeepEqual(got, []string{"path_1"}) { + t.Fatalf("required path sets = %v, want [path_1] without the root fallback", got) + } + for _, operation := range explicit.Operations { + if operation.PathExtend != nil && operation.PathExtend.MatchMode != string(spec.TraversalMatchRequired) { + t.Fatalf("path extend match mode = %q, want REQUIRED", operation.PathExtend.MatchMode) + } + } +} + +func TestBuildGraphPhysicalPlanRequiredPrefixPathSensitivity(t *testing.T) { + cases := []struct { + name string + root semantic.SemanticNode + wantPathSets []string + wantRequiredKeys []string + }{ + { + name: "required then required", + root: graphPlanTestRoot(graphPlanTestCondition("condition", spec.TraversalMatchRequired, graphPlanTestFocusedObservation("observation", spec.TraversalMatchRequired))), + wantPathSets: []string{"path_1", "path_2"}, + wantRequiredKeys: []string{"required_0_result", "required_1_result"}, + }, + { + name: "required under optional", + root: graphPlanTestRoot(graphPlanTestCondition("condition", spec.TraversalMatchOptional, graphPlanTestFocusedObservation("observation", spec.TraversalMatchRequired))), + wantPathSets: []string{"path_root", "path_1", "path_2"}, + wantRequiredKeys: []string{}, + }, + { + name: "required then optional", + root: graphPlanTestRoot(graphPlanTestCondition("condition", spec.TraversalMatchRequired, graphPlanTestFocusedObservation("observation", spec.TraversalMatchOptional))), + wantPathSets: []string{"path_1", "path_2"}, + wantRequiredKeys: []string{"required_0_result"}, + }, + { + name: "branching required prefix", + root: graphPlanTestRoot( + graphPlanTestCondition("condition", spec.TraversalMatchRequired, graphPlanTestFocusedObservation("observation", spec.TraversalMatchRequired)), + graphPlanTestObservation("screening", spec.TraversalMatchOptional), + ), + wantPathSets: []string{"path_1", "path_2", "path_3"}, + wantRequiredKeys: []string{"required_0_result", "required_1_result"}, + }, + } + for _, test := range cases { + t.Run(test.name, func(t *testing.T) { + plan := graphPlanTestBuild(t, test.root) + if got := graphPlanTestPathSets(t, plan); !reflect.DeepEqual(got, test.wantPathSets) { + t.Fatalf("path sets = %v, want %v", got, test.wantPathSets) + } + if got := graphPlanTestRequiredMatchKeys(plan); !reflect.DeepEqual(got, test.wantRequiredKeys) { + t.Fatalf("required semi-join bindings = %v, want %v", got, test.wantRequiredKeys) + } + graphPlanTestRequiredFiltersPrecedePathSeed(t, plan) + }) + } +} + +func TestBuildGraphPhysicalPlanTraversalDeclarationLimitBoundary(t *testing.T) { + atLimit := graphPlanTestBuild(t, graphPlanTestRoot(graphPlanTestObservationSiblings(32, spec.TraversalMatchOptional)...)) + wantAtLimit := []string{"path_root"} + for index := 1; index <= 32; index++ { + wantAtLimit = append(wantAtLimit, fmt.Sprintf("path_%d", index)) + } + if got := graphPlanTestPathSets(t, atLimit); !reflect.DeepEqual(got, wantAtLimit) { + t.Fatalf("path sets at the declaration limit = %v, want %v", got, wantAtLimit) + } + _, err := BuildGraphPhysicalPlan(semantic.OutputPlan{Root: graphPlanTestRoot(graphPlanTestObservationSiblings(33, spec.TraversalMatchOptional)...)}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err == nil || err.Error() != "graph traversal declaration count exceeds 32" { + t.Fatalf("over-limit error = %v, want the declaration count error", err) + } + shaping := graphPlanTestObservationSiblings(33, spec.TraversalMatchOptional) + shaping[0].Fields = []semantic.SemanticField{{Name: "status"}} + _, err = BuildGraphPhysicalPlan(semantic.OutputPlan{Root: graphPlanTestRoot(shaping...)}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err == nil || err.Error() != "graph traversal declaration count exceeds 32" { + t.Fatalf("count check must precede shaping validation, got %v", err) + } + invalid := graphPlanTestObservationSiblings(33, spec.TraversalMatchOptional) + invalid[0].MatchMode = "SMALL" + _, err = BuildGraphPhysicalPlan(semantic.OutputPlan{Root: graphPlanTestRoot(invalid...)}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err == nil || !strings.Contains(err.Error(), `unsupported traversal match mode "SMALL"`) { + t.Fatalf("semantic validation must precede the count check, got %v", err) + } +} + +func TestBuildGraphPhysicalPlanMatchModeAndShapingValidation(t *testing.T) { + _, err := BuildGraphPhysicalPlan(semantic.OutputPlan{Root: graphPlanTestRoot(graphPlanTestObservation("observation", "SMALL"))}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err == nil || err.Error() != `semantic traversal Patient -> Observation (subject_Patient) at Patient.children[0]: unsupported traversal match mode "SMALL"` { + t.Fatalf("invalid match mode error = %v", err) + } + shapingChild := graphPlanTestObservation("observation", spec.TraversalMatchOptional) + shapingChild.Fields = []semantic.SemanticField{{Name: "status"}} + _, err = BuildGraphPhysicalPlan(semantic.OutputPlan{Root: graphPlanTestRoot(shapingChild)}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err == nil || err.Error() != `graph traversal "observation" cannot declare dataframe shaping fields` { + t.Fatalf("shaping field error = %v", err) + } + rootMode := graphPlanTestRoot(graphPlanTestObservation("observation", spec.TraversalMatchOptional)) + rootMode.MatchMode = spec.TraversalMatchRequired + _, err = BuildGraphPhysicalPlan(semantic.OutputPlan{Root: rootMode}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err == nil || err.Error() != `semantic graph root must not declare traversal match mode "REQUIRED"` { + t.Fatalf("root match mode error = %v", err) + } + bothChild := graphPlanTestObservation("observation", "SMALL") + bothChild.Fields = []semantic.SemanticField{{Name: "status"}} + _, err = BuildGraphPhysicalPlan(semantic.OutputPlan{Root: graphPlanTestRoot(bothChild)}, semantic.ExecutionContext{Project: "p"}, 100, ir.DefaultPhysicalOptimizationPolicy()) + if err == nil || !strings.Contains(err.Error(), `unsupported traversal match mode "SMALL"`) { + t.Fatalf("semantic validation must precede shaping validation, got %v", err) + } +} + +func TestBuildGraphPhysicalPlanLeavesCallerGraphUnmodified(t *testing.T) { + input := graphPlanTestRoot( + graphPlanTestCondition("condition", "", graphPlanTestFocusedObservation("observation", spec.TraversalMatchRequired)), + graphPlanTestObservation("screening", spec.TraversalMatchOptional), + ) + template := graphPlanTestRoot( + graphPlanTestCondition("condition", "", graphPlanTestFocusedObservation("observation", spec.TraversalMatchRequired)), + graphPlanTestObservation("screening", spec.TraversalMatchOptional), + ) + first := graphPlanTestBuild(t, input) + if !reflect.DeepEqual(input, template) { + t.Fatal("BuildGraphPhysicalPlan mutated the caller-provided graph") + } + second := graphPlanTestBuild(t, input) + if !reflect.DeepEqual(input, template) { + t.Fatal("second BuildGraphPhysicalPlan mutated the caller-provided graph") + } + if !reflect.DeepEqual(first, second) { + t.Fatal("plan output is not deterministic for an identical caller graph") + } + if input.Children[0].MatchMode != "" { + t.Fatalf("omitted match mode was mutated to %q", input.Children[0].MatchMode) + } +}