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

sql: refactored diff tool strucutres to remove JSON complexity #70952

Merged
merged 1 commit into from
Oct 8, 2021

Conversation

mnovelodou
Copy link
Contributor

Previously, diff tool shared the same structures for table metadata
and diffs
This was inadequate because they have different concerns
To address this, this patch refactored the code so every concern or
process have their own data types

Release note: None

@mnovelodou mnovelodou requested review from RichardJCai and a team September 30, 2021 20:49
@blathers-crl
Copy link

blathers-crl bot commented Sep 30, 2021

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.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Sep 30, 2021
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@blathers-crl
Copy link

blathers-crl bot commented Oct 1, 2021

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.

@mnovelodou mnovelodou added the do-not-merge bors won't merge a PR with this label. label Oct 1, 2021
@mnovelodou mnovelodou force-pushed the diff-tool-refactor branch 5 times, most recently from 614b4dc to 0c05da6 Compare October 5, 2021 22:27
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mnovelodou and @RichardJCai)


pkg/sql/testdata/postgres_test_expected_diffs_on_pg_catalog.json, line 8 at r1 (raw file):

    "MissingTables": 0,
    "MissingColumns": 0,
    "DatatypeMismatches": 0

just curious: when did we resolve these dataTypeMisMatches?

Copy link
Contributor Author

@mnovelodou mnovelodou left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)


pkg/sql/testdata/postgres_test_expected_diffs_on_pg_catalog.json, line 8 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

just curious: when did we resolve these dataTypeMisMatches?

This is a nice catch, if you noticed there are datatype mismatches below, let me check

@mnovelodou mnovelodou removed the do-not-merge bors won't merge a PR with this label. label Oct 7, 2021
@mnovelodou mnovelodou requested a review from rafiss October 7, 2021 16:15
@mnovelodou mnovelodou force-pushed the diff-tool-refactor branch 3 times, most recently from da7b59d to 93d6305 Compare October 7, 2021 17:45
Copy link
Contributor

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, minor nits. Awesome change though thanks for this.

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mnovelodou and @rafiss)


pkg/sql/pg_metadata_diff.go, line 60 at r2 (raw file):

}

// PGMetadataColumnDiff describes diffs information for a column type. Fields are exported for Marshaling purposes.

nit: Lower case marshaling


pkg/sql/pg_metadata_diff.go, line 71 at r2 (raw file):

type PGMetadataColumnDiffs map[string]*PGMetadataColumnDiff

// PGMetadataTableDiffs Is used to store and load expected diffs:

nit: s/Is/is


pkg/sql/pg_metadata_test.go, line 95 at r2 (raw file):

// Where to start when debugging?
// -> func TestDiffTool
//

Nice, thanks for this detailed writeup here.


pkg/sql/pg_metadata_test.go, line 267 at r2 (raw file):

}

type DiffTool struct {

Nit: Maybe we should use a more specific name here, VirtualSchemaDiffTool perhaps?

Previously, diff tool shared the same structures for table metadata
and diffs
This was inadequate because they have different concerns
To address this, this patch refactored the code so every concern or
process have their own data types

Release note: None
Copy link
Contributor Author

@mnovelodou mnovelodou left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)


pkg/sql/pg_metadata_test.go, line 95 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Nice, thanks for this detailed writeup here.

Done.

@mnovelodou mnovelodou requested a review from RichardJCai October 7, 2021 20:51
@RichardJCai
Copy link
Contributor

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 8, 2021

Build succeeded:

@craig craig bot merged commit a0d824a into cockroachdb:master Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants