fix(core): ensure that the RLAC can be applied when the relationship access#1190
fix(core): ensure that the RLAC can be applied when the relationship access#1190douenergy merged 4 commits intoCanner:mainfrom
Conversation
WalkthroughThe changes refine the handling of required fields and partial model plans in the logical plan analysis, separating the collection of calculated and non-calculated fields for partial models. The control flow for building partial model nodes is unified, and error handling is improved. Test code is updated to use clearer manifest definitions and verifies row-level access control in more complex model relationships. The Rust workflow is also updated to set a stack size environment variable during tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModelPlanNodeBuilder
participant Analyzer
participant PartialModelCollector
User->>ModelPlanNodeBuilder: build()
ModelPlanNodeBuilder->>Analyzer: collect_partial_model_plan_for_calculation()
Analyzer-->>ModelPlanNodeBuilder: (calculated fields)
ModelPlanNodeBuilder->>PartialModelCollector: collect_partial_model_required_fields()
PartialModelCollector-->>ModelPlanNodeBuilder: (non-calculated fields)
ModelPlanNodeBuilder->>Analyzer: collect_model_required_fields()
Analyzer-->>ModelPlanNodeBuilder: (all required fields)
ModelPlanNodeBuilder-->>User: ModelPlanNode
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
wren-core/core/src/logical_plan/analyze/plan.rs (1)
547-553:⚠️ Potential issueAlias uses the parent column name – likely a correctness bug
Each nested calculated source column is currently aliased with the outer
qualified_column’s name:expr.alias(qualified_column.name.clone())If a top-level calculation depends on multiple other calculated fields, they will all share the same alias, producing name collisions in the logical plan and silently masking one another.
- expr.alias(qualified_column.name.clone()), + expr.alias(c.name.clone()), // c is the current required fieldPlease update the alias to the actual field being added and add a regression test that ensures distinct aliases are preserved.
🧹 Nitpick comments (4)
.github/workflows/rust.yml (1)
60-66: Consider definingRUST_MIN_STACKonce instead of repeating it in every test stepSetting the same environment variable four times makes the workflow noisier and harder to maintain.
You can DRY this up by placing the variable under anenv:block at the job (or evendefaults.run) level, e.g.:jobs: linux-test: env: RUST_MIN_STACK: 8388608 steps: …All test invocations will then inherit the value automatically.
Also applies to: 78-82, 90-96, 104-107
wren-core/core/src/mdl/mod.rs (1)
2080-2180: Large inline manifests/tests are becoming unwieldy – factor out helpersThe newly-added RLAC tests reproduce a full manifest (≈ 80 lines) inline.
As these grow they hurt readability and make future changes harder.Suggestion: extract manifest construction into a helper like
fn build_rlac_manifest() -> Manifest { … }intests/utils.rs, then keep each test focused on assertions:let analyzed_mdl = Arc::new(AnalyzedWrenMDL::analyze(build_rlac_manifest())?);This keeps the test body < 20 lines and removes duplication across similar scenarios.
wren-core/core/src/logical_plan/analyze/plan.rs (2)
448-461: Duplicate logic increate_partial_calculation()The same pair of
collect_*helpers is called here right after the earlier call insidebuild().
Becausepartial_model_required_fieldsstarts empty at this point, the first call already seeds it with all relevant entries; the second call therefore repeats the work and, in the worst case, inserts duplicate aliases.Refactor to a single call (or a small helper that internally decides which branch is needed) to keep the function side-effect-free and O(n) instead of O(2n).
559-562: Typos in documentationSmall spelling issues make the new comments harder to read:
- “throguh” → “through”
- “he partial model” → “the partial model”
Fixing them helps future maintainers.
-/// required fields for the partial model used by another model throguh the relationship. -/// It collects the non-calculated fields for the he partial model used by another model. +/// Required fields for the partial model used by another model **through** the relationship. +/// It collects the non-calculated fields for the **partial** model used by another model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/rust.yml(4 hunks)wren-core/core/src/logical_plan/analyze/plan.rs(6 hunks)wren-core/core/src/logical_plan/analyze/relation_chain.rs(1 hunks)wren-core/core/src/mdl/mod.rs(3 hunks)
🔇 Additional comments (1)
wren-core/core/src/logical_plan/analyze/plan.rs (1)
199-224:Details
❌ Incorrect review comment
Potentially redundant triple-collection of required fields
Within the
elsebranch you now invoke three collectors back-to-back (collect_partial_model_plan_for_calculation,collect_partial_model_required_fields, andcollect_model_required_fields).
All three eventually populate the samemodel_required_fieldsmap.
Besides the additional hash look-ups this may generate identicalOrdExprinstances differing only by aliasing, leading to bloated sets and unexpected duplicates later on (especially becausecollect_model_required_fieldsalready covers both calculated and non-calculated columns).Consider:
-collect_partial_model_plan_for_calculation(...)?; -collect_partial_model_required_fields(...)?; -collect_model_required_fields(...)?; +collect_model_required_fields(...)?; // one pass is enough +// If you really need the separation, deduplicate the returned sets before merging.Stream-lining to a single authoritative collector will simplify reasoning and avoid latent ordering / precedence bugs.
🏁 Script executed:
#!/bin/bash set -e echo "Searching for collect_partial_model_plan_for_calculation:" rg "fn collect_partial_model_plan_for_calculation" -n -A20 echo echo "Searching for collect_partial_model_required_fields:" rg "fn collect_partial_model_required_fields" -n -A20 echo echo "Searching for collect_model_required_fields:" rg "fn collect_model_required_fields" -n -A20Length of output: 6086
🏁 Script executed:
#!/bin/bash set -e echo "=== collect_partial_model_plan_for_calculation body ===" sed -n '517,560p' wren-core/core/src/logical_plan/analyze/plan.rs || true echo echo "=== collect_partial_model_required_fields body ===" sed -n '561,601p' wren-core/core/src/logical_plan/analyze/plan.rs || true echo echo "=== collect_model_required_fields body ===" sed -n '602,650p' wren-core/core/src/logical_plan/analyze/plan.rs || trueLength of output: 5633
Ignore redundant-collector suggestion
The three “collector” functions serve distinct purposes and even produce different aliases, so they are not interchangeable:
- collect_partial_model_plan_for_calculation
• Only whencolumn.is_calculatedand only for building the calculation plan
• Aliases each expression to the qualified_column name- collect_partial_model_required_fields
• Only when!column.is_calculatedand only for building related (partial) models- collect_model_required_fields
• Covers both calculated and non-calculated fields, but for the source model plan
• Uses the column’s own name for aliasing and includes extra debug logic and dataset checksMerging these into a single pass would lose the clear separation of calculation‐vs-related-model logic and break the aliasing semantics each downstream planner relies on. Leave the three calls as is.
Likely an incorrect or invalid review comment.
| rust-version: stable | ||
| - name: Run tests (excluding doctests) | ||
| run: cargo test --lib --tests --bins | ||
| run: RUST_MIN_STACK=8388608 cargo test --lib --tests --bins |
There was a problem hiding this comment.
To avoid the following test error, add the min stack size of rust:
thread 'mdl::test::test_plan_calculation_without_unnamed_subquery' has overflowed its stack.
|
Thanks @goldmedal |
2 participants
Description
This PR fixes the issue that the RLAC won't be applied when the relationship accesses the model. For example:
orderswith the columncustomer_namewhich points to the columnnamein another modelcustomerthrough the relationship.customerhas the RLAC rules, the SQL trying to queryorders.customer_nameshould also apply the RLAC rules for the modelcustomer.Changed
Because we apply the RLAC in
ModelPlanNode, we can reuse the logic for the relationship accessing. Previously, we built the to-one relationship access throughModelSourceNodethat won't be applied RLAC.This PR will build
PartialModelPlanNodeinstead ofModelSourceNodefor the relationship accessing to ensure the RLAC can be used.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests
Chores