You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/09/14 23:16:50 UTC
[PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
mikebridge opened a new pull request, #44270:
URL: https://github.com/apache/superset/pull/44270
### SUMMARY
`get_table` had no way to choose a time grain on a semantic view, so "revenue by month" was not expressible through the tool:
* `dimensions: ["metric_time__month"]` (the grain-variant id Explore's time-grain control uses) → `ValidationError: Unknown dimension: 'metric_time__month'. Did you mean: metric_time?`
* `dimensions: ["metric_time"]` → succeeds, but silently returns whichever variant the layer picks (DAY) as `metric_time__day`, typed `data_type: "string"` with values like `"2024-09-01T00:00:00Z"`.
The plumbing already existed on both sides and was simply never connected: `build_query_dict` (`superset/common/tabular_query.py`) emits a `BASE_AXIS` column plus `extras["time_grain_sqla"]` when given `time_grain`/`grain_column`, and the semantic-layer mapper resolves that to the matching per-grain dimension variant.
* **`GetTableRequest.time_grain`** — an ISO-8601 duration (`P1M`) or a grain name (`month`, case-insensitive), normalized from the `Grains` registry rather than a second hardcoded table.
* **Validated against the view's own grains** (`SemanticView.get_time_grains()`); an unsupported value is refused with the view's queryable grains listed. It applies to `time_column`, otherwise to the single selected temporal dimension; selecting several temporal dimensions without `time_column` is an explicit ambiguity error rather than a silent pick. Built-in datasets reject the parameter instead of ignoring it.
* **Grain-alias hint** — a dimension shaped `<temporal dimension>__<grain name>` adds a hint naming the base dimension and the `time_grain` value to use, alongside the existing "Did you mean".
* **Temporal typing** — `format_data_columns` takes an optional explicit set of temporal column names (omitted ⇒ existing inference, so every other caller is byte-identical); `get_table` passes its datasource's temporal columns and matches the `<dimension>__<grain>` ids semantic views return. Temporal columns are typed `datetime` instead of `string`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Live `superset mcp run` against the Jaffle Shop semantic view:
```
{"view_id": 1, "metrics": ["revenue"], "dimensions": ["metric_time"], "time_grain": "P1M"}
before: (no such parameter)
after: columns [("metric_time", "datetime"), ("revenue", "numeric")]
rows 2024-09-01T00:00:00Z 15218.0 | 2024-10-01T00:00:00Z 19422.0 | 2024-11-01T00:00:00Z 24135.0
"time_grain": "month" → identical result
"time_grain": "PT1S" → Unsupported time_grain 'PT1S' on view 'Jaffle Shop'.
Queryable grains: P1D (Day), P1M (Month), P1W (Week), P1Y (Year), P3M (Quarter).
dimensions ["metric_time__month"] → …Did you mean: metric_time?; For 'metric_time__month', request
dimension 'metric_time' and pass time_grain='P1M' instead.
no time_grain (unchanged path) → column metric_time__day, now typed datetime rather than string
```
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/mcp_service/semantic_layer/ tests/unit_tests/mcp_service/utils/
```
Tests cover the grain reaching the query dict (`extras["time_grain_sqla"]` and the BASE_AXIS `timeGrain`) for both the duration and the name form, the unsupported-grain error listing the view's grains, the alias hint, the ambiguity error, the built-in rejection, and `datetime` typing. The typing and grain-threading tests were both verified red-first (`assert 'string' == 'datetime'`; `KeyError: 'extras'`).
### ADDITIONAL INFORMATION
- [x] Has associated issue: sc-120962
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5726083872
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4033310599
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -264,7 +342,18 @@ def _build_response(
warnings=warnings,
)
- columns_meta = format_data_columns(data, raw_columns)
+ # Semantic views return grain variants as <temporal dimension>__<grain>.
+ result_temporal_columns: set[str] = {
+ name
+ for name in raw_columns
+ if any(
+ name == base or name.startswith(f"{base}__")
+ for base in temporal_columns or set()
+ )
+ }
Review Comment:
**Suggestion:** Any result column beginning with a temporal column name is marked `datetime`, including unrelated dimensions or metrics such as `date__label`.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Incorrect condition logic`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9139fd88cc0a4725b7e707d33c7d5ecc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9139fd88cc0a4725b7e707d33c7d5ecc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/semantic_layer/tool/get_table.py
**Line:** 346:353
**Comment:**
*Incorrect Condition Logic: Any result column beginning with a temporal column name is marked `datetime`, including unrelated dimensions or metrics such as `date__label`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44270&comment_hash=bc903e0cf0243665ce453e641427f1bf12674e8b5214ab9acc76f41b86037dee&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44270&comment_hash=bc903e0cf0243665ce453e641427f1bf12674e8b5214ab9acc76f41b86037dee&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5672105900
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44270:
URL: https://github.com/apache/superset/pull/44270
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4041219273
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,52 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ duration: str | None
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
+ if (duration := grain["duration"]) is not None
+ }
+ grain_column: str | None = request.time_column
+ if request.time_grain:
+ selected: list[str] = sorted(set(request.dimensions) & valid_dttm_columns)
+ if grain_column is None:
+ if len(selected) != 1:
+ return SemanticLayerError.create(
+ error=f"time_grain on view '{display_name}' requires one temporal "
+ f"dimension; selected: {selected}. Set time_column explicitly.",
+ error_type="ValidationError",
+ )
+ grain_column = selected[0]
+ # The mapper chooses variants by dimension name, not the view-wide union.
+ valid_grains = {
+ dimension.grain.representation: dimension.grain.name
+ for dimension in view.implementation.get_dimensions()
+ if dimension.name == grain_column and dimension.grain is not None
+ }
+ if request.time_grain not in valid_grains:
+ choices: str = ", ".join(
+ f"{duration} ({name})"
+ for duration, name in sorted(valid_grains.items())
+ )
Review Comment:
addressed in 56ba2b91c0ec05f94f9c3707bef06384030fbb5e
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4041159196
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,42 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
Review Comment:
Addressed in 03ed7c6 — `valid_grains` is now built from the grain variants of the selected time column only (matched by dimension name), not the view-wide union.
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,42 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
+ if (duration := grain["duration"]) is not None
+ }
+ grain_column: str | None = request.time_column
Review Comment:
Addressed in 03ed7c6 — `time_col` is now set from `grain_column`, so the same column drives both the filter and the grain axis; the error message names the column.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4048099910
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py:
##########
@@ -106,6 +113,154 @@ def _make_view(view_id: int = 5) -> MagicMock:
return view
+@pytest.fixture
+def temporal_view() -> Generator[MagicMock, None, None]:
+ """Resolve a view with a temporal dimension and three queryable grains."""
+ view: MagicMock = _make_view()
+ view.columns = [
+ _make_column("metric_time", True),
+ _make_column("country_name"),
+ ]
+ view.get_time_grains.return_value = [
+ {"duration": "P1D", "name": "Day"},
+ {"duration": "P1W", "name": "Week"},
+ {"duration": "P1M", "name": "Month"},
+ ]
+ view.implementation.get_dimensions.return_value = [
+ Dimension(
+ id=f"metric_time__{grain.name}",
+ name="metric_time",
+ type=pa.timestamp("us"),
+ grain=grain,
+ )
+ for grain in (Grains.DAY, Grains.WEEK, Grains.MONTH)
+ ]
Review Comment:
Addressed in [ea94f09381](https://github.com/apache/superset/commit/ea94f09381b4784f0dc96cac4dd4e5a21458e453) — added both grain=None dimensions to the fixture. Removing the guard produces 13 failures; restoring it passes all 434 semantic-layer/utils tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5734400311
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5672145296
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44270?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `17.50000%` with `33 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.49%. Comparing base ([`ad90bf5`](https://app.codecov.io/gh/apache/superset/commit/ad90bf543f157c931a770ae866519f7f71b6baf7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3ae0e87`](https://app.codecov.io/gh/apache/superset/commit/3ae0e8768892eb7fb8486e86ffafa086d19d7ba0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 13 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44270?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...erset/mcp\_service/semantic\_layer/tool/get\_table.py](https://app.codecov.io/gh/apache/superset/pull/44270?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fsemantic_layer%2Ftool%2Fget_table.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2Uvc2VtYW50aWNfbGF5ZXIvdG9vbC9nZXRfdGFibGUucHk=) | 10.71% | [25 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44270?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/semantic\_layer/schemas.py](https://app.codecov.io/gh/apache/superset/pull/44270?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fsemantic_layer%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2Uvc2VtYW50aWNfbGF5ZXIvc2NoZW1hcy5weQ==) | 40.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44270?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/utils/response\_utils.py](https://app.codecov.io/gh/apache/superset/pull/44270?src=pr&el=tree&filepath=superset%2Fmcp_service%2Futils%2Fresponse_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvdXRpbHMvcmVzcG9uc2VfdXRpbHMucHk=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44270?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (ad90bf5) and HEAD (3ae0e87). Click for more details.
>
> <details><summary>HEAD has 7 uploads less than BASE</summary>
>
>| Flag | BASE (ad90bf5) | HEAD (3ae0e87) |
>|------|------|------|
>|python|8|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44270 +/- ##
===========================================
- Coverage 80.21% 57.49% -22.72%
===========================================
Files 2926 2926
Lines 172919 173032 +113
Branches 40115 40135 +20
===========================================
- Hits 138706 99487 -39219
- Misses 31619 72598 +40979
+ Partials 2594 947 -1647
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.44% <17.50%> (+0.22%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.44% <17.50%> (-45.18%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44270/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44270?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4042114702
##########
superset/mcp_service/utils/response_utils.py:
##########
@@ -195,6 +200,8 @@ def format_data_columns(
data_type = "boolean"
elif all(isinstance(v, (int, float)) for v in sample_values):
data_type = "numeric"
+ if temporal_columns and col_name in temporal_columns:
+ data_type = "datetime"
Review Comment:
Confirmed: the same DataColumn field is `temporal` in get_chart_data and `datetime` here. Decision: get_table adopts the existing `temporal` vocabulary — one contract across tools, no consumer change. Follow-up commit on this PR with the tests updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4043535227
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -207,15 +268,31 @@ def _validate_request_names(
# cannot resolve.
metrics_full_list_hint="call list_metrics for the full list",
)
+ base: str
+ separator: str
+ suffix: str
+ for dimension in request.dimensions:
+ base, separator, suffix = dimension.rpartition("__")
+ if dimension not in valid_columns and base in valid_columns:
+ for duration, name in (valid_grains or {}).items():
Review Comment:
Addressed in d30857aaca92f7acff9a851ed6be68aac79c3e22 — resolver metadata, selected-column validation, and alias hints share a per-column grain map. Added your metric_time Day/Month + signup_date Day-only fixture: no Month hint for signup_date, and the Day hint is accepted by validation. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5722693451
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4014745830
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,42 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
+ if (duration := grain["duration"]) is not None
+ }
+ grain_column: str | None = request.time_column
Review Comment:
With `time_range` set and `time_column` omitted, `time_col` and `grain_column` can differ: verified `granularity=metric_time` while BASE_AXIS/`timeGrain` sits on `signup_date`. `_get_grain_time_axis_column` returns `granularity` first (mapper.py:1054-1055), so the grain is dropped.
##########
superset/mcp_service/utils/response_utils.py:
##########
@@ -195,6 +200,8 @@ def format_data_columns(
data_type = "boolean"
elif all(isinstance(v, (int, float)) for v in sample_values):
data_type = "numeric"
+ if temporal_columns and col_name in temporal_columns:
+ data_type = "datetime"
Review Comment:
`get_chart_data` already ships `data_type="temporal"` for this concept via `_GENERIC_TYPE_MAP`; `get_table` now ships `"datetime"` in the same `DataColumn` field, so one MCP surface publishes two words for one type. No suggestion fence: this PR's tests assert `"datetime"`.
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,42 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
Review Comment:
`get_time_grains()` unions grains across all temporal dimensions (models.py:708), so a grain validates here even when `grain_column` has no such variant; the mapper then falls back silently (mapper.py:388-398). Verified: union `['P1D','P1M','P1Y']`, `signup_date` (P1D/P1Y) accepts P1M.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5711883062
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5708967710
Addressed the two grain-validation findings in 03ed7c6b5cc39ef8b4b03cd9e22517b1b72a1ad7:
- https://github.com/apache/superset/pull/44270#discussion_r4014745825 — validate against the selected column's provider variants; a view-wide MONTH capability no longer permits MONTH on a DAY/YEAR-only signup_date. Both explicit and inferred column cases are covered.
- https://github.com/apache/superset/pull/44270#discussion_r4014745830 — use the selected grain axis for granularity and the time-range filter when time_column is inferred. The regression checks both BASE_AXIS and actual TEMPORAL_RANGE filter placement.
Three added cases fail on the previous implementation; final scoped get_table/response suites:61passed. Full-branch Python/semantic review and all applicable file-scoped hooks (including MyPy) pass. A manually composed fixture with #44265's filter fix passes5targeted checks and preserves both resolver contracts; the second PR to land still needs final merge reconciliation.
https://github.com/apache/superset/pull/44270#discussion_r4014745835 and Bito's temporal-vocabulary note remain **open contract disposition**, not silently fixed. Existing datetime output/tests are preserved in this follow-up. Fresh CI/reviewer confirmation remains; no thread was auto-resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4043682387
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -264,7 +342,18 @@ def _build_response(
warnings=warnings,
)
- columns_meta = format_data_columns(data, raw_columns)
+ # Semantic views return grain variants as <temporal dimension>__<grain>.
+ result_temporal_columns: set[str] = {
+ name
+ for name in raw_columns
+ if any(
+ name == base or name.startswith(f"{base}__")
+ for base in temporal_columns or set()
+ )
+ }
Review Comment:
addressed in 1ec03ef4645f7edb6d2375e00b43ddd497f78054
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4041159196
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,42 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
Review Comment:
Addressed in 03ed7c6 — the grain set is now the intersection across the selected time columns rather than the union.
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,42 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
+ if (duration := grain["duration"]) is not None
+ }
+ grain_column: str | None = request.time_column
Review Comment:
Addressed in 03ed7c6 — `time_col` and `grain_column` now resolve to the same column; a mismatch is rejected with the guard error rather than silently taking the first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44270:
URL: https://github.com/apache/superset/pull/44270#issuecomment-5672121051
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 3ae0e8768892eb7fb8486e86ffafa086d19d7ba0 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa8806231405800088145c5 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44270--superset-docs-preview.netlify.app](https://deploy-preview-44270--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcoxDoAgDADAv3S2SUEg1R_4jApETUAJshjj32W74V4o9cqlwQwwwFrl9Hv37VFpmpzGLTZssqaI7cgRtyrH2WeIJV3PEvp1Iszk9KgMWSYiZmWst_D9) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44270--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4034958884
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -180,8 +194,52 @@ def _resolve_external_view(
error_type="ValidationError",
)
+ duration: str | None
+ valid_grains: dict[str, str] = {
+ duration: grain["name"]
+ for grain in view.get_time_grains()
+ if (duration := grain["duration"]) is not None
+ }
+ grain_column: str | None = request.time_column
+ if request.time_grain:
+ selected: list[str] = sorted(set(request.dimensions) & valid_dttm_columns)
+ if grain_column is None:
+ if len(selected) != 1:
+ return SemanticLayerError.create(
+ error=f"time_grain on view '{display_name}' requires one temporal "
+ f"dimension; selected: {selected}. Set time_column explicitly.",
+ error_type="ValidationError",
+ )
+ grain_column = selected[0]
+ # The mapper chooses variants by dimension name, not the view-wide union.
+ valid_grains = {
+ dimension.grain.representation: dimension.grain.name
+ for dimension in view.implementation.get_dimensions()
+ if dimension.name == grain_column and dimension.grain is not None
+ }
+ if request.time_grain not in valid_grains:
+ choices: str = ", ".join(
+ f"{duration} ({name})"
+ for duration, name in sorted(valid_grains.items())
+ )
Review Comment:
Reusing `valid_grains` for the per-column narrowing also narrows what reaches `_validate_request_names`, so the grain-alias hint regresses: with `time_grain=P1D` on `metric_time`, `signup_date__Year` no longer gets the `time_grain='P1Y'` redirect (verified).
```suggestion
column_grains: dict[str, str] = {
dimension.grain.representation: dimension.grain.name
for dimension in view.implementation.get_dimensions()
if dimension.name == grain_column and dimension.grain is not None
}
if request.time_grain not in column_grains:
choices: str = ", ".join(
f"{duration} ({name})"
for duration, name in sorted(column_grains.items())
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4041411438
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -207,15 +268,31 @@ def _validate_request_names(
# cannot resolve.
metrics_full_list_hint="call list_metrics for the full list",
)
+ base: str
+ separator: str
+ suffix: str
+ for dimension in request.dimensions:
+ base, separator, suffix = dimension.rpartition("__")
+ if dimension not in valid_columns and base in valid_columns:
+ for duration, name in (valid_grains or {}).items():
Review Comment:
My r2 suggestion restored the view-wide union here while validation went per-column, so the hint dead-ends: `metric_time` Day/Month + `signup_date` Day-only ⇒ `signup_date__Month` is told to pass `time_grain='P1M'`, which errors `Queryable grains: P1D (Day)`. Fix spans 3 sites.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4042132112
##########
superset/mcp_service/utils/response_utils.py:
##########
@@ -195,6 +200,8 @@ def format_data_columns(
data_type = "boolean"
elif all(isinstance(v, (int, float)) for v in sample_values):
data_type = "numeric"
+ if temporal_columns and col_name in temporal_columns:
+ data_type = "datetime"
Review Comment:
addressed in c215705676c27b3624937264de4323935aac6768
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mcp): accept a time grain in get_table on semantic views [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44270:
URL: https://github.com/apache/superset/pull/44270#discussion_r4045215486
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_get_table.py:
##########
@@ -106,6 +113,154 @@ def _make_view(view_id: int = 5) -> MagicMock:
return view
+@pytest.fixture
+def temporal_view() -> Generator[MagicMock, None, None]:
+ """Resolve a view with a temporal dimension and three queryable grains."""
+ view: MagicMock = _make_view()
+ view.columns = [
+ _make_column("metric_time", True),
+ _make_column("country_name"),
+ ]
+ view.get_time_grains.return_value = [
+ {"duration": "P1D", "name": "Day"},
+ {"duration": "P1W", "name": "Week"},
+ {"duration": "P1M", "name": "Month"},
+ ]
+ view.implementation.get_dimensions.return_value = [
+ Dimension(
+ id=f"metric_time__{grain.name}",
+ name="metric_time",
+ type=pa.timestamp("us"),
+ grain=grain,
+ )
+ for grain in (Grains.DAY, Grains.WEEK, Grains.MONTH)
+ ]
Review Comment:
No fixture carries a `grain=None` dimension, but production `get_dimensions()` always returns them — the unaggregated variant plus every non-temporal dim. Deleting the `grain is not None` guard in `_grains_by_column` leaves all 70 green; with this fixture it fails 13.
```suggestion
for grain in (Grains.DAY, Grains.WEEK, Grains.MONTH)
] + [
# Production ``get_dimensions()`` also returns the unaggregated variant
# and every non-temporal dimension, both with ``grain=None``.
Dimension(id="metric_time", name="metric_time", type=pa.timestamp("us")),
Dimension(id="country_name", name="country_name", type=pa.string()),
]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org