fix(core): add quotes for the model subquery alias#1283
fix(core): add quotes for the model subquery alias#1283douenergy merged 2 commits intoCanner:mainfrom
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (3)
WalkthroughUpdated a lifetime annotation on quote_identifier, adjusted RLAC filter reduction and alias quoting in model generation, added a Unicode-focused RLAC unit test, and replaced a test clone with a borrowed reference in function tests. No public API semantics changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Analyzer
participant ModelGen
participant Quoter as IdentifierQuoter
participant Planner
Analyzer->>ModelGen: analyze(manifest, Unparse)
ModelGen->>ModelGen: reduce RLAC filters (pattern match)
ModelGen->>Quoter: quoted(model_plan.plan_name())
Quoter-->>ModelGen: quoted alias
ModelGen->>Planner: build plan with alias + RLAC
Planner-->>Analyzer: transformed plan/SQL
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit can generate a title for your PR based on the changes.Add @coderabbitai placeholder anywhere in the title of your PR and CodeRabbit will replace it with a title based on the changes in the PR. You can change the placeholder by changing the ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (2)
wren-core/core/src/mdl/mod.rs (1)
2526-2564: Great addition: Unicode RLAC coverage with quoted model aliasThis test validates the intended behavior: model subquery alias is quoted and Unicode identifiers are preserved and qualified in the RLAC filter. Nicely scoped and aligned with the PR goal.
Consider adding a companion test where the model name contains a double quote to ensure escaping works as expected end-to-end. For example, a model named VTU"藝人 and asserting the final SQL remains valid after quoting and escaping.
wren-core/core/src/logical_plan/analyze/model_generation.rs (1)
100-111: Simplify filter reduction for clarity and fewer branchesCurrent reduction nests two if-let blocks. You can achieve the same behavior more readably by flattening first and then reducing Expr directly.
Apply this diff:
- let rls_filter = filters - .into_iter() - .reduce(|acc, filter| { - if let Some(acc) = acc { - if let Some(filter) = filter { - Some(acc.and(filter)) - } else { - Some(acc) - } - } else { - filter - } - }) - .flatten(); + let rls_filter = filters + .into_iter() + .flatten() + .reduce(|acc, filter| acc.and(filter));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-core-base/src/mdl/utils.rs(1 hunks)wren-core/core/src/logical_plan/analyze/model_generation.rs(3 hunks)wren-core/core/src/mdl/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
wren-core/core/src/mdl/mod.rs (4)
wren-core/core/src/mdl/context.rs (1)
new(339-361)wren-core/core/src/logical_plan/analyze/model_anlayze.rs (2)
new(74-84)analyze(47-66)wren-core-base/src/mdl/builder.rs (1)
new_required(177-183)wren-core/core/src/logical_plan/analyze/access_control.rs (1)
build_headers(563-571)
wren-core/core/src/logical_plan/analyze/model_generation.rs (1)
wren-core/core/src/mdl/utils.rs (1)
quoted(209-211)
⏰ 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). (6)
- GitHub Check: cargo test (macos)
- GitHub Check: cargo test (macos-aarch64)
- GitHub Check: cargo check
- GitHub Check: cargo test (win64)
- GitHub Check: test
- GitHub Check: ci
🔇 Additional comments (2)
wren-core-base/src/mdl/utils.rs (1)
46-52: Explicit lifetime on Cow is correct and harmlessMaking the lifetime explicit as Cow<'_, str> is idiomatic and keeps the function signature precise without changing behavior. Call sites that rely on type inference remain unaffected.
wren-core/core/src/logical_plan/analyze/model_generation.rs (1)
237-238: Non-early return error style is fineReturning plan_err!("...") as the tail expression maintains behavior and keeps the control flow consistent. No issues here.
goldmedal
force-pushed
the
fix/rlac-case-sensitive
branch
from
2c7abed to
b3f2272
Compare
|
Thanks @goldmedal |
2 participants
I missed to add quotes for the alias of the model subquery. It causes some case-sensitive issues.
Summary by CodeRabbit
New Features
Bug Fixes
Tests