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

Column Migration: process migrations in batches #2506

Merged
merged 5 commits into from
Mar 11, 2025

Conversation

mdibaiee
Copy link
Member

@mdibaiee mdibaiee commented Mar 10, 2025

Description:

I have been unable to run tests on all connectors, I think the credentials / env for motherduck and redshift seem to have changed and I don't have credentials for cratedb

Workflow steps:

(How does one use this feature, and how has it changed)

Documentation links affected:

(list any documentation links that you created, or existing ones that you've identified as needing updates, along with a brief description)

Notes for reviewers:

(anything that might help someone review this PR)


This change is Reviewable

Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

Generally LGTM. The test credentials for Redshift and Motherduck were updated in #2388, and the working ones are in their respective config files in the integration test folders, here and here. I think these updates will work with them, but it would be good to run the tests and confirm.

@mdibaiee mdibaiee force-pushed the mahdi/column-migration-number branch from e4cae93 to 94d70d4 Compare March 10, 2025 21:05
@mdibaiee mdibaiee requested a review from williamhbaker March 10, 2025 21:26
@mdibaiee mdibaiee force-pushed the mahdi/column-migration-number branch from 3b68adc to 0c7192f Compare March 11, 2025 12:01
@mdibaiee mdibaiee force-pushed the mahdi/column-migration-number branch from 0c7192f to 6e27793 Compare March 11, 2025 12:26
@mdibaiee
Copy link
Member Author

@williamhbaker added a commit for supporting migration from integer to number as well to this same PR

Copy link
Member

@williamhbaker williamhbaker left a comment

Choose a reason for hiding this comment

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

LGTM for the number migrations.

It would be nice to cover materialize-azure-fabric-warehouse for the number migration case as well. It does migrations a little differently since it doesn't support dropping columns, but it might be fairly trivial to add these cases to it if they can use the default CAST syntax.

@mdibaiee mdibaiee merged commit 1e7bcce into main Mar 11, 2025
55 of 59 checks passed
@mdibaiee mdibaiee deleted the mahdi/column-migration-number branch March 11, 2025 15:22
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