fix(wren-core): resolve relationship handles by the model they point at - #2661
fix(wren-core): resolve relationship handles by the model they point at#2661wilyan09007 wants to merge 1 commit into
Conversation
A relationship column is a handle whose name is local to the model that
declares it, so `customer` on `orders` may point at the model `customers`.
`create_wren_calculated_field_expr` trimmed a relationship path down to its
last two identifiers, which left the handle name as the qualifier. It then
planned the expression against a schema built from the lineage's required
fields, and that schema is qualified by model name. The two agree only when
the handle happens to be named after its model, so a calculated column or a
cube dimension that used any other alias failed to plan:
column 'name' not found in 'customer'
possible column customers.name
Walk the path through the MDL instead, resolving each handle to its related
model the way the lineage does, and qualify the rewritten identifier with
that model. Paths that traverse no relationship are left as they were.
WalkthroughCalculated-field rewriting now resolves relationship handles to their destination model names before planning. New regression tests verify the generated join and projected SQL when a local relationship name differs from the target model name. ChangesRelationship path resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
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/utils.rs`:
- Around line 245-249: Restrict the model-prefix skip in the path-resolution
loop to the first component only: skip path[0] when it matches base_model, while
resolving every subsequent identifier as a relationship handle. Update the logic
around the relation/path traversal in the relevant utility function and add a
multi-hop regression test covering a repeated model name such as
orders.customer.customers.name.
🪄 Autofix
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 Plus
Run ID: 55d8d627-a30d-4275-9bd5-ffdef3c0f4be
📒 Files selected for processing (2)
core/wren-core/core/src/mdl/mod.rscore/wren-core/core/src/mdl/utils.rs
| for ident in path { | ||
| // the path may be written starting from the model owning the calculated field | ||
| if ident.value == relation { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict the model-prefix skip to the first path component.
Lines 247-249 skip an identifier whenever it equals the current destination model. Only the optional initial base-model qualifier has this meaning.
For example, if orders.customer resolves to customers, and customers has a relationship handle also named customers, orders.customer.customers.name returns customers.name instead of resolving the second relationship. Skip only path[0] when it equals base_model, then resolve every later identifier as a relationship handle. Add a multi-hop regression test for this case.
Proposed fix
- for ident in path {
- // the path may be written starting from the model owning the calculated field
- if ident.value == relation {
+ for (index, ident) in path.iter().enumerate() {
+ // Only the first component can be the explicit base-model qualifier.
+ if index == 0 && ident.value == base_model {
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for ident in path { | |
| // the path may be written starting from the model owning the calculated field | |
| if ident.value == relation { | |
| continue; | |
| } | |
| for (index, ident) in path.iter().enumerate() { | |
| // Only the first component can be the explicit base-model qualifier. | |
| if index == 0 && ident.value == base_model { | |
| continue; | |
| } |
🤖 Prompt for 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.
In `@core/wren-core/core/src/mdl/utils.rs` around lines 245 - 249, Restrict the
model-prefix skip in the path-resolution loop to the first component only: skip
path[0] when it matches base_model, while resolving every subsequent identifier
as a relationship handle. Update the logic around the relation/path traversal in
the relevant utility function and add a multi-hop regression test covering a
repeated model name such as orders.customer.customers.name.
1 participant
Summary
A calculated column or cube dimension whose expression traverses a relationship fails to plan
when the relationship handle is not named after the model it points at. Given a handle
customeronorderspointing at the modelcustomers:an expression of
customer.namefails:The only workaround is to rename the handle to
customers, which forces every relationshipalias to repeat the name of its target model.
docs/core/reference/mdl.mddocuments thesingular handle as valid, so this is a gap between the documented model and the engine.
Refs #2658. That issue reports two problems and this addresses one of them; see the last
section, which is why it does not carry a closing keyword.
Root cause
create_wren_calculated_field_expr(core/wren-core/core/src/mdl/utils.rs) plans theexpression against a schema assembled from the lineage's
required_fields_map. The lineageresolves a relationship path correctly: it follows
Column::relationshipthrough to therelated model, so every qualifier in that schema is a model name.
The expression itself was rewritten separately, by trimming any compound identifier to its
last two identifiers. That drops the leading path but keeps the handle name as the
qualifier. The expression therefore asks for
customer.namewhile the schema offerscustomers.name.The two names coincide whenever a handle is named after the model it points at, which is the
case in every fixture in the repo (
core/wren-core/core/tests/data/mdl.jsondeclares handlecustomerof typecustomer, andcore/wren-core-py/tests/test_modeling_core.pydeclareshandle
ordersof typeorders), so no existing test exercised a differing name.Fix
resolve_relationship_pathwalks the identifier path through the MDL, resolving each handlewith
get_column_referenceand advancing to the related model the way the lineage does, thenqualifies the rewritten identifier with the model the path lands on. An identifier that
traverses no relationship is returned untouched, and the previous trimming remains as the
fallback for any path the walk cannot resolve, so nothing that planned before changes shape.
Tests
test_create_wren_expr_handle_name_differs_from_model(mdl/utils.rs) callscreate_wren_calculated_field_expragainst a manifest whose handlecustomerpoints at themodel
customers, and asserts the built expression iscustomers.name.test_calculated_column_with_aliased_relationship_handle(mdl/mod.rs) putsSELECT order_id, customer_name FROM ordersthroughtransform_sql_with_ctxand snapshotsthe generated SQL, covering the join end to end.
Both tests fail without the change and pass with it.
cargo test --libis 151 passed / 0failed;
cargo fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warningsare clean.Not addressed: relationship traversal in user SQL
#2658 also reports that
SELECT orders.customer.name FROM ordersfails, and proposes thatnaming the handle after the model fixes it. That is a separate gap, and this PR does not
touch it.
Relationship traversal in a user query fails regardless of the handle name, because
relationship columns are filtered out of the schema a model registers with DataFusion
(
Model::get_physical_columnskeeps only columns whererelationship.is_none()), so the pathhas nothing to resolve against. Checked against
wren-core0.7.3 using this repo's own testmanifest, where the handle
ordersalready matches the modelorders:The same query with a handle renamed to match its model fails identically, so renaming is not
a workaround there.
docs/core/reference/mdl.mdcurrently states thatorders.customer.first_nameis valid SQL, so either the engine or that page needs a follow-up. Glad to open a separate
issue for it if that is the preference.
Summary by CodeRabbit