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

Makes several new kinds of alter table statements involving auto_increment columns work correctly #1924

Merged
merged 17 commits into from Aug 4, 2023

Conversation

zachmu
Copy link
Member

@zachmu zachmu commented Aug 3, 2023

Also: makes several kinds of ALTER TABLE statements with multiple clauses more lenient in their error checking than MySQL. These statements will now succeed instead of being rejected.

Fixes dolthub/dolt#6218

@zachmu zachmu requested a review from fulghum August 3, 2023 22:16
@zachmu
Copy link
Member Author

zachmu commented Aug 3, 2023

I need to do another pass on CREATE TABLE statements that contain similar constructions, I doubt they work properly yet

@zachmu
Copy link
Member Author

zachmu commented Aug 4, 2023

I need to do another pass on CREATE TABLE statements that contain similar constructions, I doubt they work properly yet

Nevermind, I added tests of this and it does work correctly

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looks good! 🚢

enginetest/enginetests.go Show resolved Hide resolved
sql/plan/alter_table.go Outdated Show resolved Hide resolved
}
}

return keyedColumns
Copy link
Contributor

Choose a reason for hiding this comment

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

(super minor/nit) you don't technically need to return the map, right? It'll never get reallocated like a slice does, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is true

@zachmu zachmu merged commit 46bed6e into main Aug 4, 2023
6 checks passed
@Hydrocharged Hydrocharged deleted the zachmu/alter-autoinc branch August 24, 2023 00:17
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.

Can't add a new auto_increment column with a single alter statement
2 participants