Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional JSON Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MCP_Server as MCP_Server
participant ModelObj as ModelObj
participant Storage as Storage
Client->>MCP_Server: edit_model(model_id, meta=payload)
MCP_Server->>ModelObj: load(model_id)
alt meta is _UNSET
MCP_Server->>ModelObj: (no change)
else meta is None
MCP_Server->>ModelObj: set model.meta = None
MCP_Server-->>MCP_Server: record "cleared meta"
else meta is dict
MCP_Server->>ModelObj: set model.meta = dict
MCP_Server-->>MCP_Server: record "updated meta"
end
MCP_Server->>Storage: persist(ModelObj)
MCP_Server->>Client: return result
sequenceDiagram
participant Client as Client
participant API as API_Server
participant Engine as QueryEngine
participant Storage as Storage
Client->>API: /query(request)
API->>Engine: execute(query)
Engine->>Storage: read schema & metadata
Engine-->>API: SlayerResponse(attributes: {dimensions, measures}, rows, sql)
API-->>Client: QueryResponse(attributes=..., rows, sql)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/mcp/server.py`:
- Line 451: The current parameter handling prevents callers from explicitly
clearing metadata because the function signatures use extra: Optional[Dict[str,
Any]] = None and checks like "if extra is not None" only update when non-None;
change to use a unique sentinel (e.g., _UNSET = object()) as the default for the
extra parameter (extra: Optional[Dict[str, Any]] = _UNSET) and replace checks
"if extra is not None" with "if extra is not _UNSET" so callers can pass None to
explicitly clear extra while distinguishing unspecified calls; update all
relevant signatures and conditionals referenced (the places around lines with
extra param and the checks at lines ~464-465 and ~514-516) to use the sentinel.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5c484a4-47dd-471b-bd0e-e775acd07c41
📒 Files selected for processing (4)
CLAUDE.mddocs/concepts/models.mdslayer/core/models.pyslayer/mcp/server.py
AivanF
force-pushed
the
aivan/dev-1261-slayer-metadata-in-models-mes-dim
branch
from
3bebab6 to
f0e9d72
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/client/slayer_client.py`:
- Around line 68-75: The parser _parse_meta_dict currently constructs
FieldMetadata only with label (loss of API-provided number formatting); update
_parse_meta_dict to also extract and pass format (e.g., format=v.get("format"))
into the FieldMetadata constructor so ResponseAttributes.dimensions and
.measures retain formatting metadata from attrs_raw/result; if FieldMetadata has
no format parameter, add one or set the format attribute on the FieldMetadata
instance before returning.
In `@slayer/mcp/server.py`:
- Around line 515-517: In edit_model, the conditional appending to changes uses
an undefined name extra causing a NameError; change the conditional to reference
meta instead (i.e., append "updated meta" if meta is not None else "cleared
meta") so the block that sets model.meta and appends to changes uses the correct
variable (model.meta, meta, changes).
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aad4acc0-74a4-4d43-92d6-081aaeea2e9c
📒 Files selected for processing (9)
CLAUDE.mddocs/concepts/models.mdslayer/api/server.pyslayer/client/slayer_client.pyslayer/core/models.pyslayer/engine/query_engine.pyslayer/mcp/server.pytests/integration/test_integration.pytests/test_format_propagation.py
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
AivanF
force-pushed
the
aivan/dev-1261-slayer-metadata-in-models-mes-dim
branch
from
395a7c0 to
59393ef
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
slayer/client/slayer_client.py (1)
68-69:⚠️ Potential issue | 🟠 MajorPreserve
formatwhile parsing attribute metadata.Line 69 still constructs
FieldMetadatawith onlylabel, so API-providedformatmetadata is dropped for remote clients.Suggested fix
+ from slayer.core.format import NumberFormat + def _parse_meta_dict(d: dict) -> Dict[str, FieldMetadata]: - return {k: FieldMetadata(label=v.get("label")) for k, v in (d or {}).items()} + parsed: Dict[str, FieldMetadata] = {} + for k, v in (d or {}).items(): + fmt = NumberFormat.model_validate(v["format"]) if v.get("format") else None + parsed[k] = FieldMetadata(label=v.get("label"), format=fmt) + return parsed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/client/slayer_client.py` around lines 68 - 69, The _parse_meta_dict function currently constructs FieldMetadata using only the label and discards any API-provided format; update _parse_meta_dict to pass the format (e.g., v.get("format")) into the FieldMetadata constructor alongside label so the FieldMetadata instances preserve the format metadata returned by the API (refer to _parse_meta_dict and FieldMetadata to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/api/server.py`:
- Around line 79-87: Guard against result.attributes being None before accessing
attrs.dimensions or attrs.measures: check whether result.attributes is truthy
(or explicitly None) after assigning attrs = result.attributes and only call
_convert_meta and construct AttributesResponse when attrs exists; otherwise set
attributes = None. Update the conditional that currently reads "if
attrs.dimensions or attrs.measures" to first verify attrs is not None (or use
safe defaults like empty dicts) so the references to attrs.dimensions and
attrs.measures in the AttributesResponse construction are only executed when
attrs is present. Ensure you modify the code around the symbols attrs,
result.attributes, _convert_meta, and AttributesResponse.
In `@slayer/client/slayer_client.py`:
- Around line 66-67: The local import of ResponseAttributes in slayer_client.py
should be moved to the module/top-level imports: remove the inline "from
slayer.engine.query_engine import ResponseAttributes" and add that import
alongside the file's other top-level imports; if this was done to avoid a
circular import, instead guard it with a typing.TYPE_CHECKING import or import
inside an if TYPE_CHECKING block so the symbol is still available for type
annotations while keeping runtime imports at module scope.
---
Duplicate comments:
In `@slayer/client/slayer_client.py`:
- Around line 68-69: The _parse_meta_dict function currently constructs
FieldMetadata using only the label and discards any API-provided format; update
_parse_meta_dict to pass the format (e.g., v.get("format")) into the
FieldMetadata constructor alongside label so the FieldMetadata instances
preserve the format metadata returned by the API (refer to _parse_meta_dict and
FieldMetadata to locate the change).
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 475682c9-3272-4b71-924f-874db47c17c4
📒 Files selected for processing (9)
CLAUDE.mddocs/concepts/models.mdslayer/api/server.pyslayer/client/slayer_client.pyslayer/core/models.pyslayer/engine/query_engine.pyslayer/mcp/server.pytests/integration/test_integration.pytests/test_format_propagation.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_format_propagation.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/concepts/models.md
- CLAUDE.md
- slayer/engine/query_engine.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slayer/api/server.py (1)
39-41: UseField(default_factory=dict)for mutable model defaults.Lines 40–41 use literal
{}defaults. In Pydantic v2,Field(default_factory=dict)is the recommended approach for mutable defaults; it clearly signals intent and aligns with Python standards, avoiding reliance on Pydantic's internal deep-copy mechanism.♻️ Proposed refactor
-from pydantic import BaseModel +from pydantic import BaseModel, Field @@ class AttributesResponse(BaseModel): - dimensions: Dict[str, FieldMetadataResponse] = {} - measures: Dict[str, FieldMetadataResponse] = {} + dimensions: Dict[str, FieldMetadataResponse] = Field(default_factory=dict) + measures: Dict[str, FieldMetadataResponse] = Field(default_factory=dict)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/api/server.py` around lines 39 - 41, The AttributesResponse Pydantic model currently uses literal {} for the mutable defaults on the dimensions and measures fields; replace those defaults with Field(default_factory=dict) (import Field from pydantic) so the model uses proper factory-based defaults in Pydantic v2. Update the AttributesResponse class to declare dimensions: Dict[str, FieldMetadataResponse] = Field(default_factory=dict) and measures: Dict[str, FieldMetadataResponse] = Field(default_factory=dict), leaving types and FieldMetadataResponse reference unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@slayer/api/server.py`:
- Around line 39-41: The AttributesResponse Pydantic model currently uses
literal {} for the mutable defaults on the dimensions and measures fields;
replace those defaults with Field(default_factory=dict) (import Field from
pydantic) so the model uses proper factory-based defaults in Pydantic v2. Update
the AttributesResponse class to declare dimensions: Dict[str,
FieldMetadataResponse] = Field(default_factory=dict) and measures: Dict[str,
FieldMetadataResponse] = Field(default_factory=dict), leaving types and
FieldMetadataResponse reference unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24475d96-c07d-488d-94ae-22cb1c916eda
📒 Files selected for processing (2)
slayer/api/server.pyslayer/client/slayer_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- slayer/client/slayer_client.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit