Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(spanner): upgrade spanner client to latest to support NUMERIC types #565

Merged
merged 3 commits into from
May 19, 2021
Merged

fix(spanner): upgrade spanner client to latest to support NUMERIC types #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b698f2a
fix(spanner): upgrade spanner client to latest to support NUMERIC types
kylechadha May 14, 2021
d1901a4
run go mod tidy
kylechadha May 14, 2021
71a056a
Add spanner example migration test case
kylechadha May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions database/spanner/examples/migrations/1621360367_create_transactions_table.down.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP TABLE Transactions
6 changes: 6 additions & 0 deletions database/spanner/examples/migrations/1621360367_create_transactions_table.up.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
CREATE TABLE Transactions (
UserId INT64,
TransactionId STRING(40),
Total NUMERIC
) PRIMARY KEY(UserId, TransactionId),
INTERLEAVE IN PARENT Users ON DELETE CASCADE
8 changes: 8 additions & 0 deletions database/spanner/spanner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,14 @@ func TestCleanStatements(t *testing.T) {
SET OPTIONS (allow_commit_timestamp=true);`,
expected: []string{"ALTER TABLE users ALTER COLUMN created SET OPTIONS (allow_commit_timestamp = true)"},
},
{
name: "column with NUMERIC type",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only tests that cleanStatements() passes. It doesn't test running the statement against the emulator. Was this your intention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, just updated where the last spanner upgrade PR did #427 -- will add a migration to test this case as well. Let me know if there's anywhere else that should be updated!

Copy link
Member

@dhui dhui May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If DDL parsing is all you want to test, then this is fine. But if you want to test table creation, then this doesn't test that. I'm fine either way.

EDIT: I see you added a new example migration that creates a table w/ a numeric column. These examples should be run as part of a normal test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dhui not sure I understand, when you say 'these examples' are you saying the tests are good as is, or that I should remove the migrations and add a new test function? If you have an example of what you're referring to that would be great

multiStatement: `CREATE TABLE table_name (
id STRING(255) NOT NULL,
sum NUMERIC,
) PRIMARY KEY (id)`,
expected: []string{"CREATE TABLE table_name (\n id STRING(255) NOT NULL,\n sum NUMERIC,\n) PRIMARY KEY(id)"},
},
}

for _, tc := range testCases {
Expand Down
12 changes: 5 additions & 7 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
module github.com/golang-migrate/migrate/v4

require (
cloud.google.com/go v0.64.0 // indirect
cloud.google.com/go/spanner v1.9.0
cloud.google.com/go/spanner v1.18.0
cloud.google.com/go/storage v1.10.0
github.com/ClickHouse/clickhouse-go v1.4.3
github.com/aws/aws-sdk-go v1.17.7
Expand Down Expand Up @@ -37,7 +36,7 @@ require (
github.com/nakagami/firebirdsql v0.0.0-20190310045651-3c02a58cfed8
github.com/neo4j/neo4j-go-driver v1.8.1-0.20200803113522-b626aa943eba
github.com/snowflakedb/gosnowflake v1.3.5
github.com/stretchr/testify v1.5.1
github.com/stretchr/testify v1.6.1
github.com/tidwall/pretty v0.0.0-20180105212114-65a9db5fad51 // indirect
github.com/xanzy/go-gitlab v0.15.0
github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c // indirect
Expand All @@ -46,11 +45,10 @@ require (
go.mongodb.org/mongo-driver v1.1.0
go.uber.org/atomic v1.6.0
golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83 // indirect
golang.org/x/text v0.3.5 // indirect
golang.org/x/time v0.0.0-20200630173020-3af7569d3a1e // indirect
golang.org/x/tools v0.0.0-20210106214847-113979e3529a
google.golang.org/api v0.30.0
google.golang.org/genproto v0.0.0-20210207032614-bba0dbe2a9ea
golang.org/x/tools v0.1.0
google.golang.org/api v0.45.0
google.golang.org/genproto v0.0.0-20210427215850-f767ed18ee4d
modernc.org/b v1.0.0 // indirect
modernc.org/db v1.0.0 // indirect
modernc.org/file v1.0.0 // indirect
Expand Down
Loading