feat: add MDL layout versioning and dialect field on Model and View#1556
Merged
douenergy merged 4 commits intoCanner:mainfrom Apr 16, 2026
Merged
feat: add MDL layout versioning and dialect field on Model and View#1556douenergy merged 4 commits intoCanner:mainfrom
douenergy merged 4 commits intoCanner:mainfrom
Conversation
Add `layoutVersion` to Manifest for schema feature gating, and `dialect: Option<DataSource>` to Model and View so each entity can declare what SQL dialect its embedded SQL is written in. Rust (wren-core-base): - Manifest macro: `layout_version` field (defaults to 1 via serde) - Model/View macros: `dialect` field (optional, defaults to None) - MAX_SUPPORTED_LAYOUT_VERSION constant + validation - ManifestBuilder/ModelBuilder/ViewBuilder: new builder methods - migration.rs: `migrate_manifest()` with sequential v1→v2 step - Python bindings: layout_version getter, migrate_manifest_json() Python (wren SDK): - schema_version 3 support → layoutVersion 2 mapping - build_json() stamps layoutVersion from schema_version - convert_mdl_to_project() maps layoutVersion→schema_version, preserves dialect on models/views - validate_project() validates dialect values, warns if used with schema_version < 3 - context_init() defaults new projects to schema_version 3 Documentation: - wren_project.md: schema_version 3, dialect examples, field mapping table, new Dialect Override section - model.md / view.md: dialect field in Fields table Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
documentation
Contributor
📝 WalkthroughWalkthroughAdds optional per-model/view Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Python CLI
participant Ctx as Context Builder
participant Val as Validator
participant Mig as Migration Engine
participant Store as Manifest Store
CLI->>Ctx: convert_mdl_to_project(mdl_json)
Ctx->>Ctx: derive layoutVersion ↔ schema_version
Ctx->>Val: validate_project(config, models/views)
Val->>Val: check dialect presence vs schema_version
Val->>Val: validate dialect against allowed set
Val-->>Ctx: validation result (warnings/errors)
CLI->>Ctx: build_json(project_cfg)
Ctx->>Mig: migrate_manifest(manifest_json, target_layout)
Mig->>Mig: apply sequential migration steps (e.g., 1→2)
Mig-->>Store: return migrated manifest JSON (with layoutVersion)
Store-->>CLI: final manifest JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren/src/wren/context_cli.py (1)
39-44:⚠️ Potential issue | 🟡 MinorUpdate
init()docstring to match schema v3 scaffolding.Line 43 still says “v2 YAML project”, but Line 99 now scaffolds
schema_version: 3. Please align the docstring to avoid confusing CLI users.✏️ Proposed fix
- v2 YAML project, ready for `wren context validate/build`. + v3 YAML project, ready for `wren context validate/build`.Also applies to: 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/context_cli.py` around lines 39 - 44, Update the init() docstring to reflect schema v3 scaffolding: change the phrase "v2 YAML project" to "v3 YAML project (schema_version: 3)" or similar so the documentation matches the implementation that sets schema_version: 3 in the scaffolding; edit the docstring in the init() function in context_cli.py accordingly to avoid the mismatch.
🧹 Nitpick comments (3)
wren-core-py/src/extractor.rs (1)
83-83: Add a regression assertion for extractedlayout_version.Line 83 is a behavior change worth locking with a unit test (e.g.,
extract_by(...)result retains source manifest layout version).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-py/src/extractor.rs` at line 83, Add a unit test asserting that the extracted object's layout_version equals the source manifest layout_version (regress the behavior change): in tests for extract_by(...) (or the relevant extraction function in extractor.rs) create a manifest with a known layout_version, call extract_by(...) to produce the extracted model (where layout_version is assigned from mdl.manifest.layout_version), and assert extracted.layout_version == mdl.manifest.layout_version to lock the intended behavior; place this test near other extract tests and name it to indicate regression protection for layout_version retention.wren-core-py/src/manifest.rs (1)
26-34: Add direct tests formigrate_manifest_jsonAPI behavior.Please add unit tests for at least one success path and one failure path so Python-facing migration behavior/error mapping is pinned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-py/src/manifest.rs` around lines 26 - 34, Add Rust unit tests for the Python-exposed API migrate_manifest_json: create one test that calls migrate_manifest_json with a known-valid manifest JSON and a target_version that should succeed and assert Ok(String) contains expected migrated fields, and another test that calls migrate_manifest_json with malformed or incompatible JSON (or a target_version that migration::migrate_manifest will fail on) and assert it returns Err(CoreError) and that the error message contains the original migration error text; place tests in a #[cfg(test)] mod tests in the same manifest.rs file and reference the function migrate_manifest_json and the CoreError mapping to ensure the Python-facing behavior is pinned.wren-core-base/src/mdl/manifest.rs (1)
1-20: Minor: Import statement placed before license header.The
use std::error::Error;import on line 1 is placed before the Apache license header comment. For consistency with the rest of the codebase, imports should come after the license block.♻️ Suggested fix
+/* + * Licensed to the Apache Software Foundation (ASF) under one + * ...license text... + */ use std::error::Error; -/* - * Licensed to the Apache Software Foundation (ASF) under one - * ...license text... - */ use std::fmt::Display;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-base/src/mdl/manifest.rs` around lines 1 - 20, Move the stray import line so it sits after the Apache license block with the other imports: remove the leading "use std::error::Error;" that appears before the comment header and place it alongside "use std::fmt::Display;" (and any other use statements) inside manifest.rs so all imports appear after the license header.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@wren/src/wren/context_cli.py`:
- Around line 39-44: Update the init() docstring to reflect schema v3
scaffolding: change the phrase "v2 YAML project" to "v3 YAML project
(schema_version: 3)" or similar so the documentation matches the implementation
that sets schema_version: 3 in the scaffolding; edit the docstring in the init()
function in context_cli.py accordingly to avoid the mismatch.
---
Nitpick comments:
In `@wren-core-base/src/mdl/manifest.rs`:
- Around line 1-20: Move the stray import line so it sits after the Apache
license block with the other imports: remove the leading "use
std::error::Error;" that appears before the comment header and place it
alongside "use std::fmt::Display;" (and any other use statements) inside
manifest.rs so all imports appear after the license header.
In `@wren-core-py/src/extractor.rs`:
- Line 83: Add a unit test asserting that the extracted object's layout_version
equals the source manifest layout_version (regress the behavior change): in
tests for extract_by(...) (or the relevant extraction function in extractor.rs)
create a manifest with a known layout_version, call extract_by(...) to produce
the extracted model (where layout_version is assigned from
mdl.manifest.layout_version), and assert extracted.layout_version ==
mdl.manifest.layout_version to lock the intended behavior; place this test near
other extract tests and name it to indicate regression protection for
layout_version retention.
In `@wren-core-py/src/manifest.rs`:
- Around line 26-34: Add Rust unit tests for the Python-exposed API
migrate_manifest_json: create one test that calls migrate_manifest_json with a
known-valid manifest JSON and a target_version that should succeed and assert
Ok(String) contains expected migrated fields, and another test that calls
migrate_manifest_json with malformed or incompatible JSON (or a target_version
that migration::migrate_manifest will fail on) and assert it returns
Err(CoreError) and that the error message contains the original migration error
text; place tests in a #[cfg(test)] mod tests in the same manifest.rs file and
reference the function migrate_manifest_json and the CoreError mapping to ensure
the Python-facing behavior is pinned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a285ec2-885e-434c-bc3f-149e2e24bbf3
📒 Files selected for processing (15)
docs/guide/modeling/model.mddocs/guide/modeling/view.mddocs/guide/modeling/wren_project.mdwren-core-base/manifest-macro/src/lib.rswren-core-base/src/mdl/builder.rswren-core-base/src/mdl/manifest.rswren-core-base/src/mdl/migration.rswren-core-base/src/mdl/mod.rswren-core-base/src/mdl/py_method.rswren-core-py/src/extractor.rswren-core-py/src/lib.rswren-core-py/src/manifest.rswren/src/wren/context.pywren/src/wren/context_cli.pywren/tests/unit/test_context.py
build_manifest() omitted the data_source field, so the compiled target/mdl.json was missing dataSource. The engine has no fallback for this — without it, dialect resolution falls back to Datafusion and downstream tools cannot determine the actual data source. Pull data_source from project_config and emit it when set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a CLI command to upgrade a project's schema_version, handling all intermediate steps automatically (v1→v2 restructure flat files into directories, v2→v3 enable dialect field). - plan_upgrade() computes changes without touching disk - apply_upgrade() writes the changes - --dry-run previews what would change - --to N targets a specific version instead of latest - Old-layout files are deleted after successful restructuring Also documents the upgrade workflow in wren_project.md. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wren/tests/unit/test_context_cli.py (1)
237-283: Add one error-path CLI test forUpgradeError.Please add a case like “v3 project +
--to 2” (or unsupported target) and assert exit code 1 + error text. That will pin the failure contract too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/tests/unit/test_context_cli.py` around lines 237 - 283, Add a new unit test in tests/unit/test_context_cli.py that constructs a v3 project (write "schema_version: 3" into wren_project.yml), then calls runner.invoke(app, ["context", "upgrade", "--path", str(tmp_path), "--to", "2"]) and asserts it fails with exit_code == 1 and that result.output contains the upgrade error text (e.g., contains "UpgradeError" or a clear message like "cannot downgrade to version 2"); name the test something like test_upgrade_cli_error_downgrade_to_unsupported to mirror existing tests and ensure you reference runner.invoke and app so the CI catches the expected failure contract.wren/src/wren/context_cli.py (1)
476-486: Consolidate duplicate no-op checks inupgrade().Lines 476-486 perform two equivalent no-op checks. Keeping a single branch will reduce drift and keep the flow simpler.
♻️ Suggested simplification
- if ( - not result.files_created - and not result.files_deleted - and not result.files_modified - ): - typer.echo(f"Already at schema_version {current}. Nothing to do.") - return - - if result.from_version == result.to_version: + if result.from_version == result.to_version: typer.echo(f"Already at schema_version {current}. Nothing to do.") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/context_cli.py` around lines 476 - 486, In upgrade(), remove the duplicated no-op branches and replace them with a single check that handles both cases: if there are no file changes (result.files_created/ deleted/ modified) OR the schema version didn't change (result.from_version == result.to_version), then call typer.echo(f"Already at schema_version {current}. Nothing to do.") and return; update the branch that references result and current inside the upgrade() function so only this single consolidated condition remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren/src/wren/context_cli.py`:
- Around line 99-100: The init help text/docstring still references "v2 YAML
project" while the scaffold now emits "schema_version: 3"; update the init CLI
docstring (the init function/docstring in wren/src/wren/context_cli.py) to say
"v3 YAML project" or similar wording that reflects schema_version: 3, and ensure
any other mentions of "v2" in that docstring are changed to "v3" so the help
text matches the new default scaffold output.
In `@wren/tests/unit/test_context.py`:
- Around line 698-709: The test test_convert_mdl_v1_layout_version is brittle
because it accesses files[0].content; change it to build a map from the
convert_mdl_to_project() result keyed by each file's relative_path (as in
test_convert_mdl_preserves_dialect), then lookup the expected project file via
its relative_path and read its .content to load YAML and assert schema_version
== 2—this removes the positional dependency on files[0].
---
Nitpick comments:
In `@wren/src/wren/context_cli.py`:
- Around line 476-486: In upgrade(), remove the duplicated no-op branches and
replace them with a single check that handles both cases: if there are no file
changes (result.files_created/ deleted/ modified) OR the schema version didn't
change (result.from_version == result.to_version), then call
typer.echo(f"Already at schema_version {current}. Nothing to do.") and return;
update the branch that references result and current inside the upgrade()
function so only this single consolidated condition remains.
In `@wren/tests/unit/test_context_cli.py`:
- Around line 237-283: Add a new unit test in tests/unit/test_context_cli.py
that constructs a v3 project (write "schema_version: 3" into wren_project.yml),
then calls runner.invoke(app, ["context", "upgrade", "--path", str(tmp_path),
"--to", "2"]) and asserts it fails with exit_code == 1 and that result.output
contains the upgrade error text (e.g., contains "UpgradeError" or a clear
message like "cannot downgrade to version 2"); name the test something like
test_upgrade_cli_error_downgrade_to_unsupported to mirror existing tests and
ensure you reference runner.invoke and app so the CI catches the expected
failure contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3a22cfa-96e1-4e31-bf50-1639646dd0ff
📒 Files selected for processing (5)
docs/guide/modeling/wren_project.mdwren/src/wren/context.pywren/src/wren/context_cli.pywren/tests/unit/test_context.pywren/tests/unit/test_context_cli.py
✅ Files skipped from review due to trivial changes (1)
- docs/guide/modeling/wren_project.md
🚧 Files skipped from review as they are similar to previous changes (1)
- wren/src/wren/context.py
- Update init docstring to remove hardcoded "v2" reference - Use file_map lookup instead of positional files[0] in test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wren/src/wren/context_cli.py (1)
476-486: Prefer version-based no-op detection; file-list check is redundant and potentially misleading.Using empty
files_*lists as an “already upgraded” signal can mask legitimate version bumps if planning behavior evolves. Gate no-op onresult.from_version == result.to_versiononly.♻️ Suggested simplification
- if ( - not result.files_created - and not result.files_deleted - and not result.files_modified - ): - typer.echo(f"Already at schema_version {current}. Nothing to do.") - return - if result.from_version == result.to_version: typer.echo(f"Already at schema_version {current}. Nothing to do.") return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren/src/wren/context_cli.py` around lines 476 - 486, Remove the redundant file-list based no-op check and rely solely on the version comparison: delete the if-block that checks result.files_created / result.files_deleted / result.files_modified and keep the single no-op guard that checks result.from_version == result.to_version; ensure the message printed (typer.echo) remains correct (use the existing current or result.to_version as before) and avoid duplicate "Nothing to do." branches in the function (context_cli / surrounding upgrade/apply function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wren/src/wren/context_cli.py`:
- Around line 476-486: Remove the redundant file-list based no-op check and rely
solely on the version comparison: delete the if-block that checks
result.files_created / result.files_deleted / result.files_modified and keep the
single no-op guard that checks result.from_version == result.to_version; ensure
the message printed (typer.echo) remains correct (use the existing current or
result.to_version as before) and avoid duplicate "Nothing to do." branches in
the function (context_cli / surrounding upgrade/apply function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 639ef757-09c0-4bba-ad79-7c657ee579ce
📒 Files selected for processing (2)
wren/src/wren/context_cli.pywren/tests/unit/test_context.py
douenergy
approved these changes
Apr 16, 2026
This was referenced Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
layoutVersionto Manifest for schema feature gating — consumers can check the version to know which fields are available, and engines can reject manifests with versions they don't supportdialect: Option<DataSource>to Model and View, so each entity can declare what SQL dialect its embedded SQL is written in (overrides project-leveldata_sourcewhen set)migrate_manifest()utility for sequential version migration (v1→v2→…)convert_mdl_to_project()wren context buildnow includesdataSourcein the compiledtarget/mdl.json(it was previously dropped)Changes
Rust (wren-core-base, wren-core-py)
manifest-macro/src/lib.rslayout_version: u32on Manifest,dialect: Option<DataSource>on Model and Viewmanifest.rsMAX_SUPPORTED_LAYOUT_VERSION = 2,LayoutVersionError,validate_layout_version()migration.rsmigrate_manifest()with v1→v2 stepbuilder.rs.layout_version(),.dialect()builder methodspy_method.rslayout_versionPython getterwren-core-py/manifest.rsmigrate_manifest_json()pyfunctionPython (wren SDK)
context.py_LAYOUT_VERSION_MAP,_VALID_DIALECTS;build_manifest()includesdata_sourcefrom project config (previously omitted);build_json()stampslayoutVersion;convert_mdl_to_project()preservesdialectand mapslayoutVersion → schema_version;validate_project()validatesdialectvaluescontext_cli.pyschema_version: 3Documentation
wren_project.mdmodel.mddialectfield in Model Fields tableview.mddialectfield in View Fields tableBackward Compatibility
layoutVersiondefaults to1viaserde(default)— all existing JSON manifests work unchangeddialectdefaults toNone— all existing models/views work unchangeddialectisNone, behavior is identical to current (manifest-leveldata_sourcefallback)data_sourcefix is purely additive — output manifests now contain a previously-missing fieldTest plan
cargo clippyandruffclean on all modified files🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation