This repository was archived by the owner on May 7, 2026. It is now read-only.
feat(wren-core-base): replace Metric with Cube types and remove deprecated security types - #1574
Merged
Conversation
Add Cube, Measure, CubeDimension, TimeDimension types to the MDL manifest with full support for serde, PyO3, and WASM targets. This includes proc macros, builders, PyO3 getters, and a new `cubes` field on Manifest. Remove the unused Metric type (along with TimeGrain and TimeUnit) and its associated macro, builder, and references across wren-core and wren-core-py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
core
rust
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors the manifest architecture from a metric-centric to a cube-centric model. The Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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 docstrings
🧪 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren-core-py/src/extractor.rs (1)
82-90:⚠️ Potential issue | 🟠 MajorPreserve cubes when extracting a manifest.
extract_by()now always returnscubes: vec![], so any cube-enabled manifest loses all cube definitions after extraction. At minimum this should carry the existing cubes through unchanged until cube-aware filtering is added.💡 Minimal safe fix
Ok(Manifest { layout_version: mdl.manifest.layout_version, catalog: mdl.catalog().to_string(), schema: mdl.schema().to_string(), models: used_models, relationships: used_relationships, views: used_views, data_source: mdl.data_source(), - cubes: vec![], + cubes: mdl.manifest.cubes.clone(), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-py/src/extractor.rs` around lines 82 - 90, The manifest extractor is discarding cube definitions by hardcoding cubes: vec![]; update the construction in extract_by (where Ok(Manifest { ... }) is returned) to carry the existing cubes from the source model (use the mdl's cubes accessor or clone the cubes field) so the returned Manifest preserves mdl's cubes (e.g., replace cubes: vec![] with the appropriate mdl.cubes() cloning/into conversion). Ensure you use the correct accessor/method on mdl to obtain and clone/copy the cube collection so types match Manifest::cubes.
🧹 Nitpick comments (2)
wren-core-py/src/manifest.rs (1)
63-97: Exercise a non-empty cube in this round-trip test.With
cubes: vec![], this only validates the empty/default path. A broken serde or Python binding forCube/Measure/CubeDimension/TimeDimensionwould still pass here, which leaves the new surface from this PR untested.🤖 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 63 - 97, The test's py_manifest uses Manifest with cubes: vec![] which doesn't exercise Cube/Measure/CubeDimension/TimeDimension serde or Python bindings; update the manifest construction in this test to include at least one non-empty Cube in the cubes field (create a Cube instance with a name, one or more Measure entries, and one or more CubeDimension/TimeDimension entries, referencing an existing model name like "model_1" or a table reference as appropriate) so the round-trip validates serialization/deserialization and bindings for Cube, Measure, CubeDimension, and TimeDimension; ensure you use the same struct types used elsewhere in this file (Manifest, Cube, Measure, CubeDimension, TimeDimension) and wrap the Cube in the Vec passed to cubes.wren-core-base/src/mdl/builder.rs (1)
661-707: Consider adding roundtrip tests for the new Cube-related builders.The PR test plan mentions "JSON serialization round-trip for Cube/Measure/CubeDimension/TimeDimension", but this file only tests
ViewBuilder. Consider adding roundtrip tests forMeasureBuilder,CubeDimensionBuilder,TimeDimensionBuilder, andCubeBuilderto verify serialization correctness, similar to the existingtest_view_roundtripandtest_manifest_roundtriptests.Would you like me to generate roundtrip tests for the new cube-related builders, or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core-base/src/mdl/builder.rs` around lines 661 - 707, Add JSON roundtrip tests for the cube-related builders by creating new unit tests analogous to test_view_roundtrip/test_manifest_roundtrip that build instances with sample data, serialize to JSON with serde_json::to_string, deserialize back with serde_json::from_str, and assert equality; specifically add tests that exercise MeasureBuilder (e.g., a simple measure with name/expression/type), CubeDimensionBuilder, TimeDimensionBuilder, and CubeBuilder (a cube that references measures/dimensions), using the same equality pattern (e.g., let expected = MeasureBuilder::new(...).build(); let json_str = serde_json::to_string(&expected).unwrap(); let actual: Arc<Measure> = serde_json::from_str(&json_str).unwrap(); assert_eq!(actual, expected)) and include any necessary Arc<> types and module paths to match how View and Manifest tests are written.
🤖 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-core/core/src/logical_plan/analyze/relation_chain.rs`:
- Around line 48-58: The code currently irrefutably destructures Dataset::Model
when building the relation chain (calling ModelSourceNode::new), which prevents
other plannable dataset types like Cube from being used; change the destructure
to a match on dataset and add a Dataset::Cube arm that constructs the
appropriate cube-aware relation node (e.g., a CubeSourceNode or by delegating to
the cube planning entry point) while keeping the existing Dataset::Model arm
using ModelSourceNode::new; for any unsupported variants return a descriptive
error. Apply the same pattern to the other occurrence (the block referenced at
88-121) so both relation-chain construction sites handle Cube and Model variants
instead of hard-coding Dataset::Model.
---
Outside diff comments:
In `@wren-core-py/src/extractor.rs`:
- Around line 82-90: The manifest extractor is discarding cube definitions by
hardcoding cubes: vec![]; update the construction in extract_by (where
Ok(Manifest { ... }) is returned) to carry the existing cubes from the source
model (use the mdl's cubes accessor or clone the cubes field) so the returned
Manifest preserves mdl's cubes (e.g., replace cubes: vec![] with the appropriate
mdl.cubes() cloning/into conversion). Ensure you use the correct accessor/method
on mdl to obtain and clone/copy the cube collection so types match
Manifest::cubes.
---
Nitpick comments:
In `@wren-core-base/src/mdl/builder.rs`:
- Around line 661-707: Add JSON roundtrip tests for the cube-related builders by
creating new unit tests analogous to test_view_roundtrip/test_manifest_roundtrip
that build instances with sample data, serialize to JSON with
serde_json::to_string, deserialize back with serde_json::from_str, and assert
equality; specifically add tests that exercise MeasureBuilder (e.g., a simple
measure with name/expression/type), CubeDimensionBuilder, TimeDimensionBuilder,
and CubeBuilder (a cube that references measures/dimensions), using the same
equality pattern (e.g., let expected = MeasureBuilder::new(...).build(); let
json_str = serde_json::to_string(&expected).unwrap(); let actual: Arc<Measure> =
serde_json::from_str(&json_str).unwrap(); assert_eq!(actual, expected)) and
include any necessary Arc<> types and module paths to match how View and
Manifest tests are written.
In `@wren-core-py/src/manifest.rs`:
- Around line 63-97: The test's py_manifest uses Manifest with cubes: vec![]
which doesn't exercise Cube/Measure/CubeDimension/TimeDimension serde or Python
bindings; update the manifest construction in this test to include at least one
non-empty Cube in the cubes field (create a Cube instance with a name, one or
more Measure entries, and one or more CubeDimension/TimeDimension entries,
referencing an existing model name like "model_1" or a table reference as
appropriate) so the round-trip validates serialization/deserialization and
bindings for Cube, Measure, CubeDimension, and TimeDimension; ensure you use the
same struct types used elsewhere in this file (Manifest, Cube, Measure,
CubeDimension, TimeDimension) and wrap the Cube in the Vec passed to cubes.
🪄 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: c2579d6e-ddd0-43eb-9c98-3f906b914707
📒 Files selected for processing (11)
wren-core-base/manifest-macro/src/lib.rswren-core-base/src/mdl/builder.rswren-core-base/src/mdl/manifest.rswren-core-base/src/mdl/py_method.rswren-core-py/src/extractor.rswren-core-py/src/manifest.rswren-core/core/src/logical_plan/analyze/plan.rswren-core/core/src/logical_plan/analyze/relation_chain.rswren-core/core/src/mdl/dataset.rswren-core/core/src/mdl/lineage.rswren-core/core/src/mdl/mod.rs
💤 Files with no reviewable changes (1)
- wren-core/core/src/mdl/lineage.rs
…mnLevelSecurity types Remove the legacy deprecated types (RowLevelSecurity, RowLevelOperator, ColumnLevelSecurity) and the rls/cls fields from Column. The current access control system (RowLevelAccessControl, ColumnLevelAccessControl) remains untouched. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, add roundtrip tests - Preserve cubes in manifest extractor instead of discarding them - Add JSON roundtrip tests for Measure, CubeDimension, TimeDimension, Cube builders - Add non-empty Cube to wren-core-py manifest roundtrip test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
goldmedal
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren-core/core/src/mdl/mod.rs (1)
169-223:⚠️ Potential issue | 🟠 MajorCubes are deserialized but never wired into the planning context.
While
Manifest.cubescan be populated via the builder (in wren-core-base), theWrenMDLstruct exposes no accessor for cubes and the registration path at lines 169–223 only iteratesmdl.models(). Cubes have noDatasetvariant, no qualified reference indexing, and no integration into the planning or analysis pipeline. If cubes are intentionally not queryable yet, reject them early in manifest validation with a clear error; otherwise, implement cube accessors, aDataset::Cubevariant, and register cube fields analogously to how models are handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-core/core/src/mdl/mod.rs` around lines 169 - 223, The manifest currently ignores Manifest.cubes because WrenMDL exposes no accessor and the registration loop only iterates mdl.models(), so either reject cubes during manifest validation or wire them into planning: add an accessor on WrenMDL for cubes, extend the Dataset enum with a Cube variant (Dataset::Cube), and augment the registration mapping (the code that currently calls mdl.models() and constructs WrenDataSource) to also iterate mdl.cubes()—for each cube, derive fields similarly (reuse infer_source_column / to_field logic or a cube-specific mapper), build an Arrow Schema, create a WrenDataSource, and register it under the cube's qualified name; if you choose to reject cubes instead, add a clear validation error when Manifest.cubes is non-empty during MDL validation.
🤖 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-core-base/src/mdl/builder.rs`:
- Around line 334-438: The new builders (MeasureBuilder::new,
CubeDimensionBuilder::new, TimeDimensionBuilder::new, CubeBuilder::new and its
methods measure/dimension/time_dimension/hierarchy) and the Cube.hierarchies
serde surface aren't covered by tests; add round-trip serde tests that create
Measure, CubeDimension, TimeDimension via their builders, assemble a Cube with
measures/dimensions/time_dimensions and a hierarchy (including multiple levels
to exercise ordering), serialize to the manifest JSON/struct, deserialize back,
and assert equality (or equivalent field-wise checks) to ensure builders,
hierarchies ordering and serde round-trip work correctly.
In `@wren-core-base/src/mdl/cls.rs`:
- Around line 122-131: ColumnLevelAccessControl::eval lost the
numeric/string/mismatched-type branches that validate_clac_rule relies on to
decide column visibility; restore those branches in
ColumnLevelAccessControl::eval so it evaluates numeric comparisons, string
comparisons, and explicitly handles type-mismatch cases the same way as before
(returning the prior allow/deny/unknown semantics), and add unit tests
exercising the numeric, string and mismatched-type scenarios referenced by
validate_clac_rule and clac.eval to prevent regressions. Ensure you update the
eval implementation (ColumnLevelAccessControl::eval) to reintroduce the specific
match arms for numeric vs numeric, string vs string, and a fallback for
mismatched types, and write tests that mirror the permission decisions expected
by validate_clac_rule and the analyzer logic.
---
Outside diff comments:
In `@wren-core/core/src/mdl/mod.rs`:
- Around line 169-223: The manifest currently ignores Manifest.cubes because
WrenMDL exposes no accessor and the registration loop only iterates
mdl.models(), so either reject cubes during manifest validation or wire them
into planning: add an accessor on WrenMDL for cubes, extend the Dataset enum
with a Cube variant (Dataset::Cube), and augment the registration mapping (the
code that currently calls mdl.models() and constructs WrenDataSource) to also
iterate mdl.cubes()—for each cube, derive fields similarly (reuse
infer_source_column / to_field logic or a cube-specific mapper), build an Arrow
Schema, create a WrenDataSource, and register it under the cube's qualified
name; if you choose to reject cubes instead, add a clear validation error when
Manifest.cubes is non-empty during MDL validation.
🪄 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: baee1b9b-c0dc-43f4-8336-b4e0dff9f539
📒 Files selected for processing (10)
wren-core-base/manifest-macro/src/lib.rswren-core-base/src/mdl/builder.rswren-core-base/src/mdl/cls.rswren-core-base/src/mdl/manifest.rswren-core-base/tests/data/mdl.jsonwren-core/core/src/logical_plan/utils.rswren-core/core/src/mdl/dataset.rswren-core/core/src/mdl/dialect/inner_dialect.rswren-core/core/src/mdl/function/remote_function.rswren-core/core/src/mdl/mod.rs
💤 Files with no reviewable changes (1)
- wren-core-base/tests/data/mdl.json
✅ Files skipped from review due to trivial changes (3)
- wren-core/core/src/logical_plan/utils.rs
- wren-core/core/src/mdl/dialect/inner_dialect.rs
- wren-core/core/src/mdl/function/remote_function.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-core-base/src/mdl/manifest.rs
… numeric, string, and type-mismatch cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-core-base/src/mdl/cls.rs`:
- Around line 143-150: The tests and clac evaluation currently assume numeric
strings and panic when parsing fails; update NormalizedExpr::new and the
comparison functions used by clac (e.g., gt/lt inside eval) to treat malformed
numeric input as non-numeric or as a parse failure and return false (deny)
instead of unwrapping/propagating a panic; specifically, ensure NormalizedExpr
only marks a value numeric when f64::parse succeeds and modify the numeric
comparison code paths invoked by clac(ColumnLevelOperator::GreaterThan/ LessThan
/ GreaterThanOrEquals / LessThanOrEquals) to detect parse errors and return
false rather than calling unwrap.
🪄 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: f1fd975a-dbc4-40d3-8842-06bf5658bf66
📒 Files selected for processing (1)
wren-core-base/src/mdl/cls.rs
douenergy
approved these changes
Apr 21, 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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Metrictype with a first-classCubetype in the MDL manifest, composed ofMeasure,CubeDimension, andTimeDimensionsub-types with support for ordered drill-downhierarchies(BTreeMap)TimeGrain,TimeUnit, and allMetric-related builders, tests, and references from wren-core-base, wren-core, and wren-core-pyDatasetenum toModel-only, removingMetricvariant and converting match arms to irrefutableletpatterns in plan analysis and relation chain code#[pymethods]) forCube,Measure,CubeDimension, andTimeDimensionwith getter accessorsRowLevelSecurity,RowLevelOperator,ColumnLevelSecuritytypes and therls/clsfields fromColumn(current access control systemRowLevelAccessControl/ColumnLevelAccessControlremains untouched)Test plan
cargo testpasses in wren-core-base (37 tests including new cube roundtrip tests)cargo clippy --all-targets --all-features -- -D warningscleancargo fmt --all --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor