Skip to content

Materialize: Add copy option to strip constraints from DDL#6422

Merged
sougou merged 1 commit intovitessio:masterfrom
planetscale:tj-materialize-constraint
Jul 9, 2020
Merged

Materialize: Add copy option to strip constraints from DDL#6422
sougou merged 1 commit intovitessio:masterfrom
planetscale:tj-materialize-constraint

Conversation

@teejae
Copy link
Copy Markdown
Contributor

@teejae teejae commented Jul 8, 2020

Signed-off-by: Toliver Jue toliver@planetscale.com

Add a copy:drop_constraint copy type to Materialize

@teejae teejae force-pushed the tj-materialize-constraint branch from 30501be to b8e212c Compare July 8, 2020 12:26
Signed-off-by: Toliver Jue <toliver@planetscale.com>
@teejae teejae force-pushed the tj-materialize-constraint branch from b8e212c to 03c8406 Compare July 8, 2020 12:36
@teejae teejae marked this pull request as ready for review July 8, 2020 17:27
@teejae teejae requested a review from sougou as a code owner July 8, 2020 17:27
Copy link
Copy Markdown
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

This is a good idea. The one problem with our DDL parser is that it doesn't handle all constructs. So, many constructs will fail if we went this route.

Due to this, I tried a different approach for modifying the DDL, which is based on knowing how mysql generates the create table string. I'm wondering if we can capture more use cases. Essentially, I massage the statement using string matches. Here's how I did it: https://github.com/vitessio/vitess/blob/master/go/vt/wrangler/materializer.go#L305-L331.

If this is too much trouble we can go with your approach knowing that it won't work for all cases.

@teejae
Copy link
Copy Markdown
Contributor Author

teejae commented Jul 9, 2020

@sougou yea, i was potentially worried about it failing for the general case. however @systay mentioned that this happened to work for constraints in particular.

separately, i had formatted the copy:xxx string such that we could eventually go with many options, like copy:drop_constraint,drop_index or whatever, and be backwards compatible api-wise.

one benefit with this PR's approach is that it should be robust to failure, and fail-fast, by not being able to parse certain constructs. so i'm inclined to have a relatively limited option first, knowing we can go with the more generalized approaches later. this has limited blast radius in event it's "done wrong".

@teejae teejae requested a review from sougou July 9, 2020 02:18
@sougou sougou merged commit e4f4d82 into vitessio:master Jul 9, 2020
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
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.

3 participants