Skip to content

Feat(sqlmesh_dbt): Implement --full-refresh#5370

Merged
erindru merged 2 commits intomainfrom
erin/plan-changes-and-restatements
Sep 17, 2025
Merged

Feat(sqlmesh_dbt): Implement --full-refresh#5370
erindru merged 2 commits intomainfrom
erin/plan-changes-and-restatements

Conversation

@erindru
Copy link
Collaborator

@erindru erindru commented Sep 15, 2025

This PR adds the following options to PlanBuilder:

  • always_include_local_changes - essentially overrides "force no diff" to False if restatements are present, so that the ContextDiff always includes local changes. It's also used to short-circuit the check in PlanBuilder to not throw a Model changes and restatements can't be a part of the same plan error
  • clear_restated_intervals_across_model_versions - moves the logic on whether or not to produce RestatementStage (which clears non-prod intervals from state) from the stage builder back to the plan builder where more config information, such as the VDE mode, is available.

Note that these new flags are exposed on context.plan_builder and not context.plan. The reason is so they can be kept as escape hatches for dbt compatibility and not be exposed in native/standard plan.

Utilizing these new flags, this PR also implements the --full-refresh option on the sqlmesh_dbt cli

@erindru erindru marked this pull request as ready for review September 15, 2025 22:01
if not plan.restatements or plan.is_dev:
# The RestatementStage to clear intervals from state across all environments is not needed for plans against dev, only prod
return None
if plan.restatements and plan.clear_restated_intervals_across_model_versions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add an assert here to ensure plan.is_dev. I think it's critical enough that we want to make sure we never restate cross version in dev

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ensure plan.is_dev

Do you mean ensure plan.is_prod since we want to emit RestatementStage on prod plans only?

I've updated the code to raise an error if plan.is_dev which indicates that the flag was not set properly upstream


# When handling prod restatements, only clear intervals from other model versions if we are using full virtual environments
# If we are not, then there is no point, because none of the data in dev environments can be promoted by definition
clear_restated_intervals_across_model_versions = (
Copy link
Collaborator

@izeigerman izeigerman Sep 15, 2025

Choose a reason for hiding this comment

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

versions seems overloaded here. I initially interpreted this as a snapshot version, when in reality this refers to model version aka "snapshots". Should we call this restate_all_snapshots?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, restate is overloaded as well. The term refers to the re-processing of data, but actually that's implemented as a backfill.

restate in the context of a plan just refers to clearing intervals from state, which is very much non-obvious from the word restate which is why I had clear_restated_intervals to describe what it actually does.

It's also not all snapshots - just ones that arent promoted in prod.

Maybe clear_restated_intervals_from_dev_snapshots?

Copy link
Collaborator

@izeigerman izeigerman Sep 15, 2025

Choose a reason for hiding this comment

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

It's also not all snapshots - just ones that arent promoted in prod.

The prod ones are cleared by definition. They just get immediately backfilled. The fact that we don't update state for those is an implementation detail and is not relevant here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess we think of "cleared" differently then. I tend to think of it in terms of actually physically deleting records from state.

But no problem, I can call this flag restate_all_snapshots

)

def _ensure_no_new_snapshots_with_restatements(self) -> None:
if self._always_include_local_changes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we just remove this check and rely on context to provide a valid ContextDiff? This way we don't need to worry about passing always_include_local_changes just to perform the check and can instead rely on the correctly formed ContextDiff instance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I did it this way because of the test_new_snapshots_with_restatements test that specifically validates the ContextDiff had been set up as expected.

I've updated the PR to put the burden on the caller and removed the test

@erindru erindru force-pushed the erin/plan-changes-and-restatements branch from 27a1204 to ba111a2 Compare September 16, 2025 00:21
@erindru erindru merged commit d26c779 into main Sep 17, 2025
45 of 47 checks passed
@erindru erindru deleted the erin/plan-changes-and-restatements branch September 17, 2025 01:14
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