Skip to content

Fix: do not apply model_defaults statements to external models - #6189

Open
simen-strand wants to merge 3 commits into
SQLMesh:mainfrom
simen-strand:fix/external-model-skip-default-statements
Open

simen-strand wants to merge 3 commits into
SQLMesh:mainfrom
simen-strand:fix/external-model-skip-default-statements

Conversation

@simen-strand

@simen-strand simen-strand commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

External models inherit the pre_statements, post_statements and on_virtual_update from model_defaults, but they never run them, and they're built without the project's macros. If a default statement calls a project macro and you edit it, the plan tries to render the external models' copies and fails:

ConfigError: Failed to resolve macros for
@tag_query('v2')
Macro 'tag_query' does not exist.
 at '.../external_models.yaml'

This PR stops external models from inheriting those three defaults, the same way they already skip the default gateway. Statements set explicitly on an external model are kept. The docs now say the defaults don't apply to external models.

Upgrade impact

Projects whose model_defaults define statements will see a one-off change on their first plan after upgrading:

  • Every external model shows as changed (non-breaking), and the models downstream of it as indirectly changed. No backfill is needed.
  • Exception: if SQLMesh can't determine the columns of the external model or of any model downstream of it (e.g. SELECT * from an undeclared source), the default external: full auto-categorization makes the change breaking, and that external model's downstream models are backfilled. A forward-only plan, or external: semi, avoids that.

Projects without default statements are unaffected.

Alternatives considered

  • If rendering fails, compare the unrendered statements of external models: fingerprints stay the same, so there's no upgrade impact. But external models keep statements that never run, and the fix relies on catching render errors.
  • Pass the project's macros to external models: rendering works. But fingerprints still change, so the upgrade impact is the same, and external models still carry statements that never run.

Test Plan

  • New unit test: external models no longer inherit the default statements, explicit ones are kept, and SQL models still inherit them.
  • New context test, for pre- and post-statements: plan with @tag_query('v1'), change it to v2, plan again. It fails on main with the error above and passes here.
  • Upgrade checked by applying state with main and planning with this branch. The results match the impact described above, and a forward-only plan needs no backfill.

Checklist

  • I have run make style and fixed any issues
  • I have added tests for my changes (if applicable)
  • All existing tests pass (make fast-test)
  • My commits are signed off (git commit -s) per the DCO

Summary by CodeRabbit

  • Bug Fixes

    • External models no longer inherit project-level default pre-statements, post-statements, or virtual-update statements. Statements specified directly on an external model are preserved, and other models continue to receive the defaults.
    • Changes to project-level default statements no longer cause external models to be marked as modified; affected SQL models continue to be evaluated.
  • Documentation

    • Clarified which models receive project-level statement defaults and how defaults combine with model-specific statements.

External models inherited the model_defaults pre_statements, post_statements
and on_virtual_update, but are built without the project's macros and never
execute statements. When a default statement called a project macro and its
text changed, the plan rendered the external model's statements to compare
them and failed with "Macro '<name>' does not exist".

Stop external models from inheriting these defaults, as is already done for
the default gateway. This changes the fingerprint of external models in
projects that use default statements; the empty migration bumps the state
schema version so cached models are rebuilt.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Simen Strand <simen.strand@netcheck.de>
@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

External models no longer inherit project-level pre_statements, post_statements, or on_virtual_update defaults. Explicitly supplied statements remain. Documentation and tests cover default inheritance and model modification tracking.

Changes: External model default statements

Layer / File(s) Summary
Exclude inherited statement defaults
sqlmesh/core/model/definition.py, docs/reference/model_configuration.md, tests/core/test_model.py
External models exclude inherited pre-statements, post-statements, and virtual-update defaults. Tests verify that explicit external-model statements remain and SQL models inherit the defaults.
Cover model modification tracking
sqlmesh/migrations/v0103_exclude_default_statements_from_external_models.py, tests/core/test_context.py
The migration module adds no-op schema and row migration functions. Context tests check that changing statement defaults marks the SQL model as directly modified and that the external model has no inherited pre- or post-statements.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mday-io

Merge Risk: Low · up to 000c7

The first upgrade plan can trigger downstream backfills when columns are unknown. The change is mergeable with this upgrade impact noted for release guidance.

Security architecture risk: Low · up to 000c7

The change narrowly excludes inherited statements from external models while preserving explicit statements and ordinary model defaults. The main design risk is upgrade-time replanning: projects with unknown downstream schemas can receive breaking classifications and backfills. No introduced security finding was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The supported propagation scope is an affected project's external-model dependency graph: inherited statement removal can cause downstream replanning and execution through existing backfill mechanisms. The inspected change does not add credentials, select a new gateway, or introduce a new execution authority.

Trust Boundaries and Controls

  • observed — The external-model loader supplies project defaults separately from explicit YAML model fields. Filtering the defaults therefore preserves explicit model values rather than treating inherited statements as model-specific input. The public-entrypoint routing subjects are test-only code, including a nested writer operating in a temporary project.

Resilience and Maintainability Implications

  • observed — Snapshot persistence and environment promotion retain existing transactional updates, environment locking, and plan-identity conflict checks. Apply is not globally atomic across physical execution and promotion, but the inspected PR does not introduce that limitation or change its control flow.
Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: preventing model_defaults statements from applying to external models.
Description check Passed The description includes the required Description, Test Plan, and Checklist sections. It explains the issue, implementation, upgrade impact, alternatives, tests, and completed checklist items.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 4 files.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0a4ef7e7-b6f3-4c2f-b061-2a5fb7a8c21b
Commits

Reviewing files that changed from the base of the PR and between 644e06b and 000c78c.

Files selected for processing: 1

  • tests/core/test_context.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

  • Fix comments: Fix CodeRabbit comments on this PR

Comment thread docs/reference/model_configuration.md Resolved
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Simen Strand <simen.strand@netcheck.de>
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.

1 participant