Skip to content

fix: consider sqlmesh major/minor for migrate and rollback #3446

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.

Sign up for GitHub

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
izeigerman merged 7 commits into from
Dec 4, 2024
Merged

fix: consider sqlmesh major/minor for migrate and rollback #3446

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
63ae19c
fix: consider sqlmesh major/minor for migrate and rollback
MikeWallis42 Dec 2, 2024
f9eb9fa
bugfix account for breaking change in dbt-fabric
MikeWallis42 Dec 2, 2024
6520ffd
bugfix pinning bitnami image due to pyspark pandas 3.12 issue
MikeWallis42 Dec 2, 2024
db65e00
bugfix remove impact to snapshot and environment migration process
MikeWallis42 Dec 2, 2024
633257f
revert migrate function changes
MikeWallis42 Dec 3, 2024
08c426d
Revert "bugfix pinning bitnami image due to pyspark pandas 3.12 issue"
MikeWallis42 Dec 3, 2024
9554f26
Revert "bugfix account for breaking change in dbt-fabric"
MikeWallis42 Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions sqlmesh/core/state_sync/engine_adapter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1307,16 +1307,24 @@ def _apply_migrations(
) -> bool:
versions = self.get_versions(validate=False)
migrations = MIGRATIONS[versions.schema_version :]

migrate_rows = migrations or major_minor(SQLGLOT_VERSION) != versions.minor_sqlglot_version
if not skip_backup and migrate_rows:
should_backup = any(
[
migrations,
major_minor(SQLGLOT_VERSION) != versions.minor_sqlglot_version,
major_minor(SQLMESH_VERSION) != versions.minor_sqlmesh_version,
Copy link
Member

@izeigerman izeigerman Dec 2, 2024

Choose a reason for hiding this comment

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

What is the motivation for this change? The sqlmesh minor version was omitted intentionally to avoid migration when it's not needed even if the minor version was bumped. Generally there are only 2 reasons to migrate: if there's a new migration script (the migrations collection) or if the sqlglot version changed.

Copy link
Member

@izeigerman izeigerman Dec 2, 2024

Choose a reason for hiding this comment

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

I read the issue carefully and now I understand the motivation better. However, migrate_rows not only controls whether the backups are made but also whether the migration should be performed. I suggested decoupling the 2 and only do backups on sqlmesh minor versions changes without affecting the migrate_rows flag.

Copy link
Contributor Author

@MikeWallis42 MikeWallis42 Dec 2, 2024

Choose a reason for hiding this comment

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

After reading your comments and digging in to the code a bit more I believe I can see what you're talking about.

Hopefully I've addressed your concerns by only performing a table copy when it's a sqlmesh minor change without impacting the flag that drives the complex snapshot and environment migration process.

Copy link
Member

@izeigerman izeigerman Dec 3, 2024

Choose a reason for hiding this comment

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

Yes, this looks great, thank you! However, I'm not sure why the refactor was performed in the other method (def migrate)? Is that change necessary? If not, can we please revert it.

]
)
if not skip_backup and should_backup:
self._backup_state()

for migration in migrations:
logger.info(f"Applying migration {migration}")
migration.migrate(self, default_catalog=default_catalog)

return bool(migrate_rows)
migrate_snapshots_and_environments = (
bool(migrations) or major_minor(SQLGLOT_VERSION) != versions.minor_sqlglot_version
)
return migrate_snapshots_and_environments

def _migrate_rows(self, promoted_snapshots_only: bool) -> None:
logger.info("Fetching environments")
Expand Down