chore(mdl): update the JSON schema of WrenMDL#1934
Conversation
WalkthroughThe JSON Schema for wren-mdl was updated to add column- and row-level access control constructs, introduce a reusable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Engine as Query Engine
participant Schema as Model Schema
participant Session as Session Props
participant RowRules as Row ACLs
participant ColRules as Column ACLs
User->>Engine: Submit query
Engine->>Schema: Load model, view, column definitions
Engine->>Session: Resolve `$defs/sessionProperty` values (use `defaultExpr` if present)
Engine->>RowRules: Evaluate `rowLevelAccessControls` (conditions using session props)
alt row allowed
Engine->>Schema: include rows
else
Engine->>Schema: exclude rows
end
Engine->>ColRules: For each column, check `isHidden` and `columnLevelAccessControl`
alt column visible & ACL satisfied
Engine->>User: include column in result
else
Engine->>User: omit column
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ 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: 3
🧹 Nitpick comments (4)
wren-mdl/mdl.schema.json (4)
131-158: Root fields LGTM; minor nit: redundant minLength with enum
"minLength": 1is unnecessary whenenumis present."dataSource": { "description": "the data source type", "type": "string", "enum": [ "BIGQUERY", @@ - ], - "minLength": 1 + ] },
89-95: Consider widening column.properties to match othersColumn-level
properties.additionalPropertiesstill restricts to string, unlike models/relationships/etc."properties": { "description": "the customize properties of the column", "type": "object", "additionalProperties": { - "type": "string" + "type": ["string", "number", "boolean", "object", "array", "null"] } }
206-207: Typos and terminology polish in user-facing descriptions
- Line 206-207: “OEN_TO_MANY” → “ONE_TO_MANY”.
- Line 339: “It’s should belong” → “It should belong”.
- Lines 404, 172: Prefer “SQL” over “sql”.
- Lines 242, 291, 409, 449, 460: “the customize properties” → “custom properties”.
I can prep a follow-up copy-edit PR if helpful.
Also applies to: 339-369, 404-407, 214-217
48-59: Optional: operator/dataType coherenceIf
operatorincludes numeric comparisons, consider constrainingthreshold.dataTypeaccordingly or documenting behavior for STRING comparisons.Also applies to: 78-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wren-mdl/mdl.schema.json(9 hunks)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-mdl/mdl.schema.json (7)
36-39: isHidden addition looks goodClear, boolean, and optional. No concerns.
246-246: Broadened additionalProperties for models: goodThe union type increases flexibility for custom props.
295-295: Broadened additionalProperties for relationships: goodConsistent with models; no issues.
384-384: Broadened additionalProperties for metrics: goodNo concerns.
412-412: Broadened additionalProperties for views: goodNo concerns.
452-452: Broadened additionalProperties for enum member properties: goodNo concerns.
463-463: Broadened additionalProperties for enum definitions: goodNo concerns.
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-mdl/mdl.schema.json (1)
212-214: Fix typo in user-facing description: OEN_TO_MANY → ONE_TO_MANYThis appears in docs and tooling; keep polished.
Apply:
- "description": "the primary key of the model. It's required if the model is the one side of any OEN_TO_MANY or MANY_TO_ONE relationship", + "description": "the primary key of the model. It's required if the model is the one side of any ONE_TO_MANY or MANY_TO_ONE relationship",
🧹 Nitpick comments (3)
wren-mdl/mdl.schema.json (3)
93-99: Align column.properties value types with the rest (consistency)Other sections permit string|number|boolean|object|array|null; columns still restrict to string only.
Apply:
"properties": { "description": "the customize properties of the column", "type": "object", "additionalProperties": { - "type": "string" + "type": ["string", "number", "boolean", "object", "array", "null"] } }
104-123: Session property: default and naming clarity
- Provide a default for required to reduce ambiguity at authoring time.
- Optional: consider isRequired to avoid confusion with JSON Schema “required”.
Apply:
"required": { "description": "whether the session property is required or not", - "type": "boolean" + "type": "boolean", + "default": false },If you want to rename to isRequired, I can draft the full diff including references.
279-288: Optional: enforce distinct model names in relationshipsYou already constrain to exactly two entries; make them unique.
Apply:
"models": { "description": "the list of models", "type": "array", "items": { "type": "string", "minLength": 1 }, "minItems": 2, - "maxItems": 2 + "maxItems": 2, + "uniqueItems": true },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wren-mdl/mdl.schema.json(10 hunks)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
wren-mdl/mdl.schema.json (6)
36-39: LGTM: isHidden fits the column contractBoolean defaulting handled by consumers. No schema issues here.
137-164: LGTM: sampleDataFolder and dataSourceEnums look sane; minLength is redundant with enum but harmless.
303-304: LGTM: broadened property value typesThe union matches other sections and improves extensibility.
311-389: LGTM: metrics section WIP markers and property unionsConstraints look coherent; timeGrain has solid enums.
421-422: LGTM: views.properties value type unionConsistent with models/relationships.
462-463: LGTM: enumDefinitions properties broadenedAllows richer annotations on enums and members.
Also applies to: 473-474
2 participants
Description
isHiddenfor columnSummary by CodeRabbit
New Features
Changes
Removals