[davem-bis]

Description

Fixes #5793.

Ensures that all snapshots that are representative but not deployable (they contain the same data as prod, but can never become prod) are just pass through views of the production physical table. A by-product of this is that they can never have any missing intervals.

Test Plan

• Expansion of existing unit tests for changed logic.
• Run through of INDIRECT_BREAKING_CHANGES snapshots to prove idempotence.
  • Run plan:

    1. sqlmesh init

       Option 1 throughout - basic DuckDB implementation with the demo models.

    2. sqlmesh plan

    3. Add 'ABC' AS extra_column to incremental_model.sql

    4. sqlmesh plan indirect_non_breaking

    5. sqlmesh run

    6. sqlmesh run indirect_non_breaking

    7. sqlmesh fetchdf "UPDATE sqlmesh._intervals SET end_ts = end_ts - 86400000, start_ts = start_ts - 86400000, last_altered_ts = last_altered_ts - 86400000"

       86400000 is the number of milliseconds in a day (246060*1000)

    8. sqlmesh run indirect_non_breaking

    9. sqlmesh run indirect_non_breaking

       Previously this would result in the intervals being re-run, it now correctly does nothing.

    10. Plan a new model named extra_model as a SELECT * FROM sqlmesh_example.full_model and confirm that it is selecting from the full_model physical model without the __dev extension.

  • Comments:

    • Model Name	Change Type	Deployability	Representativeness	Version Used In indirect_non_breaking
      seed_model	None	True	True	Same as prod
      incremental_model	NonBreaking	True	True	Separate physical model
      full_model	IndirectNonBreaking	False	True	Prod physical model

    • full_model uses the non-__dev physical model, and has no missing intervals.
• Run through of the above plan, but with vde_mode: dev_only.

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

[davem-bis]

This needs an appropriate unit test.

[cmgoffena13]

@davem-bis -- Took a peak at this: looks like you're making the change a level down, so it affects plan as well. That would explain your comment about why in "dev_only" mode you're getting empty tables (probably because of the return []).

So we're seeing another bug that creates and backfills a __dev table during the plan as well. And by fixing the first bug, this __dev table is then not used and wasted compute, from what I understand. So we're kind of peeling back an onion here.

My recommendation:

1. In sqlmesh/core/scheduler.py in merged_missing_intervals, the function not the method, we filter out the snapshots. This change should propagate to the scheduler and to plan. So something like this:

snapshots_to_intervals = compute_interval_params(          # NEW
    snapshots,
    start=start or earliest_start_date(snapshots),
    end=end or now_timestamp(),
    deployability_index=deployability_index,
    execution_time=execution_time or now_timestamp(),
    restatements=restatements,
    start_override_per_model=start_override_per_model,
    end_override_per_model=end_override_per_model,
    ignore_cron=ignore_cron,
    end_bounded=end_bounded,
)
deployability_index = deployability_index or DeployabilityIndex.all_deployable()  # NEW
return {                                                   # NEW
    s: intervals                                           # NEW
    for s, intervals in snapshots_to_intervals.items()     # NEW
    if not (                                               # NEW
        deployability_index.is_representative(s)           # NEW
        and not deployability_index.is_deployable(s)       # NEW
    )                                                      # NEW
}  

2. In sqlmesh/core/plan/stage.py in _get_snapshots_to_create / _should_create -- we check the deployability index here as well to avoid creating a __dev table if it is_representative. You're already doing this.

I think its your changes to core/snapshot/definition.py that are giving you trouble here. You're overriding logic too early. Appreciate you trying to solve both issues in one pass!

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• 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.

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[davem-bis]

Hi @cmgoffena13 - sorry for the massive delay but I'm picking this up again.

I'm still hitting a sticking point when VDE mode is dev_only, specifically that the table is empty as an empty physical table is created. I can see two main options to this:

1. The virtual layer points at a new physical table that is cloned from the production table, or;
2. The virtual layer points directly at the production table (presumably by expanding the logic in sqlmesh/core/snapshot/definition.py::475).

Option 2. is easiest to implement, but I don't think that pattern is used elsewhere (the physical tables look to be cloned from the production tables in this scenario by the looks of it) I stand corrected that's exactly what happens when you specify --include-unmodified.

Option 1. 1 would break if it ever tried to clone something that wasn't a table, but I believe that this scenario can only occur for tables (as opposed to a view or ephemeral) so that shouldn't be an issue.

I'm going to implement option 2 as it matches existing behaviour, but please shout if you think there's something I've missed.