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

Implement altering column with SET DEFAULT and DROP DEFAULT #152

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

JanJakes
Copy link
Collaborator

@JanJakes JanJakes commented Aug 9, 2024

This PR implements the support for the following MySQL statements:

ALTER TABLE _tmp_table CHANGE name SET DEFAULT 'abc';
ALTER TABLE _tmp_table CHANGE name DROP DEFAULT;

It required a small refactor so that the CHANGE COLUMN logic can be extracted to a reusable method.

The CHANGE COLUMN logic can be extracted to a reusable method now. I'm only keeping
the changes in a separate commit for a more useful diff.

The update column callback can be used to modify any columns. Since it is also provided with
the old column definition, it will enable column-modifying statements withtout a full column
definition (like SET/DROP default). In the future, it should also unlock implementing expressions
like MODIFY COLUMN and DROP COLUMN and enable modifying multiple columns at once
to avoid unnecessary table copying.
@JanJakes
Copy link
Collaborator Author

@brandonpayton I took another stab at this. It required extracting all the column-changing logic to a private method, and allowing edits to the columns via a callback, so that the old column definition is available to statements like SET/DROP DEFAULT. See the first two commits for that part. The third commit implements the new functionality. I tried to make as few refactors as possible, although some things could be done in a better way, I suppose, but then there would be even more refactor needed. These changes should also unlock reusing the same logic for other operations (e.g., DROP COLUMN).

I'm not sure if it's worth pushing this much further given that #153 would change the translator quite a bit, and will likely be a chance to re-architect the translator a bit more.

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.

2 participants