-
-
Notifications
You must be signed in to change notification settings - Fork 238
dolthub/dolt#9481 - Move auto_increment validation to GMS #3084
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move auto_increment constraint validation entirely from Dolt to go-mysql-server for better separation of concerns and MySQL compatibility. Changes: - Add validateAutoIncrementType() function using existing type checking - Fix validation order to prioritize auto_increment before index validation - Unskip comprehensive test coverage for all invalid data types - Fix bugs: YEAR and BIT types were incorrectly allowed - Ensure exact MySQL error message compatibility 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Improve code readability and maintainability by replacing multiple if statements with a comprehensive switch statement on t.Type(). - Add query import for type constants - Explicit case handling for all data types - Clear grouping of allowed vs disallowed types - Better performance with single switch evaluation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Use existing types.Is*() functions instead of vitess constants - Fix blob test expectations (column names were incorrect) - Comprehensive validation matches MySQL behavior exactly - All 22 data types tested: 8 valid, 14 invalid with proper errors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Use single check for types.IsNumber() with exclusions for DECIMAL, YEAR, and BIT - More maintainable and cleaner code structure - Comment lists all excluded types for clarity - All tests still passing with exact MySQL behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
angelamayxie
approved these changes
Jul 10, 2025
Contributor
angelamayxie
left a comment
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.
lgtm
jycor
reviewed
Jul 10, 2025
- Unskip float and double auto_increment tests - Update test expectations to match MySQL 8.0.42 behavior - FLOAT and DOUBLE are allowed with AUTO_INCREMENT in MySQL - Fix collation expectations in SHOW CREATE TABLE tests - All tests now pass confirming GMS matches MySQL behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Changed validateAutoIncrementType() to only allow integer types - FLOAT/DOUBLE with AUTO_INCREMENT now properly rejected (MySQL 8.4.5 behavior) - Updated float/double tests to expect "Incorrect column specifier" errors - Matches MySQL 8.4.5 stricter validation requirements 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Updated "auto increment on float" test to expect CREATE TABLE error - Updated "auto increment on double" test to expect CREATE TABLE error - Both tests now expect "Incorrect column specifier" matching MySQL 8.4.5 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…s.go - Removed "auto increment on float" and "auto increment on double" tests - These scenarios are now impossible since CREATE TABLE fails for float/double AUTO_INCREMENT - Validation is already covered in script_queries.go tests - Eliminates duplicate test coverage for the same validation logic 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
jycor
approved these changes
Jul 11, 2025
Contributor
jycor
left a comment
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.
LGTM
9cc1ac5 to
d3a0287
Compare
angelamayxie
approved these changes
Jul 11, 2025
Contributor
angelamayxie
left a comment
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.
lgtm!
- Removed Skip: true from "set with auto increment" test - Our validation now correctly handles SET columns with AUTO_INCREMENT - All assertions expect "Incorrect column specifier" errors which match MySQL behavior - Fixes issue dolthub/dolt#9470 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
d1e20fa to
7bb7438
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move auto_increment constraint validation entirely from Dolt to go-mysql-server for better separation of concerns and MySQL compatibility.
Fixes dolthub/dolt#9481
Fixes dolthub/dolt#9470