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

New interface to rewrite table on certain schema change operations #1002

Merged
merged 25 commits into from
May 9, 2022
Merged

New interface to rewrite table on certain schema change operations #1002

merged 25 commits into from
May 9, 2022

Conversation

zachmu
Copy link
Member

@zachmu zachmu commented May 6, 2022

Interface isn't quite final but this can be checked in. Complementary dolt changes are done and tested but want to get feedback on this approach first.

zachmu and others added 24 commits April 22, 2022 14:41
@zachmu
First pass at rewrite table
aab4ee7
@zachmu
Slight updates
38bed7c
@zachmu
Apply defaults in a single pass when rewriting table
83089b3
@zachmu
Unit tests and bug fixes for schema alteration
d2bcbf6
@zachmu
Bug fix for nullable columns
bb78b40
@zachmu
Better tests for add column
24a820c
@zachmu
Refined rewrite criteria. Refactored ModifyColumn to do work in Next()
20a2f4e
@zachmu
Bug fix: use resolved schema, not raw one from table
6ebc2a8
@zachmu
Better tests of drop column errors
0f64d09
@zachmu
Better tests for add column edge cases
a96ad3c
@zachmu
Doing something a little dramatic
518da48
@zachmu
Merge main
33549e4
@zachmu
Fix compile errors
ebdbea2
@zachmu
Added schema target to AlterPk
000ad2e
@zachmu
Added schema target to AlterDefaultSet
efc431d
@zachmu
Added target schema to AlterDefaultDrop
8bec96e
@zachmu
Getting closer, broke some other thingsgs
2333152
@zachmu
Fixed a couple bugs by extending SchemaTarget interface
eee49c6
@zachmu
More test fixes
f675900
@zachmu
All engine tests passing again
b4bce1e
@zachmu
Fixed several reference alteration bugs
947e04d
@zachmu
Fixed bugs in column defaults for rewrites
7aea1ef
@zachmu
Added more testse
f7c2686
@zachmu
[ga-format-pr] Run ./format_repo.sh to fix formatting
f049ff4
@zachmu zachmu requested a review from max-hoffman May 6, 2022 23:14
andy-wm-arthur
andy-wm-arthur reviewed May 9, 2022
View reviewed changes
Copy link
Contributor

@andy-wm-arthur andy-wm-arthur left a comment

Choose a reason for hiding this comment

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

Seems reasonable. It seems at this stage that the engine is deciding when table rewrites happen, not the integrator?

sql/plan/show_create_table.go Show resolved Hide resolved
max-hoffman
max-hoffman approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

LGTM, couple nits, final interface condensing alters into a target schema/projections in a resolve rule is ideal

sql/plan/alter_table.go Show resolved Hide resolved
sql/plan/alter_table.go Outdated Show resolved Hide resolved
sql/plan/alter_table.go Show resolved Hide resolved
@zachmu zachmu merged commit 35526b9 into main May 9, 2022
@max-hoffman max-hoffman deleted the zachmu/ddl-rewrites branch May 9, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-hoffman max-hoffman max-hoffman approved these changes

@andy-wm-arthur andy-wm-arthur andy-wm-arthur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zachmu @andy-wm-arthur @max-hoffman