diff --git a/go.mod b/go.mod index d1d5352a..196f007c 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,7 @@ require ( github.com/apstndb/spanemuboost v0.4.7 github.com/apstndb/spaniter v0.3.1 github.com/apstndb/spanner-docs-embed v0.0.0-20260312161525-0136df2da2a6 - github.com/apstndb/spannerplan v0.3.0 + github.com/apstndb/spannerplan v0.3.1 github.com/apstndb/spanstats v0.1.0 github.com/apstndb/spantype v0.3.13 github.com/apstndb/spanvalue v0.9.0 diff --git a/go.sum b/go.sum index c94e97b2..1b711d05 100644 --- a/go.sum +++ b/go.sum @@ -96,8 +96,8 @@ github.com/apstndb/spaniter v0.3.1 h1:hhi4+JCF80x696bg7zIco7Vo8kmPbkbTdvSOEhItp5 github.com/apstndb/spaniter v0.3.1/go.mod h1:aBSHcHIqgAZXCxFdi734R/wAQUIuCQ6WZ+CjOCxARIM= github.com/apstndb/spanner-docs-embed v0.0.0-20260312161525-0136df2da2a6 h1:JP8l0QJZRVSRwBXTxaMMAjrmF0kQDjXlCM0bwTCo5Gs= github.com/apstndb/spanner-docs-embed v0.0.0-20260312161525-0136df2da2a6/go.mod h1:Enpmw/D11tME865ROYDQzoHSIo9V8uCgWdu+fxA7n2M= -github.com/apstndb/spannerplan v0.3.0 h1:EKB9Iw5iCloWoJXIFUxh93zXIO6NqoAld9fnzzkL/UA= -github.com/apstndb/spannerplan v0.3.0/go.mod h1:zVUS187Z4DGaJekIBt2E1JYb9UMKQ7s5ZZo3HOr522E= +github.com/apstndb/spannerplan v0.3.1 h1:CloO1irr01HDurWxLy1gYAliIGbNdeVl/bpD0Q+zK8U= +github.com/apstndb/spannerplan v0.3.1/go.mod h1:zVUS187Z4DGaJekIBt2E1JYb9UMKQ7s5ZZo3HOr522E= github.com/apstndb/spanstats v0.1.0 h1:wAPqtFn1AfQaIudB4HwTLiIPinwWJw/gQhgE7NQWVKo= github.com/apstndb/spanstats v0.1.0/go.mod h1:DFGYC9e7WE0BFQL0st6EQwrl1Pn2FuvDWSefwI9FTWc= github.com/apstndb/spantype v0.3.13 h1:FTP3zUpVXMfPlZ3P+1RK6SYHND+96YVht0I+ZHGIzMc= diff --git a/internal/mycli/explain_print_sections.go b/internal/mycli/explain_print_sections.go index 2cbe1f69..5b2623d2 100644 --- a/internal/mycli/explain_print_sections.go +++ b/internal/mycli/explain_print_sections.go @@ -17,7 +17,6 @@ package mycli import ( "fmt" "slices" - "strings" sppb "cloud.google.com/go/spanner/apiv1/spannerpb" "github.com/apstndb/spannerplan/plantree" @@ -68,48 +67,25 @@ func resolveExplainPrintSections(sysVars *systemVariables, override *planref.Pri return append(planref.PrintSections{}, sysVars.Display.ParsedExplainPrintSections...) } -func buildPlanAppendices(rows []plantree.RowWithPredicates, sections planref.PrintSections) ([]string, []ResultAppendix) { +func buildPlanAppendices(rows []plantree.RowWithPredicates, sections planref.PrintSections) ([]string, []ResultAppendix, error) { + // Always pass WithPrintSections so an empty CLI/override list stays empty. + // Omitting the option would select the library default (predicates only). + // Do not enable scalar-variable resolution; keep the historical raw descriptions. + built, err := planref.BuildAppendices(rows, planref.WithPrintSections(sections...)) + if err != nil { + return nil, nil, err + } + var predicates []string var appendices []ResultAppendix - for _, section := range sections { - var appendix ResultAppendix - switch section { - case planref.PrintPredicates: - predicates = appendixLines(rows, func(row plantree.RowWithPredicates) []string { - return row.Predicates - }) - appendix = ResultAppendix{Title: "Predicates(identified by ID):", Lines: predicates} - case planref.PrintOrdering: - appendix = ResultAppendix{ - Title: "Ordering(identified by ID):", - Lines: appendixLines(rows, func(row plantree.RowWithPredicates) []string { - return scalarLinkLines(row, isOrderingScalarLink, func(link plantree.ScalarChildLink) string { - return normalizeKeyOrderSuffix(link.Description) - }) - }), - } - case planref.PrintAggregate: - appendix = ResultAppendix{ - Title: "Aggregates(identified by ID):", - Lines: appendixLines(rows, func(row plantree.RowWithPredicates) []string { - return scalarLinkLines(row, isAggregateScalarLink, scalarLinkDescription) - }), - } - case planref.PrintTyped, planref.PrintFull: - appendix = ResultAppendix{ - Title: "Node Parameters(identified by ID):", - Lines: appendixLines(rows, func(row plantree.RowWithPredicates) []string { - return scalarLinkLines(row, func(_ plantree.RowWithPredicates, link plantree.ScalarChildLink) bool { - return section == planref.PrintFull || link.Type != "" - }, formatRawScalarLink) - }), - } - } - if len(appendix.Lines) > 0 { - appendices = append(appendices, appendix) + for _, appendix := range built { + mapped := ResultAppendix{Title: appendix.Title, Lines: appendix.Lines} + appendices = append(appendices, mapped) + if appendix.Section == planref.PrintPredicates { + predicates = appendix.Lines } } - return predicates, appendices + return predicates, appendices, nil } // buildQueryPlanAppendix renders a query plan as one or more titled result @@ -143,7 +119,10 @@ func buildQueryPlanAppendix(sysVars *systemVariables, plan *sppb.QueryPlan) ([]R } planAppendix := ResultAppendix{Title: "Query Plan(identified by ID):", Lines: lines} - _, sectionAppendices := buildPlanAppendices(rows, sections) + _, sectionAppendices, err := buildPlanAppendices(rows, sections) + if err != nil { + return nil, err + } return append([]ResultAppendix{planAppendix}, sectionAppendices...), nil } @@ -154,105 +133,3 @@ func formatPlanRowID(row plantree.RowWithPredicates, sections planref.PrintSecti } return fmt.Sprint(row.ID) } - -func appendixLines(rows []plantree.RowWithPredicates, items func(plantree.RowWithPredicates) []string) []string { - var maxIDLength int - for _, row := range rows { - if length := len(fmt.Sprint(row.ID)); length > maxIDLength { - maxIDLength = length - } - } - - var lines []string - for _, row := range rows { - for i, item := range items(row) { - var prefix string - if i == 0 { - prefix = fmt.Sprintf("%*d:", maxIDLength, row.ID) - } else { - prefix = strings.Repeat(" ", maxIDLength+1) - } - lines = append(lines, fmt.Sprintf("%s %s", prefix, item)) - } - } - return lines -} - -type scalarLinkGroup struct { - typ string - values []string -} - -func scalarLinkLines( - row plantree.RowWithPredicates, - include func(plantree.RowWithPredicates, plantree.ScalarChildLink) bool, - format func(plantree.ScalarChildLink) string, -) []string { - groupByType := map[string]int{} - var groups []scalarLinkGroup - - for _, link := range row.ScalarChildLinks { - if !include(row, link) { - continue - } - - groupIndex, ok := groupByType[link.Type] - if !ok { - groupIndex = len(groups) - groupByType[link.Type] = groupIndex - groups = append(groups, scalarLinkGroup{typ: link.Type}) - } - groups[groupIndex].values = append(groups[groupIndex].values, format(link)) - } - - lines := make([]string, 0, len(groups)) - for _, group := range groups { - joined := strings.Join(group.values, ", ") - if joined == "" { - continue - } - - typePart := "" - if group.typ != "" { - typePart = group.typ + ": " - } - lines = append(lines, typePart+joined) - } - return lines -} - -func formatRawScalarLink(link plantree.ScalarChildLink) string { - if link.Variable != "" { - return fmt.Sprintf("$%s=%s", link.Variable, link.Description) - } - return link.Description -} - -func scalarLinkDescription(link plantree.ScalarChildLink) string { - return link.Description -} - -func normalizeKeyOrderSuffix(s string) string { - s = strings.TrimSpace(s) - for _, suffix := range []string{"(ASC)", "(DESC)"} { - if before, ok := strings.CutSuffix(s, " "+suffix); ok { - return before + " " + strings.Trim(suffix, "()") - } - } - return s -} - -func isOrderingScalarLink(row plantree.RowWithPredicates, link plantree.ScalarChildLink) bool { - switch row.DisplayName { - case "Sort", "Sort Limit": - return link.Type == "Key" - case "Minor Sort", "Minor Sort Limit": - return link.Type == "MajorKey" || link.Type == "MinorKey" - default: - return false - } -} - -func isAggregateScalarLink(row plantree.RowWithPredicates, link plantree.ScalarChildLink) bool { - return row.DisplayName == "Aggregate" && (link.Type == "Key" || link.Type == "Agg") -} diff --git a/internal/mycli/explain_print_sections_test.go b/internal/mycli/explain_print_sections_test.go index 5f82c314..0f865fa0 100644 --- a/internal/mycli/explain_print_sections_test.go +++ b/internal/mycli/explain_print_sections_test.go @@ -15,8 +15,10 @@ package mycli import ( + "strings" "testing" + "github.com/apstndb/spanner-mycli/enums" "github.com/apstndb/spannerplan/plantree" planref "github.com/apstndb/spannerplan/plantree/reference" "github.com/google/go-cmp/cmp" @@ -34,80 +36,74 @@ func TestBuildPlanAppendices(t *testing.T) { DisplayName: "Sort", ScalarChildLinks: []plantree.ScalarChildLink{ {Type: "Key", Description: "$LastName (ASC)"}, - {Type: "Key", Description: "$FirstName (DESC)"}, - }, - }, - { - ID: 2, - DisplayName: "Aggregate", - ScalarChildLinks: []plantree.ScalarChildLink{ - {Type: "Key", Description: "$SingerId"}, - {Type: "Agg", Variable: "count", Description: "COUNT(*)"}, }, }, } - predicates, appendices := buildPlanAppendices(rows, planref.PrintSections{ + predicates, appendices, err := buildPlanAppendices(rows, planref.PrintSections{ planref.PrintPredicates, planref.PrintOrdering, - planref.PrintAggregate, }) - - wantPredicates := []string{"0: Condition: ($SingerId = 1)"} - if diff := cmp.Diff(wantPredicates, predicates); diff != "" { - t.Errorf("predicates mismatch (-want +got):\n%s", diff) + if err != nil { + t.Fatalf("buildPlanAppendices() error = %v", err) + } + if len(appendices) != 2 { + t.Fatalf("len(appendices) = %d, want 2: %+v", len(appendices), appendices) + } + if appendices[0].Title != "Predicates(identified by ID):" { + t.Errorf("appendices[0].Title = %q", appendices[0].Title) + } + if appendices[1].Title != "Ordering(identified by ID):" { + t.Errorf("appendices[1].Title = %q", appendices[1].Title) + } + wantPredicateLine := "0: Condition: ($SingerId = 1)" + wantOrderingLine := "1: Key: $LastName ASC" + if diff := cmp.Diff([]string{wantPredicateLine}, appendices[0].Lines); diff != "" { + t.Errorf("predicate appendix lines mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff([]string{wantOrderingLine}, appendices[1].Lines); diff != "" { + t.Errorf("ordering appendix lines mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff([]string{wantPredicateLine}, predicates); diff != "" { + t.Errorf("legacy predicates slice mismatch (-want +got):\n%s", diff) } - wantAppendices := []ResultAppendix{ - { - Title: "Predicates(identified by ID):", - Lines: []string{"0: Condition: ($SingerId = 1)"}, - }, - { - Title: "Ordering(identified by ID):", - Lines: []string{"1: Key: $LastName ASC, $FirstName DESC"}, - }, - { - Title: "Aggregates(identified by ID):", - Lines: []string{ - "2: Key: $SingerId", - " Agg: COUNT(*)", - }, - }, + predicates, appendices, err = buildPlanAppendices(rows, planref.PrintSections{}) + if err != nil { + t.Fatalf("empty sections error = %v", err) } - if diff := cmp.Diff(wantAppendices, appendices); diff != "" { - t.Errorf("appendices mismatch (-want +got):\n%s", diff) + if len(predicates) != 0 || len(appendices) != 0 { + t.Fatalf("empty sections returned predicates=%q appendices=%+v", predicates, appendices) } } -func TestBuildPlanAppendicesTypedAndFull(t *testing.T) { +const invalidPrintSectionsCause = `print section "full" cannot be combined with other sections` + +func TestBuildPlanAppendicesPropagatesInvalidSections(t *testing.T) { t.Parallel() - rows := []plantree.RowWithPredicates{ - { - ID: 0, - ScalarChildLinks: []plantree.ScalarChildLink{ - {Type: "Condition", Description: "($SingerId = 1)"}, - {Variable: "SingerId", Description: "SingerId"}, - }, - }, + _, _, err := buildPlanAppendices(nil, planref.PrintSections{planref.PrintFull, planref.PrintPredicates}) + if err == nil || !strings.Contains(err.Error(), invalidPrintSectionsCause) { + t.Fatalf("buildPlanAppendices() error = %v, want %q", err, invalidPrintSectionsCause) } +} - _, typed := buildPlanAppendices(rows, planref.PrintSections{planref.PrintTyped}) - wantTyped := []ResultAppendix{{ - Title: "Node Parameters(identified by ID):", - Lines: []string{"0: Condition: ($SingerId = 1)"}, - }} - if diff := cmp.Diff(wantTyped, typed); diff != "" { - t.Errorf("typed appendices mismatch (-want +got):\n%s", diff) +func TestBuildQueryPlanAppendixPropagatesAppendixError(t *testing.T) { + t.Parallel() + sysVars := newSystemVariablesWithDefaultsForTest() + sysVars.Display.ParsedExplainPrintSections = planref.PrintSections{planref.PrintFull, planref.PrintPredicates} + _, err := buildQueryPlanAppendix(sysVars, testQueryPlan(t)) + if err == nil || !strings.Contains(err.Error(), invalidPrintSectionsCause) { + t.Fatalf("buildQueryPlanAppendix() error = %v, want %q", err, invalidPrintSectionsCause) } +} - _, full := buildPlanAppendices(rows, planref.PrintSections{planref.PrintFull}) - wantFull := []ResultAppendix{{ - Title: "Node Parameters(identified by ID):", - Lines: []string{"0: Condition: ($SingerId = 1)", " $SingerId=SingerId"}, - }} - if diff := cmp.Diff(wantFull, full); diff != "" { - t.Errorf("full appendices mismatch (-want +got):\n%s", diff) +func TestBuildExplainAnalyzeResultPropagatesAppendixError(t *testing.T) { + t.Parallel() + sysVars := newSystemVariablesWithDefaultsForTest() + invalid := planref.PrintSections{planref.PrintFull, planref.PrintPredicates} + _, err := buildExplainAnalyzeResult(sysVars, testQueryPlan(t), QueryStats{}, enums.ExplainFormatUnspecified, 0, &invalid) + if err == nil || !strings.Contains(err.Error(), "failed to process query plan") || !strings.Contains(err.Error(), invalidPrintSectionsCause) { + t.Fatalf("buildExplainAnalyzeResult() error = %v, want wrapped %q", err, invalidPrintSectionsCause) } } diff --git a/internal/mycli/statements_explain_describe.go b/internal/mycli/statements_explain_describe.go index efd34aee..a6fb1f0c 100644 --- a/internal/mycli/statements_explain_describe.go +++ b/internal/mycli/statements_explain_describe.go @@ -683,7 +683,10 @@ func processPlan(plan *sppb.QueryPlan, columnRenderDefs []columnRenderDef, inlin } rows = append(rows, toRow(rowStrs...)) } - predicates, appendices = buildPlanAppendices(rowsWithPredicates, printSections) + predicates, appendices, err = buildPlanAppendices(rowsWithPredicates, printSections) + if err != nil { + return nil, nil, nil, err + } return rows, predicates, appendices, nil } diff --git a/internal/mycli/statements_explain_describe_test.go b/internal/mycli/statements_explain_describe_test.go index 46914a2f..15e881a1 100644 --- a/internal/mycli/statements_explain_describe_test.go +++ b/internal/mycli/statements_explain_describe_test.go @@ -562,18 +562,20 @@ func TestProcessPlanAppendicesUsingRealPlan(t *testing.T) { t.Parallel() plan := loadTestPlan(t, "testdata/plans/scalar_subqueries.input.json") - _, _, appendices, err := processPlanWithoutStats(plan, enums.ExplainFormatTraditional, 0, false, planref.PrintSections{ - planref.PrintAggregate, - planref.PrintTyped, - }) + _, _, aggregates, err := processPlanWithoutStats(plan, enums.ExplainFormatTraditional, 0, false, planref.PrintSections{planref.PrintAggregate}) if err != nil { - t.Fatalf("processPlanWithoutStats() error = %v", err) + t.Fatalf("processPlanWithoutStats(aggregate) error = %v", err) + } + if !appendixContains(aggregates, "Aggregates(identified by ID):", "Agg: COUNT()") { + t.Fatalf("aggregate appendix does not contain COUNT() line: %#v", aggregates) } - if !appendixContains(appendices, "Aggregates(identified by ID):", "Agg: COUNT()") { - t.Fatalf("aggregate appendix does not contain COUNT() line: %#v", appendices) + + _, _, typed, err := processPlanWithoutStats(plan, enums.ExplainFormatTraditional, 0, false, planref.PrintSections{planref.PrintTyped}) + if err != nil { + t.Fatalf("processPlanWithoutStats(typed) error = %v", err) } - if !appendixContains(appendices, "Node Parameters(identified by ID):", "Condition:") { - t.Fatalf("typed appendix does not contain condition line: %#v", appendices) + if !appendixContains(typed, "Node Parameters(identified by ID):", "Condition:") { + t.Fatalf("typed appendix does not contain condition line: %#v", typed) } }