-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql: improved diff tool for any namespace #60283
Conversation
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mnovelodou and @rafiss)
pkg/sql/pg_catalog_test.go, line 49 at r1 (raw file):
// Test data files const ( pgCatalogDump = "%s_tables.json" // PostgreSQL pg_catalog schema
Doesn't make sense to call this pgCatalogDump anymore, same with the comments below.
pkg/sql/pg_catalog_test.go, line 85 at r1 (raw file):
func loadTestData(t testing.TB) PGCatalogTables { var pgCatalogFile PGCatalogFile testdataFile := filepath.Join(testdata, fmt.Sprintf(pgCatalogDump, *postgresCatalog))
I think we should figure out how to parameterize *postgresCatalog here and the other instances where we use it for formatting since right now it's all hardcoded in a global variable.
b27e131
to
c07feba
Compare
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
c07feba
to
6bcbe81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)
pkg/sql/pg_catalog_test.go, line 49 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Doesn't make sense to call this pgCatalogDump anymore, same with the comments below.
Done.
pkg/sql/pg_catalog_test.go, line 85 at r1 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
I think we should figure out how to parameterize *postgresCatalog here and the other instances where we use it for formatting since right now it's all hardcoded in a global variable.
It is done as the value comes from flag
af1c255
to
a8dec9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me except for the folder name / some var names.
Need approval from @rafiss as well.
a8dec9f
to
6f89411
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry! i just noticed some more names that should be renamed now.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mnovelodou and @RichardJCai)
pkg/sql/pg_catalog_diff.go, line 1 at r3 (raw file):
// Copyright 2020 The Cockroach Authors.
nit: rename the file to pg_metadata_diff.go
pkg/sql/pg_catalog_diff.go, line 24 at r3 (raw file):
// GetPGCatalogSQL is a query uses udt_name::regtype instead of data_type column because // data_type only says "ARRAY" but does not say which kind of array it is. const GetPGCatalogSQL = `
nit: rename to GetPGMetadataSQL
pkg/sql/pg_catalog_diff.go, line 41 at r3 (raw file):
// PGCatalogColumn is a structure which contains a small description about the datatype of a column, but this can also be // used as a diff information if populating ExpectedOid. Fields are exported for Marshaling purposes. type PGCatalogColumn struct {
nit: rename to PGMetadataColumn
pkg/sql/pg_catalog_diff.go, line 49 at r3 (raw file):
// PGCatalogColumns maps column names to datatype description type PGCatalogColumns map[string]*PGCatalogColumn
nit: rename to PGMetadataColumns
pkg/sql/pg_catalog_diff.go, line 61 at r3 (raw file):
// - If column Name points to a not null PGCatalogColumn, the test column describes how we expect that data type to be // different between cockroach db and postgres type PGCatalogTables map[string]PGCatalogColumns
nit: rename to PGMetadataTables
pkg/sql/pg_catalog_diff.go, line 65 at r3 (raw file):
// PGCatalogFile is used to export pg_catalog from postgres and store the representation of this structure as a // json file type PGCatalogFile struct {
nit: rename to PGMetadataFile
pkg/sql/pg_catalog_diff.go, line 67 at r3 (raw file):
type PGCatalogFile struct { PgVersion string `json:"pgVersion"` PgCatalog PGCatalogTables `json:"pgCatalog"`
nit: rename the variable to PgMetadata
also, not sure but should it be PGMetadata
to match the capitalisation of the other names?
6f89411
to
48fcac7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mnovelodou, @rafiss, and @RichardJCai)
pkg/sql/pg_catalog_diff.go, line 41 at r3 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: rename to
PGMetadataColumn
Renamed PGMetadataColumnType as this structure contains information more related to the type
Previously, diff tool worked only for pg_catalog This was inadequate because it can be used for information_schema as well To address this, this patch takes the namespace as parameter to compare a different database Release note: None Fixes cockroachdb#58037 Release justification: non-production code changes
48fcac7
to
f95040f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! thanks
bors r+
Reviewed 1 of 5 files at r1, 1 of 3 files at r3, 6 of 6 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)
Build failed (retrying...): |
This PR was included in a batch that was canceled, it will be automatically retried |
Build succeeded: |
Previously, diff tool worked only for pg_catalog
This was inadequate because it can be used for information_schema as well
To address this, this patch takes the namespace as parameter to compare a
different database
Release note: None
Release justification: non-production code changes
Fixes #58037