fix(core): allow the RLAC condition invoke the hidden columns#1330
fix(core): allow the RLAC condition invoke the hidden columns#1330douenergy merged 1 commit intoCanner:mainfrom
Conversation
WalkthroughAdds visibility-aware APIs for column access. Model::get_physical_columns now takes a show_visible_only flag. New Model::get_visible_column is introduced; get_column now returns from all columns. Dataset::to_qualified_schema now accepts a visibility flag. Call sites updated across analysis, planning, and utils. New RLAC test validates hidden column behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Q as Query
participant A as Analyzer
participant DS as Dataset::Model
participant M as Model
Q->>A: Analyze SQL
A->>DS: to_qualified_schema(show_visible_only=true)
DS->>M: get_physical_columns(true)
M-->>DS: Visible non-relationship columns
DS-->>A: DFSchema (visible only)
A-->>Q: Planned query (hidden cols excluded)
sequenceDiagram
autonumber
participant A as Analyzer
participant AC as AccessControl
participant DS as Dataset::Model
participant M as Model
A->>AC: build_filter_expression
AC->>DS: to_qualified_schema(false)
DS->>M: get_physical_columns(false)
M-->>DS: All non-relationship columns
DS-->>AC: DFSchema (includes hidden)
AC-->>A: RLAC filter expression
note over A: Final projection still uses visible columns
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
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/logical_plan/analyze/access_control.rs (1)
344-351: Don’t block calculated-field validation on column visibility.Using
get_visible_columnwill fail when a calculated field depends on a hidden column (which is a supported pattern elsewhere), causing false “Column not found” errors. Useget_columninstead to validate CLAC recursively regardless of visibility.- let Some(ref_column) = ref_model.get_visible_column(field.name()) else { + let Some(ref_column) = ref_model.get_column(field.name()) else { return plan_err!( "Column {}.{} not found", model_name.table(), field.name() ); };
🧹 Nitpick comments (3)
wren-core/core/src/mdl/mod.rs (1)
2647-2694: Fix test name typo and normalize SQL casing.
- Rename ralc → rlac in the test name for consistency.
- Prefer consistent "FROM" casing in SQL literals.
Apply this diff:
- async fn test_ralc_condition_contain_hidden() -> Result<()> { + async fn test_rlac_condition_contains_hidden() -> Result<()> { @@ - let sql = "SELECT c_name FROm customer"; + let sql = "SELECT c_name FROM customer";wren-core/core/src/mdl/dataset.rs (1)
31-44: API change looks good; consider clarifying param intent.Signature change and usage are correct. Consider renaming
show_visible_onlytovisible_onlyand adding a brief rustdoc to avoid ambiguity.wren-core/core/src/logical_plan/analyze/access_control.rs (1)
201-206: Typo in comment.Fix “shcmea” → “schema”.
- // The condition could contains the hidden columns, so we need to build the shcmea with hidden columns + // The condition may contain hidden columns, so we need to build the schema with hidden columns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
wren-core-base/src/mdl/manifest.rs(2 hunks)wren-core/core/src/logical_plan/analyze/access_control.rs(3 hunks)wren-core/core/src/logical_plan/analyze/plan.rs(5 hunks)wren-core/core/src/mdl/context.rs(1 hunks)wren-core/core/src/mdl/dataset.rs(2 hunks)wren-core/core/src/mdl/mod.rs(1 hunks)wren-core/core/src/mdl/utils.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
wren-core/core/src/mdl/dataset.rs (2)
wren-core/core/src/mdl/context.rs (1)
model(342-363)wren-core-base/manifest-macro/src/lib.rs (1)
model(119-154)
wren-core/core/src/logical_plan/analyze/plan.rs (3)
wren-core/core/src/mdl/context.rs (1)
model(342-363)wren-core/core/src/mdl/dataset.rs (2)
model(34-38)model(62-67)wren-core/core/src/mdl/mod.rs (1)
model(201-219)
wren-core/core/src/logical_plan/analyze/access_control.rs (3)
wren-core/core/src/mdl/context.rs (1)
model(342-363)wren-core/core/src/mdl/dataset.rs (2)
model(34-38)model(62-67)wren-core/core/src/mdl/mod.rs (1)
model(201-219)
wren-core/core/src/mdl/mod.rs (2)
wren-core/core/src/logical_plan/analyze/plan.rs (6)
new(72-85)new(112-127)new(783-785)new(900-994)new(1048-1097)new(1156-1158)wren-core/core/src/mdl/context.rs (1)
new(336-366)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cargo check
- GitHub Check: ci
- GitHub Check: test
🔇 Additional comments (13)
wren-core/core/src/mdl/dataset.rs (2)
62-77: Remote schema should exclude hidden top-level columns (LGTM).Using
get_physical_columns(true)and expanding only expression sources is correct to prevent exposing hidden columns while still resolving expressions.
31-44: All call-sites updated to the new signature. Repository scan shows only calls that pass a boolean (e.g. wren-core/core/src/mdl/utils.rs, logical_plan/analyze/access_control.rs); no stale to_qualified_schema() calls remain.wren-core/core/src/logical_plan/analyze/access_control.rs (1)
257-260: Early return for empty required properties (LGTM).This short‑circuit matches the documented behavior and avoids unnecessary work.
wren-core/core/src/mdl/utils.rs (2)
133-136: Visible-only schema choice (LGTM).Using
to_qualified_schema(true)here is appropriate since calculated-field expr qualification should not expose hidden columns.
166-172: Visible-only schema for model exprs (LGTM).Consistent with visibility semantics and prevents accidental resolution of hidden columns via unqualified names.
wren-core-base/src/mdl/manifest.rs (2)
252-265: Visibility-aware physical columns (LGTM).The split between visible-only and all physical columns is correct and keeps relationship columns excluded.
278-289: Accessor split is sound (LGTM).Providing both
get_visible_columnandget_columncovers callers that need visibility enforcement vs. internal resolution.wren-core/core/src/mdl/context.rs (1)
343-366: Expose only visible columns in table provider schema (LGTM).Restricting WrenDataSource columns to visible ones while still validating CLAC is the right balance for preventing leaks.
wren-core/core/src/logical_plan/analyze/plan.rs (5)
145-153: Include hidden columns when required by rules (LGTM).Using
get_physical_columns(false)ensures RLAC-required hidden fields are available to build filters without exposing them directly.
241-244: Primary key lookup should honor visibility (LGTM).
get_visible_column(pk)ensures we don’t join on a hidden PK inadvertently.
924-944: Wildcard expansion against all physical columns (LGTM).Collecting from
get_physical_columns(false)then filtering out calculated fields is consistent with making hidden fields available only when explicitly required downstream.
946-952: Direct field resolution against all physical columns (LGTM).This allows hidden fields referenced explicitly in required exprs (e.g., RLAC) to be found while later stages handle visibility of outputs.
1057-1062: Consistent PK visibility in CalculationPlanNode (LGTM).Using
get_visible_columnavoids surfacing hidden keys in outputs.
2 participants
Description
isHiddenis used to avoid a column from being used by SQL directly. However, the column should be available to used by internal usage (e.g.expression,rlac,clac, ...). This PR fixed the issue about rlac using a hidden column.Summary by CodeRabbit