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

Add ForceNew for table column name changes #363

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

bobbyiliev
Copy link
Contributor

As discussed here, adding a ForceNew for table column name changes as we do not yet support RENAME COLUMN:

https://github.com/MaterializeInc/database-issues/issues/1640

@bobbyiliev bobbyiliev requested a review from dehume as a code owner November 15, 2023 11:18
Copy link
Contributor

@dehume dehume left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. While we are doing this I think we need to make two other changes:

  • We should include a ForceNew for both type and nullable as well. The nested ForceNew is confusing but right now I think it would trigger a force new on new columns being added OR changing the name of an existing column. So we still want to catch changes on an existing column if it is type or nullable
  • The only column update we support are comments, so comments do not need a ForceNew but the update it here to column. Since currently it will never trigger.

@bobbyiliev
Copy link
Contributor Author

Thanks for doing this. While we are doing this I think we need to make two other changes:

  • We should include a ForceNew for both type and nullable as well. The nested ForceNew is confusing but right now I think it would trigger a force new on new columns being added OR changing the name of an existing column. So we still want to catch changes on an existing column if it is type or nullable
  • The only column update we support are comments, so comments do not need a ForceNew but the update it here to column. Since currently it will never trigger.

Ah yes! very good catch, I missed this initially as I only was testing the table comments rather than column comments. I've made some changes to this now and it seems to be working as expected. Let me know what you think @dehume!

@bobbyiliev bobbyiliev requested a review from dehume November 15, 2023 13:03
@dehume dehume added the bug Something isn't working label Nov 15, 2023
@bobbyiliev bobbyiliev merged commit fce621d into main Nov 15, 2023
4 checks passed
@bobbyiliev bobbyiliev deleted the bugfix-table-column-name-force-new branch November 15, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants