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.

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

fix timestamp with default values comparison #54

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

reedom
Copy link
Contributor

@reedom reedom commented Aug 8, 2023

If both sides have identical columns of timestamp with a default value,

CREATE TABLE  (
  column TIMESTAMP NOT NULL DEFAULT(TIMESTAMP '2000-01-01 00:00:00.000000+00:00'),
  …

it panics:

panic: cannot handle unexported field at {spansql.ColumnDef}.Default.(spansql.TimestampLiteral).wall:
	"cloud.google.com/go/spanner/spansql".TimestampLiteral
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]
	panic: cannot handle unexported field at {spansql.ColumnDef}.Default.(spansql.TimestampLiteral).wall:
	"cloud.google.com/go/spanner/spansql".TimestampLiteral
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported

test that reproduces:

https://github.com/murasaki-bv/hammer/blob/f8af0e9e7cd4b06716aee0fdffe46c21db6aa690/internal/hammer/diff_test.go#L1364-L1381

@reedom reedom changed the title fix: fix timestamp with default fields comparison fix timestamp with default fields comparison Aug 8, 2023
@reedom reedom changed the title fix timestamp with default fields comparison fix timestamp with default values comparison Aug 8, 2023
Copy link
Owner

@daichirata daichirata left a comment

Choose a reason for hiding this comment

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

@reedom Thank you sincerely for the bug report! I have left some comments, please check them out.

@@ -611,7 +611,7 @@ func (g *Generator) primaryKeyEqual(x, y *Table) bool {
}

func (g *Generator) columnDefEqual(x, y spansql.ColumnDef) bool {
return cmp.Equal(x, y, cmpopts.IgnoreTypes(spansql.Position{}))
return cmp.Equal(x, y, cmpopts.IgnoreTypes(spansql.Position{}), cmpopts.IgnoreUnexported(spansql.TimestampLiteral{}))
Copy link
Owner

Choose a reason for hiding this comment

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

If I ignore changes here, won't changes to the default value be ignored?

I thought the code would be much better if it could be changed to pass the following test cases.

			from: `
CREATE TABLE Nonces (
  nonce INT64 NOT NULL,
  expires TIMESTAMP NOT NULL DEFAULT(TIMESTAMP '2000-01-01 00:00:00.000000+00:00'),
) PRIMARY KEY(nonce);
`,
			to: `
CREATE TABLE Nonces (
  nonce INT64 NOT NULL,
  expires TIMESTAMP NOT NULL DEFAULT(TIMESTAMP '2000-01-01 12:00:00.000000+00:00'),
) PRIMARY KEY(nonce);
`,
			ignoreAlterDatabase: true,
			expected: []string{
				`ALTER TABLE Nonces ALTER COLUMN expires TIMESTAMP NOT NULL DEFAULT (TIMESTAMP '2000-01-01 12:00:00.000000+00:00')`,
			},

This is an example of an approach, but you might want to make the following comparison here and modify it so that subsequent code will issue an alter statement for changes in the default value.

cmp.Comparer(func(x, y spansql.TimestampLiteral) bool {
	return time.Time(x).Equal(time.Time(y))
}))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad…

Ok, I've improved the code.
Could you check it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it seems I had rerun only a failed test at that time. 🙇
Now the test runs in success locally.

@daichirata daichirata merged commit cd53fc6 into daichirata:master Aug 10, 2023
6 checks passed
@daichirata
Copy link
Owner

Thank you for fixing the problem! I appreciate your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants