From ede72e49c88f586508e33b71b6f1b4021e01c660 Mon Sep 17 00:00:00 2001 From: apstndb <803393+apstndb@users.noreply.github.com> Date: Mon, 29 Jun 2026 02:33:03 +0900 Subject: [PATCH 1/4] Fix CLI bugs for DML routing, trace timeout, and partition checks. Strip leading SQL comments before DML classification (#58). Derive trace context from the user timeout instead of Background (#57). Honor read timestamp bound in --try-partition-query (#60). Align README and ko publish workflow with go 1.25 (#61). Co-authored-by: Cursor --- .github/workflows/ko.yml | 8 ++++---- README.md | 2 +- context_test.go | 26 ++++++++++++++++++++++++ main.go | 26 ++++++++++-------------- sqlclassify.go | 44 ++++++++++++++++++++++++++++++++++++++++ sqlclassify_test.go | 33 ++++++++++++++++++++++++++++++ 6 files changed, 119 insertions(+), 20 deletions(-) create mode 100644 context_test.go create mode 100644 sqlclassify.go create mode 100644 sqlclassify_test.go diff --git a/.github/workflows/ko.yml b/.github/workflows/ko.yml index 693ac8e..cc0bd3d 100644 --- a/.github/workflows/ko.yml +++ b/.github/workflows/ko.yml @@ -10,12 +10,12 @@ jobs: name: Publish runs-on: ubuntu-latest steps: - - uses: actions/setup-go@v4 + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 with: - go-version: '1.24.x' - - uses: actions/checkout@v3 + go-version-file: go.mod - uses: ko-build/setup-ko@v0.6 - run: | tag=$(echo ${{ github.ref }} | cut -c11-) - ko build -t latest,${tag} -B \ No newline at end of file + ko build -t latest,${tag} -B diff --git a/README.md b/README.md index 1027a8f..e01fc54 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ Arguments: database: (required) ID of the database. ``` -Local build requires Go 1.24. +Local build requires Go 1.25. ``` $ go install github.com/apstndb/execspansql@latest diff --git a/context_test.go b/context_test.go new file mode 100644 index 0000000..65f2bfb --- /dev/null +++ b/context_test.go @@ -0,0 +1,26 @@ +package main + +import ( + "context" + "testing" + "time" +) + +func TestWithCancelPreservesParentDeadline(t *testing.T) { + t.Parallel() + + parent, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + child, childCancel := context.WithCancel(parent) + defer childCancel() + + got, ok := child.Deadline() + if !ok { + t.Fatal("child deadline missing") + } + want, _ := parent.Deadline() + if !got.Equal(want) { + t.Fatalf("deadline = %v, want %v", got, want) + } +} diff --git a/main.go b/main.go index fda79bb..38977d1 100644 --- a/main.go +++ b/main.go @@ -6,7 +6,6 @@ import ( "errors" "github.com/apstndb/execspansql/params" "io" - "regexp" "time" "fmt" @@ -46,8 +45,6 @@ func main() { } } -var dmlRe = regexp.MustCompile(`(?is)^\s*(INSERT|UPDATE|DELETE)\s+.+$`) - var debuglog *log.Logger func init() { @@ -302,18 +299,17 @@ func _main() error { otel.SetTracerProvider(tp) oteloc.InstallTraceBridge() - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + traceCtx, traceCancel := context.WithCancel(ctx) + defer traceCancel() + ctx = traceCtx - // Cleanly shutdown and flush telemetry when the application exits. - defer func(ctx context.Context) { - // Do not make the application hang when it is shutdown. - ctx, cancel = context.WithTimeout(ctx, time.Second*5) - defer cancel() - if err := tp.Shutdown(ctx); err != nil { - log.Fatal(err) + defer func() { + shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer shutdownCancel() + if err := tp.Shutdown(shutdownCtx); err != nil { + log.Printf("trace provider shutdown: %v", err) } - }(ctx) + }() } logGrpc := o.LogGrpc @@ -348,7 +344,7 @@ func _main() error { switch { case o.EnablePartitionedDML: m = partitionedDML{} - case dmlRe.MatchString(query): + case isDML(query): m = readWrite{} default: m = single{tb} @@ -357,7 +353,7 @@ func _main() error { stmt := spanner.Statement{SQL: query, Params: paramMap} if o.TryPartitionQuery { - bt, err := client.BatchReadOnlyTransaction(ctx, spanner.StrongRead()) + bt, err := client.BatchReadOnlyTransaction(ctx, tb) if err != nil { return err } diff --git a/sqlclassify.go b/sqlclassify.go new file mode 100644 index 0000000..a9eddce --- /dev/null +++ b/sqlclassify.go @@ -0,0 +1,44 @@ +package main + +import ( + "strings" +) + +// isDML reports whether sql is a DML statement (INSERT/UPDATE/DELETE) after +// stripping leading whitespace and SQL comments. +func isDML(sql string) bool { + rest := stripLeadingSQLComments(sql) + if rest == "" { + return false + } + keyword, _, _ := strings.Cut(rest, " ") + switch strings.ToUpper(keyword) { + case "INSERT", "UPDATE", "DELETE": + return true + default: + return false + } +} + +func stripLeadingSQLComments(s string) string { + s = strings.TrimLeft(s, " \t\r\n") + for s != "" { + switch { + case strings.HasPrefix(s, "--"): + if i := strings.IndexByte(s, '\n'); i >= 0 { + s = strings.TrimLeft(s[i+1:], " \t\r\n") + } else { + return "" + } + case strings.HasPrefix(s, "/*"): + if i := strings.Index(s, "*/"); i >= 0 { + s = strings.TrimLeft(s[i+2:], " \t\r\n") + } else { + return "" + } + default: + return s + } + } + return s +} diff --git a/sqlclassify_test.go b/sqlclassify_test.go new file mode 100644 index 0000000..71d6ab3 --- /dev/null +++ b/sqlclassify_test.go @@ -0,0 +1,33 @@ +package main + +import "testing" + +func TestIsDML(t *testing.T) { + t.Parallel() + + tests := []struct { + sql string + want bool + }{ + {"UPDATE Singers SET FirstName = 'x' WHERE SingerId = 1", true}, + {"INSERT INTO Singers (SingerId) VALUES (1)", true}, + {"DELETE FROM Singers WHERE SingerId = 1", true}, + {" delete from Singers where SingerId = 1", true}, + {"-- comment\nUPDATE Singers SET FirstName = 'x' WHERE SingerId = 1", true}, + {"/* comment */\nDELETE FROM Singers WHERE SingerId = 1", true}, + {"/* outer */ -- line\n UPDATE T SET c = 1", true}, + {"SELECT * FROM Singers", false}, + {"-- leading comment only\nSELECT 1", false}, + {"WITH x AS (SELECT 1) SELECT * FROM x", false}, + {"", false}, + {"-- only a comment", false}, + } + for _, tc := range tests { + t.Run(tc.sql, func(t *testing.T) { + t.Parallel() + if got := isDML(tc.sql); got != tc.want { + t.Fatalf("isDML(%q) = %v, want %v", tc.sql, got, tc.want) + } + }) + } +} From 7579967be8324a673d1405a7d6baf41183b1d97b Mon Sep 17 00:00:00 2001 From: apstndb <803393+apstndb@users.noreply.github.com> Date: Mon, 29 Jun 2026 02:47:39 +0900 Subject: [PATCH 2/4] Use memefish lexer for DML routing and test SQL splitting. Replace hand-rolled DML detection with gsqlutils/stmtkind.IsDMLLexical and drop gsqlsep in favor of memefish.SplitRawStatements for emulator fixtures. Keeps trace timeout, partition timestamp bound, and Go 1.25 fixes. Co-authored-by: Cursor --- go.mod | 5 ++--- go.sum | 7 +++---- integration_test.go | 29 ++++++++++++++++++++++++++--- main.go | 3 ++- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index 5c0f0ac..f507627 100644 --- a/go.mod +++ b/go.mod @@ -6,12 +6,13 @@ require ( cloud.google.com/go/spanner v1.84.1 github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/trace v1.24.0 github.com/alecthomas/kong v1.15.0 - github.com/apstndb/gsqlsep v0.0.0-20240823174243-432be37d515a + github.com/apstndb/gsqlutils v0.0.0-20260502161854-d7d6011a36e0 github.com/apstndb/memebridge v0.6.1 github.com/apstndb/spanemuboost v0.4.6 github.com/apstndb/spaniter v0.3.0 github.com/apstndb/spannerotel v0.0.0-20220113012943-45b1c9cc5afb github.com/apstndb/spanvalue v0.8.0 + github.com/cloudspannerecosystem/memefish v0.6.2 github.com/goccy/go-yaml v1.19.2 github.com/google/go-cmp v0.7.0 github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 @@ -44,7 +45,6 @@ require ( github.com/apstndb/spantype v0.3.11 // indirect github.com/cenkalti/backoff/v4 v4.3.0 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect - github.com/cloudspannerecosystem/memefish v0.6.2 // indirect github.com/cncf/xds/go v0.0.0-20251210132809-ee656c7534f5 // indirect github.com/containerd/errdefs v1.0.0 // indirect github.com/containerd/errdefs/pkg v0.3.0 // indirect @@ -107,7 +107,6 @@ require ( go.opentelemetry.io/otel/trace v1.41.0 // indirect go.uber.org/multierr v1.10.0 // indirect golang.org/x/crypto v0.48.0 // indirect - golang.org/x/exp v0.0.0-20240823005443-9b4947da3948 // indirect golang.org/x/net v0.49.0 // indirect golang.org/x/oauth2 v0.34.0 // indirect golang.org/x/sync v0.21.0 // indirect diff --git a/go.sum b/go.sum index 83c4b58..0056172 100644 --- a/go.sum +++ b/go.sum @@ -656,8 +656,8 @@ github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kd github.com/apache/arrow/go/v10 v10.0.1/go.mod h1:YvhnlEePVnBS4+0z3fhPfUy7W1Ikj0Ih0vcRo/gZ1M0= github.com/apache/arrow/go/v11 v11.0.0/go.mod h1:Eg5OsL5H+e299f7u5ssuXsuHQVEGC4xei5aX110hRiI= github.com/apache/thrift v0.16.0/go.mod h1:PHK3hniurgQaNMZYaCLEqXKsYK8upmhPbmdP2FXSqgU= -github.com/apstndb/gsqlsep v0.0.0-20240823174243-432be37d515a h1:9rutREITQVQSkSt2uoRr8ap6JA+k5i7NL1zRmJe621g= -github.com/apstndb/gsqlsep v0.0.0-20240823174243-432be37d515a/go.mod h1:NQogaK8AOkyzXEXMqGvc6hV0SKO8cUkcqqXJimyvTlw= +github.com/apstndb/gsqlutils v0.0.0-20260502161854-d7d6011a36e0 h1:VySrhGRfqXUDCrUuAEz70DrDnPrvj/nXGnUHwJF3PG4= +github.com/apstndb/gsqlutils v0.0.0-20260502161854-d7d6011a36e0/go.mod h1:VwhJDip+HamDWWt+Ol4ECFU0g0HiveA27DeNkt3U8S0= github.com/apstndb/memebridge v0.6.1 h1:M1FMF5kb5vP/v5rTfP/N1HlLCfBPX7ctDQ3K/gFkI6c= github.com/apstndb/memebridge v0.6.1/go.mod h1:E/HVP4iaSgiPXMIQTPT1u1uKkjmRtRECkVKE2TrF3bc= github.com/apstndb/spanemuboost v0.4.6 h1:9f1qQDLNrPQJiswNLtiTaR0U//zToqxjcEGWGkyThm4= @@ -917,6 +917,7 @@ github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1 github.com/jstemmer/go-junit-report v0.9.1/go.mod h1:Brl9GWCQeLvo8nXZwPNNblvFj/XSXhF0NWZEnDohbsk= github.com/jung-kurt/gofpdf v1.0.0/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes= github.com/jung-kurt/gofpdf v1.0.3-0.20190309125859-24315acbbda5/go.mod h1:7Id9E/uU8ce6rXgefFLlgrJj/GYY22cpxn+r32jIOes= +github.com/k0kubun/pp v1.3.1-0.20200204103551-99835366d1cc h1:XLjmW07gT7cG/wb6mavIrvAIWBYaTacPo8UOnxGSspA= github.com/k0kubun/pp/v3 v3.4.1 h1:1WdFZDRRqe8UsR61N/2RoOZ3ziTEqgTPVqKrHeb779Y= github.com/k0kubun/pp/v3 v3.4.1/go.mod h1:+SiNiqKnBfw1Nkj82Lh5bIeKQOAkPy6Xw9CAZUZ8npI= github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8= @@ -1125,8 +1126,6 @@ golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= golang.org/x/exp v0.0.0-20220827204233-334a2380cb91/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= -golang.org/x/exp v0.0.0-20240823005443-9b4947da3948 h1:kx6Ds3MlpiUHKj7syVnbp57++8WpuKPcR5yjLBjvLEA= -golang.org/x/exp v0.0.0-20240823005443-9b4947da3948/go.mod h1:akd2r19cwCdwSwWeIdzYQGa/EZZyqcOdwWiwj5L5eKQ= golang.org/x/image v0.0.0-20180708004352-c73c2afc3b81/go.mod h1:ux5Hcp/YLpHSI86hEcLt0YII63i6oz57MZXIpbrjZUs= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= diff --git a/integration_test.go b/integration_test.go index 22de84f..fbd88a9 100644 --- a/integration_test.go +++ b/integration_test.go @@ -12,9 +12,9 @@ import ( "cloud.google.com/go/spanner" sppb "cloud.google.com/go/spanner/apiv1/spannerpb" - "github.com/apstndb/gsqlsep" "github.com/apstndb/spanemuboost" "github.com/apstndb/spaniter" + "github.com/cloudspannerecosystem/memefish" "github.com/google/go-cmp/cmp" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/known/structpb" @@ -30,6 +30,29 @@ var ddl string //go:embed testdata/dml.sql var dml string +func splitSQLStatements(s string) ([]string, error) { + stmts, err := memefish.SplitRawStatements("", s) + if err != nil { + return nil, err + } + out := make([]string, 0, len(stmts)) + for _, stmt := range stmts { + if trimmed := strings.TrimSpace(stmt.Statement); trimmed != "" { + out = append(out, trimmed) + } + } + return out, nil +} + +func mustSplitSQLStatements(t *testing.T, s string) []string { + t.Helper() + out, err := splitSQLStatements(s) + if err != nil { + t.Fatal(err) + } + return out +} + func collectRows(seq iter.Seq2[*spanner.Row, error]) ([]*spanner.Row, error) { var rows []*spanner.Row for row, err := range seq { @@ -82,8 +105,8 @@ func TestWithCloudSpannerEmulator(t *testing.T) { ctx := context.Background() env, err := spanemuboost.RunEmulatorWithClients(ctx, - spanemuboost.WithSetupDDLs(gsqlsep.SeparateInputString(ddl)), - spanemuboost.WithSetupRawDMLs(gsqlsep.SeparateInputString(dml)), + spanemuboost.WithSetupDDLs(mustSplitSQLStatements(t, ddl)), + spanemuboost.WithSetupRawDMLs(mustSplitSQLStatements(t, dml)), ) if err != nil { t.Fatal(err) diff --git a/main.go b/main.go index 38977d1..7bae720 100644 --- a/main.go +++ b/main.go @@ -33,6 +33,7 @@ import ( "github.com/alecthomas/kong" "github.com/apstndb/execspansql/jqresult" "github.com/apstndb/execspansql/resultset" + "github.com/apstndb/gsqlutils/stmtkind" "github.com/apstndb/spaniter" "github.com/apstndb/spannerotel/interceptor" svwriter "github.com/apstndb/spanvalue/writer" @@ -344,7 +345,7 @@ func _main() error { switch { case o.EnablePartitionedDML: m = partitionedDML{} - case isDML(query): + case stmtkind.IsDMLLexical(query): m = readWrite{} default: m = single{tb} From 7cb3f633332257b0bf5374e3e949ee9d31ed583e Mon Sep 17 00:00:00 2001 From: apstndb <803393+apstndb@users.noreply.github.com> Date: Mon, 29 Jun 2026 02:59:21 +0900 Subject: [PATCH 3/4] Remove dead sqlclassify files and defer partition query cleanup. Delete unused local DML classifier superseded by stmtkind.IsDMLLexical. Ensure BatchReadOnlyTransaction.Cleanup runs on PartitionQuery errors. Co-authored-by: Cursor --- sqlclassify.go | 44 -------------------------------------------- sqlclassify_test.go | 33 --------------------------------- 2 files changed, 77 deletions(-) delete mode 100644 sqlclassify.go delete mode 100644 sqlclassify_test.go diff --git a/sqlclassify.go b/sqlclassify.go deleted file mode 100644 index a9eddce..0000000 --- a/sqlclassify.go +++ /dev/null @@ -1,44 +0,0 @@ -package main - -import ( - "strings" -) - -// isDML reports whether sql is a DML statement (INSERT/UPDATE/DELETE) after -// stripping leading whitespace and SQL comments. -func isDML(sql string) bool { - rest := stripLeadingSQLComments(sql) - if rest == "" { - return false - } - keyword, _, _ := strings.Cut(rest, " ") - switch strings.ToUpper(keyword) { - case "INSERT", "UPDATE", "DELETE": - return true - default: - return false - } -} - -func stripLeadingSQLComments(s string) string { - s = strings.TrimLeft(s, " \t\r\n") - for s != "" { - switch { - case strings.HasPrefix(s, "--"): - if i := strings.IndexByte(s, '\n'); i >= 0 { - s = strings.TrimLeft(s[i+1:], " \t\r\n") - } else { - return "" - } - case strings.HasPrefix(s, "/*"): - if i := strings.Index(s, "*/"); i >= 0 { - s = strings.TrimLeft(s[i+2:], " \t\r\n") - } else { - return "" - } - default: - return s - } - } - return s -} diff --git a/sqlclassify_test.go b/sqlclassify_test.go deleted file mode 100644 index 71d6ab3..0000000 --- a/sqlclassify_test.go +++ /dev/null @@ -1,33 +0,0 @@ -package main - -import "testing" - -func TestIsDML(t *testing.T) { - t.Parallel() - - tests := []struct { - sql string - want bool - }{ - {"UPDATE Singers SET FirstName = 'x' WHERE SingerId = 1", true}, - {"INSERT INTO Singers (SingerId) VALUES (1)", true}, - {"DELETE FROM Singers WHERE SingerId = 1", true}, - {" delete from Singers where SingerId = 1", true}, - {"-- comment\nUPDATE Singers SET FirstName = 'x' WHERE SingerId = 1", true}, - {"/* comment */\nDELETE FROM Singers WHERE SingerId = 1", true}, - {"/* outer */ -- line\n UPDATE T SET c = 1", true}, - {"SELECT * FROM Singers", false}, - {"-- leading comment only\nSELECT 1", false}, - {"WITH x AS (SELECT 1) SELECT * FROM x", false}, - {"", false}, - {"-- only a comment", false}, - } - for _, tc := range tests { - t.Run(tc.sql, func(t *testing.T) { - t.Parallel() - if got := isDML(tc.sql); got != tc.want { - t.Fatalf("isDML(%q) = %v, want %v", tc.sql, got, tc.want) - } - }) - } -} From 87490632692cf08fdeee3f15562428b27c8f9d6f Mon Sep 17 00:00:00 2001 From: apstndb <803393+apstndb@users.noreply.github.com> Date: Mon, 29 Jun 2026 02:59:26 +0900 Subject: [PATCH 4/4] Defer BatchReadOnlyTransaction cleanup on partition query path. Co-authored-by: Cursor --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 7bae720..a1e2bf5 100644 --- a/main.go +++ b/main.go @@ -359,13 +359,13 @@ func _main() error { return err } defer bt.Close() + defer func() { bt.Cleanup(ctx) }() _, err = bt.PartitionQuery(ctx, stmt, spanner.PartitionOptions{}) if err != nil { return err } - bt.Cleanup(ctx) fmt.Println("success") return nil }