feat(wren-core): validate cubes in AnalyzedWrenMDL::analyze#2264
Conversation
Reject manifests whose cubes reference an unknown baseObject, have circular derived-measure expressions, or list a hierarchy level that isn't a defined dimension or time_dimension. Validation runs from all three analyze entry points so any caller path catches the error before lineage is built. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds cube-level validation to MDL analysis: a new ChangesCube-level validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/wren-core/core/src/mdl/lineage.rs`:
- Around line 355-361: The current code skips adding edges for self-referential
identifiers (ident.name.as_str() != measure.name), which lets self-loops bypass
is_dag() detection; either remove the != measure.name condition so
graph.add_edge(from, to, ()) will add self-loop edges and let is_dag() catch
them, or add an explicit validation before this loop that checks if
ident.name.as_str() == measure.name and returns/reports an error for the measure
(e.g., using the same error path the caller expects); locate the check using the
symbols measure.name, ident.name, measure_names, node_map and
graph.add_edge/is_dag() to implement the chosen fix.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18e3a73c-e967-4d10-9569-1348067261e9
📒 Files selected for processing (2)
core/wren-core/core/src/mdl/lineage.rscore/wren-core/core/src/mdl/mod.rs
Dropping the != measure.name guard lets is_dag see self-loops, so a measure defined in terms of itself (e.g. revenue = revenue * 1.1) is now rejected instead of silently passing. Adds a regression test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
baseObjectdoesn't resolve to a known Model or View.petgraphcycle detection on measure expressions.validate_cubesinto all threeAnalyzedWrenMDLentry points (analyze,analyze_with_tables,analyze_with_url_tables) so every caller path is covered.Test plan
RUST_MIN_STACK=8388608 cargo test --lib --tests --bins— 83 tests pass (4 new + 79 prior)cargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— clean🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests