You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "FrancescoCastaldi (via GitHub)" <gi...@apache.org> on 2026/09/20 20:33:48 UTC
[PR] fix(mcp): add sort_by option to XYChartConfig (#42627) [superset]
FrancescoCastaldi opened a new pull request, #44464:
URL: https://github.com/apache/superset/pull/44464
### SUMMARY
Fixes #42627.
MCP bar charts and other XY charts had no option to configure sorting; non-temporal charts were hardcoded to alphabetical ordering by category name (`x_axis_sort_series_type = "name"` and `x_axis_sort_series_ascending = True`).
This PR adds the `sort_by` field to `XYChartConfig`:
- Adds `sort_by: SortByConfig | str | List[SortByConfig | str] | None` to `XYChartConfig` with alias support (`x_axis_sort`, `order_by`) and a `coerce_sort_by` field validator supporting bare strings, dicts, or single-item lists (defaulting bare strings to descending order).
- Implements `add_xy_sort_config` in `superset/mcp_service/chart/chart_utils.py`:
- When non-temporal, sets `x_axis_sort` and `x_axis_sort_asc`, alongside backward-compatibility keys `x_axis_sort_series_type` (`"name"` if sorting by x-axis dimension, `"value"` otherwise) and `x_axis_sort_series_ascending`.
- When temporal, ignores `sort_by` with a warning in `form_data["_mcp_warnings"]` to maintain chronological sorting.
- Maintains default behavior when `sort_by` is not specified.
- Updates `XYChartPlugin` in `superset/mcp_service/chart/plugins/xy.py`:
- Extracts `ColumnRef` for the sort column in `extract_column_refs`.
- Resolves canonical column or metric name in `normalize_column_refs`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (Backend MCP API / Schema change)
### TESTING INSTRUCTIONS
Run unit tests for MCP chart schemas and chart utils:
```bash
pytest tests/unit_tests/mcp_service/chart/test_chart_schemas.py tests/unit_tests/mcp_service/chart/test_chart_utils.py
```
All 297 unit tests pass, covering:
- Coercion of bare strings, single-item lists, dicts, and `SortByConfig` objects.
- Validation aliases (`x_axis_sort`, `order_by`).
- Non-temporal metric and category sorting with ascending/descending flags.
- Temporal chart warning behavior.
- Column reference extraction and canonicalization in `XYChartPlugin`.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #42627
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4173577051
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1095,6 +1095,10 @@ class SortByConfig(UnknownFieldCheckMixin):
description="Sort ascending. Defaults to False (descending) to match "
"the typical sort-by-metric top-N use case.",
)
+ saved_metric: bool | None = Field(
Review Comment:
Confirmed at ee9a0b4c61: the no-null-defaults test passes, and dropping `OmittedMeansUnchanged` from `SortByConfig` turns it red again.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4232914600
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1086,13 +1110,30 @@ def build_single_query_dict(
order_desc if order_desc is not None else form_data.get("order_desc", True)
)
qd["orderby"] = [(sort_metric, not descending)]
+ elif (
Review Comment:
Yes, please drop it, together with its `build_single_query_dict` x_axis_sort tests; XY sort is already covered through `build_query_dicts_from_form_data`.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5849793079
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4116055441
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2569,6 +2588,34 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), a "
+ "[column, ascending] pair, or a single-item list containing either. "
+ "Multi-column sorting is not supported for XY charts."
+ ),
Review Comment:
Applied the suggested description wording in commit 81a94cc. The serialized schema size is now within the 50,000 B budget and mcp_service unit tests pass.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4112768061
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
Thanks for the review and suggestions! This is now fully resolved and verified across the pipeline as of \ db3ea5\:
- \xtract_column_refs\ checks covered XY names (including x-axis, group-by, and y-axis metrics/labels and SQL expressions), skipping redundant \ColumnRef\ additions that previously triggered premature validation failures.
- Saved metrics and SQL expression metrics are resolved against \dataset_context.available_metrics\ with \saved_metric=True\.
- End-to-end integration tests were added in \TestValidationPipelineWithXYChartSortBy\ to ensure \sort_by\ traverses \ValidationPipeline\ and sets \ orm_data[\x_axis_sort\]\, correctly resolving to metric labels (e.g. \SUM(sales)\).
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115308064
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -699,8 +722,19 @@ def _build_single_query_dict(
# metric descending. buildQuery derives this on the frontend; translate
# the flag here when there is no explicit ordering or a row_limit truncates
# an unordered result (dropping the heaviest rows rather than the top-N).
+ viz_type = form_data.get("viz_type") or ""
+ is_temporal = (
+ is_timeseries
+ or viz_type == "mixed_timeseries"
+ or bool(form_data.get("granularity_sqla"))
+ )
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif not is_temporal and form_data.get("x_axis_sort") and not qd.get("orderby"):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ qd["orderby"] = [(sort_target, sort_asc)]
Review Comment:
Good catch. Updated _build_single_query_dict to check that sort_target is a valid metric or column before adding it to orderby, avoiding invalid series-sort aggregates from being propagated. Also added unit test est_build_single_query_dict_x_axis_sort_ignores_unmatched_series_sort in commit \8ab8a7\.
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115405780
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `916ffe2`.
Added sort-by normalization that resolves saved metrics and Y-axis labels to canonical names before validation, preserving the saved-metric status in `SortByConfig`.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5954462369
Thanks @rusackas! All set — rebased and merged with `master` against #44746.
Resolved the conflicts in `chart_helpers.py` and `test_chart_helpers.py` by aligning with the new public `build_single_query_dict`, ensuring full compliance with the plugin architectural contracts, and keeping both test suites intact. All 2,800+ chart MCP unit tests and ruff lint/format checks are passing green.
Ready for merge whenever you're good!
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4173195687
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1095,6 +1095,10 @@ class SortByConfig(UnknownFieldCheckMixin):
description="Sort ascending. Defaults to False (descending) to match "
"the typical sort-by-metric top-N use case.",
)
+ saved_metric: bool | None = Field(
Review Comment:
Merged master (including #44573) and added OmittedMeansUnchanged to SortByConfig bases in commit ee9a0b4c61.
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4112263723
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
+ if isinstance(first, str):
+ return SortByConfig(column=first, ascending=False)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `ff3dc4e`.
Multi-item sort lists are now rejected with a clear validation error instead of being reduced to the first entry. The field description also documents that XY charts support only a single sort column.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5753606037
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115375161
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `4f996ea`.
`sort_by` entries are normalized through `_update_sort_by_column`, which resolves canonical names and preserves saved-metric status via `SortByConfig` before validation.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115808691
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2569,6 +2588,34 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), a "
+ "[column, ascending] pair, or a single-item list containing either. "
+ "Multi-column sorting is not supported for XY charts."
+ ),
Review Comment:
`unit-tests` fails on this PR: this description pushes the `generate_chart` schema to 50127 B, over the 50000 B budget in `test_tool_inventory.py` (master is 49531 B). This wording brings it to 49978 B, and all `mcp_service` tests pass:
```suggestion
description=(
"Sort by the x column or one y metric; a bare name sorts "
"descending, or pass [column, ascending]."
),
```
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4066340246
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +126,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
codeant C1 confirmed, and broader: it fails on **every** path, not only pre-normalization. Probed: sort by saved metric -> `ref doesn't have saved_metric=true`; sort by a y label -> `Column 'Total Sales' not found`. Skipping refs already covered by `y` fixes both; suite 1884 passed.
```suggestion
if sort_col and not any(
sort_col.lower() in {n.lower() for n in (yc.name, yc.label) if n}
for yc in config.y
):
refs.append(ColumnRef(name=sort_col))
```
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -42,6 +42,29 @@
logger = logging.getLogger(__name__)
+def _normalize_xy_sort_by(config_dict: dict[str, Any], dataset_context: Any) -> None:
+ """Resolve canonical column or metric name for sort_by in XY charts."""
+ sort_by = config_dict.get("sort_by")
+ if not sort_by:
+ return
+
+ get_canonical = DatasetValidator.get_canonical_column_name
+
+ def _resolve_name(raw_name: str) -> str:
+ raw_lower = raw_name.lower()
+ for y_col in config_dict.get("y") or []:
+ if y_col.get("label") and y_col["label"].lower() == raw_lower:
+ return y_col["label"]
Review Comment:
bito B1 is **wrong** — returning `label` here is correct. `map_xy_config` emits the metric as `label: 'Total Sales'` (hasCustomLabel), and Timeseries/transformProps.ts:502 sorts series by that label, so `x_axis_sort='Total Sales'` matches. Returning `name` would be the bug. No change needed.
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,250 @@ def test_normalize_column_names_skips_sql_metric_dicts(self) -> None:
)
assert normalized.y[0].sql_expression == _SQL_EXPR
assert normalized.y[0].name is None
+
+
+class TestAddXYSortConfig:
+ """Test add_xy_sort_config helper function."""
+
+ def test_no_sort_by_does_nothing(self) -> None:
+ form_data: dict[str, Any] = {
+ "x_axis_sort_series_type": "name",
+ "x_axis_sort_series_ascending": True,
+ }
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_non_temporal_sort_by_metric_descending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert form_data["x_axis_sort_series_type"] == "value"
+ assert form_data["x_axis_sort_series_ascending"] is False
+
+ def test_non_temporal_sort_by_metric_ascending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="sales", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "sales"
+ assert form_data["x_axis_sort_asc"] is True
+ assert form_data["x_axis_sort_series_type"] == "value"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_non_temporal_sort_by_x_axis_column(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="category", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "category"
+ assert form_data["x_axis_sort_asc"] is True
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_temporal_sort_by_ignored_with_warning(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="line",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=True)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert "x_axis_sort_series_type" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+
+class TestMapXYConfigWithSortBy:
+ """Test map_xy_config integration with sort_by."""
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_metric(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert form_data["x_axis_sort_series_type"] == "value"
+ assert form_data["x_axis_sort_series_ascending"] is False
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_x_axis(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="department", ascending=True),
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "department"
+ assert form_data["x_axis_sort_asc"] is True
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_temporal_ignores_sort_by(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = True
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="line",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert "x_axis_sort" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_without_sort_by_keeps_defaults(
+ self, mock_is_temporal
+ ) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert "x_axis_sort" not in form_data
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+
+class TestXYChartPluginSortBy:
+ """Test XYChartPlugin extract_column_refs and normalize_column_refs with sort_by."""
+
+ def test_extract_column_refs_includes_sort_by(self) -> None:
+ from superset.mcp_service.chart.plugins.xy import XYChartPlugin
+
+ plugin = XYChartPlugin()
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ sort_by=SortByConfig(column="sales", ascending=False),
+ )
+ refs = plugin.extract_column_refs(config)
+ ref_names = [r.name for r in refs]
+
+ assert "category" in ref_names
+ assert "sales" in ref_names
Review Comment:
bito B2 confirmed by mutation. Control arm (no-op edit): 297 passed. Then `if False and config.sort_by:` in `extract_column_refs` -> still **297 passed**, zero tests red. `sales` is already in `y`, so the assertion cannot see the branch. Sorting by a distinct column kills the mutant.
```suggestion
sort_by=SortByConfig(column="profit", ascending=False),
)
refs = plugin.extract_column_refs(config)
ref_names = [r.name for r in refs]
assert "category" in ref_names
assert "profit" in ref_names
```
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
Review Comment:
codeant C2 confirmed. `model_json_schema()` advertises `type: array` with no `maxItems`, so a client may legitimately send 2 items; they silently become `SortByConfig(column='a')`. Only the prose says single-item. Raising leaves 1-item and empty unchanged; suite 1884 passed.
```suggestion
if isinstance(v, list):
if not v:
return None
if len(v) > 1:
raise ValueError(
"sort_by accepts at most one sort criterion, "
f"received {len(v)}."
)
first = v[0]
```
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,50 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``, along with backward-compatibility keys
Review Comment:
Not back-compat keys: `x_axis_sort_series_type` occurs nowhere else in the repo, and transformProps.ts:502 derives `xAxisSortSeries` from `x_axis_sort`. Migration 378cecfdba9f pops `x_axis_sort_series*` into `x_axis_sort`. Both writes are dead (`:1176` predates this 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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4112768061
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
Thanks for the review and suggestions! This is now fully resolved and verified across the pipeline as of `fdb3ea5`:
- `extract_column_refs` checks covered XY names (including x-axis, group-by, and y-axis metrics/labels and SQL expressions), skipping redundant `ColumnRef` additions that previously triggered premature validation failures.
- Saved metrics and SQL expression metrics are resolved against `dataset_context.available_metrics` with `saved_metric=True`.
- End-to-end integration tests were added in `TestValidationPipelineWithXYChartSortBy` to ensure `sort_by` traverses `ValidationPipeline` and sets `form_data["x_axis_sort"]`, correctly resolving to metric labels (e.g. `SUM(sales)`).
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4112895981
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,630 @@ def test_normalize_column_names_skips_sql_metric_dicts(self) -> None:
)
assert normalized.y[0].sql_expression == _SQL_EXPR
assert normalized.y[0].name is None
+
+
+class TestAddXYSortConfig:
+ """Test add_xy_sort_config helper function."""
+
+ def test_no_sort_by_does_nothing(self) -> None:
+ form_data: dict[str, Any] = {
+ "x_axis_sort_series_type": "name",
+ "x_axis_sort_series_ascending": True,
+ }
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_non_temporal_sort_by_metric_descending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_ascending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="sales", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_with_custom_label(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM", label="Total Sales")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "Total Sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_saved_metric(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="total_revenue", saved_metric=True)],
+ kind="bar",
+ sort_by="total_revenue",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "total_revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_column(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="category", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "category"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_case_insensitive(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="category",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "ProductCategory"
+
+ form_data2: dict[str, Any] = {}
+ config2 = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="productcategory",
+ )
+ add_xy_sort_config(form_data2, config2, x_is_temporal=False)
+ assert form_data2["x_axis_sort"] == "ProductCategory"
+
+ def test_non_temporal_sort_by_sql_expression(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[
+ ColumnRef(
+ sql_expression="COUNT(DISTINCT user_id)", label="unique_users"
+ )
+ ],
+ kind="bar",
+ sort_by="count(distinct user_id)",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "unique_users"
+
+ def test_non_temporal_sort_by_pair_format(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=["sales", True],
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+
+ def test_temporal_sort_by_ignored_with_warning(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="line",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=True)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert "x_axis_sort_series_type" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
BITO.md rule 12148 requires every newly added test function to carry a docstring documenting purpose and expected behavior; none of the ten tests in `TestAddXYSortConfig` has one. Adding one-line docstrings (e.g. `sort_by absent leaves series-sort keys untouched`) aligns the new class with the org standard and makes each case's intent greppable. Older tests in this file also lack them, but the rule targets new functions.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Incomplete series-sort assertion</b></div>
<div id="fix">
`add_xy_sort_config` pops both `x_axis_sort_series_type` and `x_axis_sort_series_ascending` (chart_utils.py:1202-1203), and sibling tests assert both keys are absent (lines 2538, 2554, 2600). This custom-label test checks only `x_axis_sort_series_type`, leaving the ascending-key pop unverified on this resolution path. Add the missing assertion for parity and coverage.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_helpers.py:
##########
@@ -1325,3 +1327,119 @@ def test_shared_query_builder_keeps_mixed_timeseries_ordering_per_query(
expected_secondary["orderby"] = secondary_orderby
assert secondary == expected_secondary
assert form_data["orderby"] == [["count", True]]
+
+
+def test_build_single_query_dict_x_axis_sort_with_metric_label() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], [metric])
+ assert qd["orderby"] == [(metric, False)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_metric_column_name() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "sales",
+ "x_axis_sort_asc": True,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], [metric])
+ assert qd["orderby"] == [(metric, True)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_saved_metric() -> None:
+ form_data = {
+ "x_axis_sort": "revenue",
+ "x_axis_sort_asc": False,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], ["revenue"])
+ assert qd["orderby"] == [("revenue", False)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_dimension_column() -> None:
+ form_data = {
+ "x_axis_sort": "category",
+ "x_axis_sort_asc": True,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], ["revenue"])
+ assert qd["orderby"] == [("category", True)]
+
+
+def test_build_single_query_dict_prefers_existing_orderby() -> None:
+ form_data = {
+ "orderby": [["count", True]],
+ "x_axis_sort": "sales",
+ "x_axis_sort_asc": False,
+ }
+ qd = _build_single_query_dict(form_data, ["category"], ["sales"])
+ assert qd["orderby"] == [["count", True]]
+
+
+def test_build_query_dicts_from_form_data_xy_bar_with_x_axis_sort() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "viz_type": "echarts_timeseries_bar",
+ "x_axis": "category",
+ "metrics": [metric],
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ with patch(
+ "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine",
+ return_value="base",
+ ):
+ queries = build_query_dicts_from_form_data(form_data, 1, "table")
+
+ assert len(queries) == 1
+ assert queries[0]["columns"] == ["category"]
+ assert queries[0]["metrics"] == [metric]
+ assert queries[0]["orderby"] == [(metric, False)]
+
+
+def test_resolve_x_axis_sort_target_case_insensitive_label() -> None:
+ metric = {
+ "label": "Total Sales",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ resolved = _resolve_x_axis_sort_target("total sales", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_case_insensitive_column() -> None:
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "Sales"},
+ }
+ resolved = _resolve_x_axis_sort_target("sales", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_sql_expression() -> None:
+ metric = {
+ "expressionType": "SQL",
+ "sqlExpression": "COUNT(DISTINCT user_id)",
+ "label": "unique_users",
+ }
+ resolved = _resolve_x_axis_sort_target("count(distinct user_id)", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_string_metric_case_insensitive() -> None:
+ resolved = _resolve_x_axis_sort_target("totalsales", ["TotalSales"])
+ assert resolved == "TotalSales"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
The ten new test functions (lines 1332-1445) have no docstrings. BITO adaptive rule 12148 requires every newly added test function to carry a docstring stating purpose and expected behavior; neighboring tests in this file (e.g. line 1264) follow that pattern. One-line docstrings keep the file consistent and make test intent scannable.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: resolves the sort target to the corresponding metric label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0], ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
+ sort_lower = sort_entry.column.lower()
+ x_name = (config.x.name or "").lower() if config.x else None
+ x_label = (config.x.label or "").lower() if config.x else None
+
+ # If sorting by the x-axis dimension itself (case-insensitive check)
+ if config.x and (
+ (x_name and sort_lower == x_name)
+ or (x_label and sort_lower == x_label)
+ ):
+ sort_target = config.x.label or config.x.name
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>x sort target uses label not name</b></div>
<div id="fix">
`map_xy_config` sets `form_data['x_axis'] = config.x.name` (chart_utils.py:1470), but here `x_axis_sort` receives `config.x.label`. When label != name, `_resolve_x_axis_sort_target` (chart_helpers.py:679-698) matches no metric and returns the raw label string, which flows into `qd['orderby']` (chart_helpers.py:726-730) as an unresolvable name. Prefer `config.x.name`.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Agg expr mismatch vs metric label</b></div>
<div id="fix">
`create_metric_object` (chart_utils.py:1023-1038) normalizes STDDEV/VAR aliases and labels SIMPLE metrics as upper-cased `AGG(name)`. `agg_expr` here uses the raw `y_col.aggregate`, so `sort_by='stddev(height)'` never matches label 'STDDEV(height)' and falls through to `sort_entry.column`, yielding an orderby on an unmatched name. Normalize the aggregate before comparing.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,630 @@ def test_normalize_column_names_skips_sql_metric_dicts(self) -> None:
)
assert normalized.y[0].sql_expression == _SQL_EXPR
assert normalized.y[0].name is None
+
+
+class TestAddXYSortConfig:
+ """Test add_xy_sort_config helper function."""
+
+ def test_no_sort_by_does_nothing(self) -> None:
+ form_data: dict[str, Any] = {
+ "x_axis_sort_series_type": "name",
+ "x_axis_sort_series_ascending": True,
+ }
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_non_temporal_sort_by_metric_descending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_ascending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="sales", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_with_custom_label(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM", label="Total Sales")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "Total Sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_saved_metric(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="total_revenue", saved_metric=True)],
+ kind="bar",
+ sort_by="total_revenue",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "total_revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_column(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="category", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "category"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_case_insensitive(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="category",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "ProductCategory"
+
+ form_data2: dict[str, Any] = {}
+ config2 = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="productcategory",
+ )
+ add_xy_sort_config(form_data2, config2, x_is_temporal=False)
+ assert form_data2["x_axis_sort"] == "ProductCategory"
+
+ def test_non_temporal_sort_by_sql_expression(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[
+ ColumnRef(
+ sql_expression="COUNT(DISTINCT user_id)", label="unique_users"
+ )
+ ],
+ kind="bar",
+ sort_by="count(distinct user_id)",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "unique_users"
+
+ def test_non_temporal_sort_by_pair_format(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=["sales", True],
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+
+ def test_temporal_sort_by_ignored_with_warning(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="line",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=True)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert "x_axis_sort_series_type" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+
+class TestMapXYConfigWithSortBy:
+ """Test map_xy_config integration with sort_by."""
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_metric(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "SUM(revenue)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_x_axis(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="department", ascending=True),
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "department"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_temporal_ignores_sort_by(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = True
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="line",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert "x_axis_sort" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_without_sort_by_keeps_defaults(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
All four new tests in `TestMapXYConfigWithSortBy` lack docstrings, unlike sibling tests such as `test_map_xy_config_with_filters` which document intent inline. Repo rule 12148 requires a docstring on every newly added test function. Adding one line per test keeps the file consistent and makes the sort_by scenario under test explicit.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Missing mock/return type hints</b></div>
<div id="fix">
The new tests leave `mock_is_temporal` unannotated and three of the four omit `-> None` (only test_map_xy_config_non_temporal_sort_by_metric has it). Repo rules 12787/12101 require mock and return type annotations; `MagicMock` is already imported in this file, so the fix is mechanical.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_schemas.py:
##########
@@ -1219,3 +1220,223 @@ def test_update_chart_identifier_chart_id_alias(self) -> None:
def test_list_charts_select_columns_columns_alias(self) -> None:
req = ListChartsRequest.model_validate({"columns": ["id", "slice_name"]})
assert req.select_columns == ["id", "slice_name"]
+
+
+class TestXYChartConfigSortBy:
+ """Test sort_by options, coercions, and aliases in XYChartConfig."""
+
+ def test_sort_by_default_none(self) -> None:
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ )
+ assert config.sort_by is None
+
+ def test_sort_by_bare_string_coerced(self) -> None:
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ sort_by="sales",
+ )
+ assert isinstance(config.sort_by, SortByConfig)
+ assert config.sort_by.column == "sales"
+ assert config.sort_by.ascending is False
+
+ def test_sort_by_single_item_string_list_coerced(self) -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated test method logic</b></div>
<div id="fix">
A syntactic duplication was detected in tests/unit_tests/mcp_service/chart/test_chart_schemas.py. The same 12-line block appears at lines 1247-1258 and 1362-1373. Consider refactoring to a shared helper or parameterized test to reduce maintenance overhead.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/chart/test_chart_utils.py:
##########
@@ -2497,3 +2498,630 @@ def test_normalize_column_names_skips_sql_metric_dicts(self) -> None:
)
assert normalized.y[0].sql_expression == _SQL_EXPR
assert normalized.y[0].name is None
+
+
+class TestAddXYSortConfig:
+ """Test add_xy_sort_config helper function."""
+
+ def test_no_sort_by_does_nothing(self) -> None:
+ form_data: dict[str, Any] = {
+ "x_axis_sort_series_type": "name",
+ "x_axis_sort_series_ascending": True,
+ }
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert form_data["x_axis_sort_series_type"] == "name"
+ assert form_data["x_axis_sort_series_ascending"] is True
+
+ def test_non_temporal_sort_by_metric_descending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_ascending(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="sales", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_metric_with_custom_label(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM", label="Total Sales")],
+ kind="bar",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "Total Sales"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_saved_metric(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="total_revenue", saved_metric=True)],
+ kind="bar",
+ sort_by="total_revenue",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "total_revenue"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_column(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="category", ascending=True),
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+
+ assert form_data["x_axis_sort"] == "category"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ def test_non_temporal_sort_by_x_axis_case_insensitive(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="category",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "ProductCategory"
+
+ form_data2: dict[str, Any] = {}
+ config2 = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="Category", label="ProductCategory"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by="productcategory",
+ )
+ add_xy_sort_config(form_data2, config2, x_is_temporal=False)
+ assert form_data2["x_axis_sort"] == "ProductCategory"
+
+ def test_non_temporal_sort_by_sql_expression(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[
+ ColumnRef(
+ sql_expression="COUNT(DISTINCT user_id)", label="unique_users"
+ )
+ ],
+ kind="bar",
+ sort_by="count(distinct user_id)",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "unique_users"
+
+ def test_non_temporal_sort_by_pair_format(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="category"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="bar",
+ sort_by=["sales", True],
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=False)
+ assert form_data["x_axis_sort"] == "SUM(sales)"
+ assert form_data["x_axis_sort_asc"] is True
+
+ def test_temporal_sort_by_ignored_with_warning(self) -> None:
+ form_data: dict[str, Any] = {}
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="sales", aggregate="SUM")],
+ kind="line",
+ sort_by="sales",
+ )
+ add_xy_sort_config(form_data, config, x_is_temporal=True)
+
+ assert "x_axis_sort" not in form_data
+ assert "x_axis_sort_asc" not in form_data
+ assert "x_axis_sort_series_type" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+
+class TestMapXYConfigWithSortBy:
+ """Test map_xy_config integration with sort_by."""
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_metric(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "SUM(revenue)"
+ assert form_data["x_axis_sort_asc"] is False
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_non_temporal_sort_by_x_axis(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ sort_by=SortByConfig(column="department", ascending=True),
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort"] == "department"
+ assert form_data["x_axis_sort_asc"] is True
+ assert "x_axis_sort_series_type" not in form_data
+ assert "x_axis_sort_series_ascending" not in form_data
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_temporal_ignores_sort_by(self, mock_is_temporal) -> None:
+ mock_is_temporal.return_value = True
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="order_date"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="line",
+ sort_by="revenue",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert "x_axis_sort" not in form_data
+ assert len(form_data.get("_mcp_warnings", [])) == 1
+ expected_msg = "was ignored because the x-axis column 'order_date' is temporal"
+ assert expected_msg in form_data["_mcp_warnings"][0]
+
+ @patch("superset.mcp_service.chart.chart_utils.is_column_truly_temporal")
+ def test_map_xy_config_without_sort_by_keeps_defaults(
+ self, mock_is_temporal
+ ) -> None:
+ mock_is_temporal.return_value = False
+ config = XYChartConfig(
+ chart_type="xy",
+ x=ColumnRef(name="department"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ kind="bar",
+ )
+ form_data = map_xy_config(config, dataset_id=1)
+
+ assert form_data["x_axis_sort_series_ascending"] is True
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete default-path assertions</b></div>
<div id="fix">
test_map_xy_config_without_sort_by_keeps_defaults only asserts `x_axis_sort_series_ascending is True`, but the default path it names also sets `x_axis_sort_series_type = "name"` in `configure_temporal_handling` and must leave `x_axis_sort` unset (no `sort_by`). Asserting all three would catch regressions where `add_xy_sort_config` or the temporal handling defaults drift.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -701,6 +723,11 @@ def _build_single_query_dict(
# an unordered result (dropping the heaviest rows rather than the top-N).
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif form_data.get("x_axis_sort") and not qd.get("orderby"):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ qd["orderby"] = [(sort_target, sort_asc)]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Temporal sort guard bypassed</b></div>
<div id="fix">
`add_xy_sort_config` (chart_utils.py) deliberately refuses to set `x_axis_sort` when the x-axis is temporal and warns that temporal charts sort chronologically. This new branch applies `x_axis_sort` unconditionally for every viz type built here, including `echarts_timeseries`/`mixed_timeseries` (and the secondary query via `_build_mixed_timeseries_secondary`), so saved params carrying `x_axis_sort` flip temporal charts from chronological to value ordering. Guard temporal viz types before setting `orderby`.
</div>
</div>
<small><i>Code Review Run #ed1edd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5854654062
Thanks for the thorough review! All points addressed in 81af1e5b:
- **Schema & coercion**: Delegated top-level pair coercion in \coerce_sort_by\ to \_coerce_sort_item(v)\ to eliminate duplication, documented the pair format in the field description, and simplified \dd_xy_sort_config\ to rely on the pre-coerced \SortByConfig\.
- **Temporal handling**: Guarded \_build_single_query_dict\ against overriding chronological ordering with \x_axis_sort\ when temporal indicators (\granularity_sqla\, \ ime_grain_sqla\, or \mixed_timeseries\) are present.
- **Metric & dimension matching**: Normalized aggregate aliases (\STDDEV\ / \VAR\) in \_match_y_metric_label\ and aligned x-axis dimension targeting with \getXAxisLabel\ in \sortOperator.ts\.
- **Tests & typing**: Added docstrings and explicit local type annotations across \ est_dataset_validator.py\, \ est_chart_utils.py\, \ est_chart_helpers.py\, and \ est_chart_schemas.py\, removed the duplicate test case in \ est_chart_schemas.py\, and verified clean passes across Ruff and all 449 unit 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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5943463210
Heya @FrancescoCastaldi, LGTM still stands on the content. Looks like a big MCP refactor merged to `master` today (#44746) and that's what's conflicting here now, not leftover review stuff. Want to rebase, or should I take a stab at 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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4131857947
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `251ef2c`.
Added sort-by normalization that resolves saved metrics and Y-axis labels to canonical names before validation, preserving the saved-metric status in `SortByConfig`.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-6082695563
Hi @rusackas, @aminghadersohi,
I have merged the latest upstream master into this branch (commit 0f4150bf77) and resolved all upstream sync conflicts cleanly. The full unit test suite passes green. The branch is up to date and ready for merge whenever you have a chance.
Thanks!
Francesco
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44464:
URL: https://github.com/apache/superset/pull/44464
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5848470802
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 91d10c4b8cad9763cfdeca894318d48b33f593a9 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ab80517c97ade000897f84b |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44464--superset-docs-preview.netlify.app](https://deploy-preview-44464--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#q1YqKMrPLShRslJS0lFKKkrMS84AstMyK_Rzkwt0Kyp1kzMSi0p0i_OBRFIlUE1KakFOfqVnClCVWWKShYGpoXmypXliSqqBgYGFpXmahUmSUi0A) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44464--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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115242717
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -863,13 +898,19 @@ def build_query_dicts_from_form_data(
if x_axis_col and x_axis_col not in groupby:
groupby = [x_axis_col] + groupby
+ is_temporal = bool(
+ form_data.get("granularity_sqla")
+ or form_data.get("time_grain_sqla")
+ or viz_type == "mixed_timeseries"
+ )
Review Comment:
New Explore charts default `time_grain_sqla` to `P1D` whatever the x-axis type, so this drops `x_axis_sort` for Explore-saved categorical bar charts; the x-axis sort control already resets on temporal axes. No test covers this clause.
```suggestion
is_temporal = bool(
form_data.get("granularity_sqla") or viz_type == "mixed_timeseries"
)
```
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -699,8 +722,19 @@ def _build_single_query_dict(
# metric descending. buildQuery derives this on the frontend; translate
# the flag here when there is no explicit ordering or a row_limit truncates
# an unordered result (dropping the heaviest rows rather than the top-N).
+ viz_type = form_data.get("viz_type") or ""
+ is_temporal = (
+ is_timeseries
+ or viz_type == "mixed_timeseries"
+ or bool(form_data.get("granularity_sqla"))
+ )
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif not is_temporal and form_data.get("x_axis_sort") and not qd.get("orderby"):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ qd["orderby"] = [(sort_target, sort_asc)]
Review Comment:
Still reproduces at this head when `time_grain_sqla` is unset: a saved series-sort value (`sum`, `max`) becomes `orderby` and `get_chart_preview` fails with `Unknown column used in orderby: sum`. Only order by a real metric or column:
```suggestion
col_labels = [c if isinstance(c, str) else c.get("label") for c in columns]
if sort_target in metrics or sort_target in col_labels:
qd["orderby"] = [(sort_target, sort_asc)]
```
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115308064
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -699,8 +722,19 @@ def _build_single_query_dict(
# metric descending. buildQuery derives this on the frontend; translate
# the flag here when there is no explicit ordering or a row_limit truncates
# an unordered result (dropping the heaviest rows rather than the top-N).
+ viz_type = form_data.get("viz_type") or ""
+ is_temporal = (
+ is_timeseries
+ or viz_type == "mixed_timeseries"
+ or bool(form_data.get("granularity_sqla"))
+ )
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif not is_temporal and form_data.get("x_axis_sort") and not qd.get("orderby"):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ qd["orderby"] = [(sort_target, sort_asc)]
Review Comment:
Good catch. Updated `_build_single_query_dict` to check that `sort_target` is a valid metric or column before adding it to `orderby`, avoiding invalid series-sort aggregates from being propagated. Also added unit test `test_build_single_query_dict_x_axis_sort_ignores_unmatched_series_sort` in commit `a8ab8a7`.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: resolves the sort target to the corresponding metric label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0], ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
Review Comment:
Applied in commit `bc6d9ff`. Added check `if form_data.get("groupby"):` with warning in `add_xy_sort_config`, along with unit tests `test_group_by_sort_by_ignored_with_warning` and `test_map_xy_config_with_group_by_ignores_sort_by`.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: resolves the sort target to the corresponding metric label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0], ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
+ sort_lower = sort_entry.column.lower()
+ x_name = (config.x.name or "").lower() if config.x else None
+ x_label = (config.x.label or "").lower() if config.x else None
+
+ # If sorting by the x-axis dimension itself (case-insensitive check)
+ if config.x and (
+ (x_name and sort_lower == x_name)
+ or (x_label and sort_lower == x_label)
+ ):
+ sort_target = config.x.label or config.x.name
+ else:
+ # Match against y metrics (by column name, metric label, agg expr, or sql)
+ matched_label = _match_y_metric_label(config.y, sort_lower)
+ sort_target = matched_label or sort_entry.column
Review Comment:
Applied in commit `bc6d9ff`. If `sort_by` targets a column not matching the x-axis or a y-axis metric, `add_xy_sort_config` now warns and skips setting `x_axis_sort`, covered by unit tests `test_unmatched_sort_by_ignored_with_warning` and `test_map_xy_config_unmatched_sort_by_ignores_and_warns`.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -701,6 +723,11 @@ def _build_single_query_dict(
# an unordered result (dropping the heaviest rows rather than the top-N).
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif form_data.get("x_axis_sort") and not qd.get("orderby"):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ qd["orderby"] = [(sort_target, sort_asc)]
Review Comment:
Applied in commit `bc6d9ff`. Added `and not form_data.get("groupby")` and validated `sort_target in metrics or sort_target in col_labels` in `_build_single_query_dict`, covered by unit test `test_build_single_query_dict_x_axis_sort_ignored_when_groupby_set`.
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -863,13 +898,19 @@ def build_query_dicts_from_form_data(
if x_axis_col and x_axis_col not in groupby:
groupby = [x_axis_col] + groupby
+ is_temporal = bool(
+ form_data.get("granularity_sqla")
+ or form_data.get("time_grain_sqla")
+ or viz_type == "mixed_timeseries"
+ )
Review Comment:
Applied in commit `bc6d9ff` (and `a8ab8a7`). Removed `time_grain_sqla` from the temporal check so Explore-saved categorical bar charts preserve `x_axis_sort`, covered by unit test `test_build_query_dicts_from_form_data_xy_bar_explore_default_grain`.
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4116057314
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `81a94cc`.
Added sort-by normalization that resolves saved metrics and Y-axis labels to canonical names before validation, preserving the saved-metric status in `SortByConfig`.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4114822940
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `81af1e5`.
`sort_by` is now normalized through `_update_sort_by_column`, which resolves canonical names and marks saved metrics before downstream validation.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
+ if isinstance(first, str):
+ return SortByConfig(column=first, ascending=False)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `81af1e5`.
The validator now rejects lists with more than one sort entry by raising `ValueError`, preventing additional criteria from being silently discarded.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4112204831
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `91d10c4`.
Added a guard that skips adding `sort_by` as a separate `ColumnRef` when its name is already covered by the x-axis, y-axis, or group-by fields, preventing metric labels from being validated as physical columns.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
+ if isinstance(first, str):
+ return SortByConfig(column=first, ascending=False)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `91d10c4`.
The validator now rejects lists containing more than one sort specification with an explicit error instead of retaining only the first entry. The field description also states that multi-column sorting is unsupported for XY charts.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5849842086
Thanks a lot for the thorough review and real-dataset testing, @gabotorresruiz and @aminghadersohi!
All points raised have been addressed and covered with unit and end-to-end pipeline tests in the latest commits (`fdb3ea5`):
### 1. `x_axis_sort` resolution to metric label & `sortOperator` compatibility
- In `superset/mcp_service/chart/chart_utils.py` (`add_xy_sort_config`), the sort target is resolved against metrics in `config.y` (by name, custom label, or `sql_expression`). When matched, it derives the metric label (e.g., `SUM(sales)` or custom label `Total Sales`) matching what frontend `sortOperator.ts` expects in `sortableLabels`.
- Case-insensitivity is supported for both metric matching and dimension column matching against the x-axis.
### 2. `ValidationPipeline` & `extract_column_refs`
- In `superset/mcp_service/chart/plugins/xy.py`, `_get_covered_xy_names` and `_collect_y_metric_names` ensure that sort targets covered by `y`, `x`, or `group_by` are not emitted as raw physical columns, preventing premature `column_not_found` validation failures at Layer 2.
- For independent saved metrics not in `y`, `_extract_sort_col_info` checks `dataset_context.available_metrics` and marks `saved_metric=True` on the `ColumnRef`, allowing `DatasetValidator` to accept them.
### 3. Schema validation & multi-item rejection
- In `superset/mcp_service/chart/schemas.py`, `SortByConfig` rejects multi-item sort lists with an explicit `ValueError` stating that XY charts support at most one sort criterion.
### 4. Query Dictionary `orderby` mapping
- In `chart_helpers.py`, `_resolve_x_axis_sort_target` now resolves `x_axis_sort` to metric dictionaries in `queries[0]["orderby"]` so MCP data/preview calls also receive the expected ordering.
### 5. Tests
- Added `TestValidationPipelineWithXYChartSortBy` in `tests/unit_tests/mcp_service/chart/test_chart_utils.py` driving requests through `ValidationPipeline.validate_request_with_warnings` and `map_xy_config` to assert `form_data["x_axis_sort"] == "SUM(sales)"`, custom labels, and saved metrics.
- Added tests in `test_dataset_validator.py` verifying that unmarked saved metrics fail appropriately while marked ones pass.
- All 500 unit tests across affected suites pass, and `ruff check` passes with 0 issues.
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4058026753
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
**Suggestion:** `sort_by` is always added as a normal column reference, so saved metrics and Y-axis labels fail dataset validation before normalization can resolve them.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b2c2f4ad83a4a0fbb85b8b9a751dbc5&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=4b2c2f4ad83a4a0fbb85b8b9a751dbc5&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/chart/plugins/xy.py
**Line:** 131:145
**Comment:**
*Api Mismatch: `sort_by` is always added as a normal column reference, so saved metrics and Y-axis labels fail dataset validation before normalization can resolve them.
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%2F44464&comment_hash=410d7dac7b0123cc9c7658adc1cab8ee0a19587c3f1d4c6fbd63fafa1a4c386f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44464&comment_hash=410d7dac7b0123cc9c7658adc1cab8ee0a19587c3f1d4c6fbd63fafa1a4c386f&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
+ if isinstance(first, str):
+ return SortByConfig(column=first, ascending=False)
Review Comment:
**Suggestion:** Multi-item lists are accepted by the annotated field but only the first entry is retained, silently dropping additional sort criteria from valid requests.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=699791d2c4624e3f92af853a9620f58f&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=699791d2c4624e3f92af853a9620f58f&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/chart/schemas.py
**Line:** 2369:2376
**Comment:**
*Api Mismatch: Multi-item lists are accepted by the annotated field but only the first entry is retained, silently dropping additional sort criteria from valid requests.
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%2F44464&comment_hash=86571c200350c6b8a2fc08f6a89a02ec9d59abe3f9a821b18945b4b439f6f9f2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44464&comment_hash=86571c200350c6b8a2fc08f6a89a02ec9d59abe3f9a821b18945b4b439f6f9f2&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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4170681259
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -42,6 +43,149 @@
logger = logging.getLogger(__name__)
+def _match_y_metric_name(raw_lower: str, y_configs: list[dict[str, Any]]) -> str | None:
+ for y_col in y_configs:
+ if y_col.get("label") and y_col["label"].lower() == raw_lower:
+ return y_col["label"]
+ if y_col.get("name") and y_col["name"].lower() == raw_lower:
+ return y_col.get("label") or y_col["name"]
+ if y_col.get("sql_expression") and y_col["sql_expression"].lower() == raw_lower:
+ return y_col.get("label") or y_col["sql_expression"]
+ agg = y_col.get("aggregate")
+ name = y_col.get("name")
+ if agg and name and f"{agg}({name})".lower() == raw_lower:
+ return y_col.get("label") or f"{agg.upper()}({name})"
+ return None
+
+
+def _resolve_xy_sort_name_and_metric_status(
+ raw_name: str, config_dict: dict[str, Any], dataset_context: Any
+) -> tuple[str, bool]:
+ raw_lower = raw_name.lower()
+ if matched_y := _match_y_metric_name(raw_lower, config_dict.get("y") or []):
+ return matched_y, False
+
+ x_col = config_dict.get("x")
+ if isinstance(x_col, dict):
+ if x_col.get("label") and x_col["label"].lower() == raw_lower:
+ return x_col["label"], False
+ if x_col.get("name") and x_col["name"].lower() == raw_lower:
+ return (
+ DatasetValidator.get_canonical_column_name(
+ x_col["name"], dataset_context
+ ),
+ False,
+ )
+
+ if dataset_context and getattr(dataset_context, "available_metrics", None):
+ for m in dataset_context.available_metrics:
Review Comment:
Not a blocker, but I do not think this branch can reach a working sort.
Probing `generate_chart` against a dataset whose saved metric `total_sales` is not in `y`: `sort_by: "total_sales"` is rejected at dataset validation with `saved_metric_not_marked`, because `extract_column_refs` reads the flag off the incoming config (`saved_metric` is still `None` there) while `_normalize_xy_sort_by` only sets it in the normalization layer, which runs after validation. Taking the error's advice and sending `sort_by: {"column": "total_sales", "saved_metric": true}` does pass validation, and then `add_xy_sort_config` drops it with `sort_by='total_sales' was ignored: XY charts can only sort by the x-axis or a y-axis metric.` So both spellings end with no sort, and the error message points at the one that warns.
The contract itself looks right to me, since `sortOperator` can only sort by the x-axis label or a label that is in the query's `metrics`. But that means this `available_metrics` lookup and the `saved_metric=True` ref it produces only ever buy a warning. Would it be simpler to drop them and have the validator say the target must be the x column or a `y` metric? Or is there a path where an independent saved metric does sort that I am missing?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1268,6 +1268,111 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ aggregate_aliases = {"STDDEV": "STDDEV_SAMP", "VAR": "VAR_SAMP"}
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg = y_col.aggregate or ""
+ norm_agg = aggregate_aliases.get(agg.upper(), agg.upper())
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ agg_matches: set[str] = set()
+ if agg and y_col.name:
+ agg_matches.add(f"{agg}({y_col.name})".lower())
+ if norm_agg and y_col.name:
+ agg_matches.add(f"{norm_agg}({y_col.name})".lower())
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (sort_lower in agg_matches)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If ``group_by`` is set: records a warning and ignores ``sort_by`` because
+ x-axis sort applies only to single-series charts in Superset.
+ - If the sort target does not match the x-axis column or a y-axis metric:
+ records a warning and does not apply ``x_axis_sort``.
+ - If valid and non-temporal: resolves the sort target to the corresponding
+ metric label (or dimension column name) and sets ``form_data["x_axis_sort"]``
+ and ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if not isinstance(sort_entry, SortByConfig):
+ from superset.mcp_service.chart.schemas import _coerce_sort_item
+
+ coerced = _coerce_sort_item(sort_entry)
+ if not isinstance(coerced, SortByConfig):
+ return
+ sort_entry = coerced
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
+ if form_data.get("groupby"):
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because group_by is "
+ "set; the x-axis sort applies only to single-series charts."
+ )
+ return
+
+ sort_lower = sort_entry.column.lower()
+ x_name = (config.x.name or "").lower() if config.x else None
+ x_label = (config.x.label or "").lower() if config.x else None
+
+ # If sorting by the x-axis dimension itself (case-insensitive check)
+ if config.x and (
+ (x_name and sort_lower == x_name) or (x_label and sort_lower == x_label)
+ ):
+ sort_target = config.x.name or config.x.label
+ else:
+ # Match against y metrics (by column name, metric label, agg expr, or sql)
+ matched_label = _match_y_metric_label(config.y, sort_lower)
+ if not matched_label:
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored: XY charts can "
+ "only sort by the x-axis or a y-axis metric."
Review Comment:
Worth knowing rather than a change request: these three warnings only reach the caller from `generate_chart`, which pops `_mcp_warnings` into the response at `superset/mcp_service/chart/tool/generate_chart.py:333`. `update_chart` and `update_chart_preview` discard them instead (`update_chart.py:376` and `update_chart_preview.py:241`), so `update_chart` on a chart that already has a `group_by` accepts `sort_by`, applies nothing and says nothing.
The discard predates this PR, but these are the first `_mcp_warnings` a user would actually act on, so it may be worth carrying them through on the update path as well.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2623,6 +2642,32 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
Review Comment:
Just a small NIT on the annotation: the description promises `[column, ascending]` and `coerce_sort_by` handles it two lines below, but the list member serializes as `items: {anyOf: [string, SortByConfig]}`, so the pair form is not valid against the schema the tool advertises. I ran `XYChartConfig.model_json_schema()` through `Draft202012Validator`: `["amount", true]` fails the schema while pydantic accepts it, so a client that validates against `inputSchema` cannot send the form the description recommends.
A `Tuple[str, bool]` member in the union would line the two up, and `max_length=1` on the list member would put the single-sort-column rule in the schema too.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5855852694
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5752482911
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5969306433
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-6082712307
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; the current code addresses the reported validation and multi-sort concerns.
- XY sorting is limited to valid non-temporal, single-series targets, with warnings for ignored requests and chronological sorting preserved for temporal charts.
- Tests cover sort input formats, metric and column normalization, dataset validation, and query ordering.
**Assessed commit:** `0f4150bf7706`
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4231389169
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1086,13 +1110,30 @@ def build_single_query_dict(
order_desc if order_desc is not None else form_data.get("order_desc", True)
)
qd["orderby"] = [(sort_metric, not descending)]
+ elif (
+ not is_temporal
+ and form_data.get("x_axis_sort")
+ and not form_data.get("groupby")
+ and not qd.get("orderby")
+ ):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ col_labels = [
+ c if isinstance(c, str) else (c.get("label") or c.get("column_name"))
+ for c in columns
+ if isinstance(c, (str, dict))
+ ]
+ if sort_target in metrics or sort_target in col_labels:
+ qd["orderby"] = [(sort_target, sort_asc)]
apply_form_data_filters_to_query(qd, form_data)
granularity = form_data.get("granularity", form_data.get("granularity_sqla"))
if granularity is not None:
qd["granularity"] = granularity
return qd
+
Review Comment:
The merge left a third blank line here; `ruff-format` fails `pre-commit (current)` on it.
```suggestion
```
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -1086,13 +1110,30 @@ def build_single_query_dict(
order_desc if order_desc is not None else form_data.get("order_desc", True)
)
qd["orderby"] = [(sort_metric, not descending)]
+ elif (
Review Comment:
Unreachable since https://github.com/apache/superset/pull/43771: XY charts no longer call `build_single_query_dict`, and its four plugin callers never set `x_axis_sort` or `is_timeseries`. Consider dropping this branch, `_resolve_x_axis_sort_target` and the new param.
##########
tests/unit_tests/mcp_service/chart/test_chart_helpers.py:
##########
@@ -1515,6 +1517,276 @@ def test_shared_query_builder_keeps_mixed_timeseries_ordering_per_query(
assert form_data["orderby"] == [["count", True]]
+def test_build_single_query_dict_x_axis_sort_with_metric_label() -> None:
+ """
+ Verify build_single_query_dict maps x_axis_sort metric label to
+ query orderby.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd = build_single_query_dict(form_data, ["category"], [metric])
+ assert qd["orderby"] == [(metric, False)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_metric_column_name() -> None:
+ """
+ Verify build_single_query_dict matches x_axis_sort column name to
+ metric dict.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "sales",
+ "x_axis_sort_asc": True,
+ }
+ qd = build_single_query_dict(form_data, ["category"], [metric])
+ assert qd["orderby"] == [(metric, True)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_saved_metric() -> None:
+ """Verify build_single_query_dict handles saved string metric in x_axis_sort."""
+ form_data = {
+ "x_axis_sort": "revenue",
+ "x_axis_sort_asc": False,
+ }
+ qd = build_single_query_dict(form_data, ["category"], ["revenue"])
+ assert qd["orderby"] == [("revenue", False)]
+
+
+def test_build_single_query_dict_x_axis_sort_with_dimension_column() -> None:
+ """Verify build_single_query_dict preserves dimension column sort."""
+ form_data = {
+ "x_axis_sort": "category",
+ "x_axis_sort_asc": True,
+ }
+ qd = build_single_query_dict(form_data, ["category"], ["revenue"])
+ assert qd["orderby"] == [("category", True)]
+
+
+def test_build_single_query_dict_prefers_existing_orderby() -> None:
+ """Verify explicit orderby in form_data takes precedence over x_axis_sort."""
+ form_data = {
+ "orderby": [["count", True]],
+ "x_axis_sort": "sales",
+ "x_axis_sort_asc": False,
+ }
+ qd = build_single_query_dict(form_data, ["category"], ["sales"])
+ assert qd["orderby"] == [["count", True]]
+
+
+def test_build_single_query_dict_temporal_chart_guards_against_x_axis_sort() -> None:
+ """
+ Verify temporal charts do not override chronological order with
+ x_axis_sort.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ # Case 1: is_timeseries flag passed
+ form_data = {
+ "x_axis": "order_date",
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd = build_single_query_dict(
+ form_data, ["order_date"], [metric], is_timeseries=True
+ )
+ assert "orderby" not in qd
+
+ # Case 2: granularity_sqla present indicates temporal chart
+ form_data_temporal = {
+ "viz_type": "echarts_timeseries_line",
+ "x_axis": "order_date",
+ "granularity_sqla": "order_date",
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd_temporal = build_single_query_dict(form_data_temporal, ["order_date"], [metric])
+ assert "orderby" not in qd_temporal
+
+ # Case 3: time_grain_sqla indicates temporal chart
+ form_data_grain = {
+ "viz_type": "echarts_timeseries_bar",
+ "x_axis": "order_date",
+ "granularity_sqla": "order_date",
+ "time_grain_sqla": "P1D",
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ qd_grain = build_single_query_dict(form_data_grain, ["order_date"], [metric])
+ assert "orderby" not in qd_grain
+
+
+def test_build_query_dicts_from_form_data_xy_bar_with_x_axis_sort() -> None:
+ """
+ Verify build_query_dicts_from_form_data sets orderby for categorical
+ xy bar chart.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "viz_type": "echarts_timeseries_bar",
+ "x_axis": "category",
+ "metrics": [metric],
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ with patch(
+ "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine",
+ return_value="base",
+ ):
+ queries = build_query_dicts_from_form_data(form_data, 1, "table")
+
+ assert len(queries) == 1
+ col = queries[0]["columns"][0]
+ assert (
+ col if isinstance(col, str) else col.get("sqlExpression") or col.get("label")
+ ) == "category"
+ assert queries[0]["metrics"] == [metric]
+ assert queries[0]["orderby"] == [[metric, False]]
+
+
+def test_resolve_x_axis_sort_target_case_insensitive_label() -> None:
+ """Verify _resolve_x_axis_sort_target matches metric label case-insensitively."""
+ metric = {
+ "label": "Total Sales",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ resolved = _resolve_x_axis_sort_target("total sales", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_case_insensitive_column() -> None:
+ """
+ Verify _resolve_x_axis_sort_target matches metric column name
+ case-insensitively.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "Sales"},
+ }
+ resolved = _resolve_x_axis_sort_target("sales", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_sql_expression() -> None:
+ """Verify _resolve_x_axis_sort_target matches adhoc SQL expression metric."""
+ metric = {
+ "expressionType": "SQL",
+ "sqlExpression": "COUNT(DISTINCT user_id)",
+ "label": "unique_users",
+ }
+ resolved = _resolve_x_axis_sort_target("count(distinct user_id)", [metric])
+ assert resolved == metric
+
+
+def test_resolve_x_axis_sort_target_string_metric_case_insensitive() -> None:
+ """
+ Verify _resolve_x_axis_sort_target matches bare string metric
+ case-insensitively.
+ """
+ resolved = _resolve_x_axis_sort_target("totalsales", ["TotalSales"])
+ assert resolved == "TotalSales"
+
+
+def test_build_single_query_dict_x_axis_sort_ignores_unmatched_series_sort() -> None:
+ """
+ Verify build_single_query_dict does not set orderby when x_axis_sort
+ contains a series-sort value (like 'sum' or 'max') that is neither
+ a metric nor a column, preventing 'Unknown column used in orderby: sum'.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "x_axis_sort": "sum",
+ "x_axis_sort_asc": False,
+ }
+ qd = build_single_query_dict(form_data, ["category"], [metric])
+ assert "orderby" not in qd
+
+ # Also test with 'max'
+ form_data_max = {
+ "x_axis_sort": "max",
+ "x_axis_sort_asc": True,
+ }
+ qd_max = build_single_query_dict(form_data_max, ["category"], [metric])
+ assert "orderby" not in qd_max
+
+
+def test_build_query_dicts_from_form_data_xy_bar_explore_default_grain() -> None:
+ """
+ Verify explore default time_grain_sqla ('P1D') on categorical bar chart
+ (without granularity_sqla) does not cause x_axis_sort to be dropped.
+ """
+ metric = {
+ "label": "SUM(sales)",
+ "aggregate": "SUM",
+ "column": {"column_name": "sales"},
+ }
+ form_data = {
+ "viz_type": "echarts_timeseries_bar",
+ "x_axis": "category",
+ "time_grain_sqla": "P1D",
+ "metrics": [metric],
+ "x_axis_sort": "SUM(sales)",
+ "x_axis_sort_asc": False,
+ }
+ with patch(
+ "superset.mcp_service.chart.chart_helpers.resolve_datasource_engine",
+ return_value="base",
+ ):
+ queries = build_query_dicts_from_form_data(form_data, 1, "table")
+
+ assert len(queries) == 1
+ assert queries[0]["columns"] == ["category"]
+ assert queries[0]["metrics"] == [metric]
+ assert queries[0]["orderby"] == [(metric, False)]
Review Comment:
Fails at this head: since https://github.com/apache/superset/pull/43771 XY queries come from `form_data_query_context`, so columns are axis dicts and the sort is a post-processing op (`orderby` stays metric-desc even when ascending).
```suggestion
col = queries[0]["columns"][0]
assert (col if isinstance(col, str) else col.get("label")) == "category"
assert queries[0]["metrics"] == [metric]
sort_op = {"operation": "sort", "options": {"ascending": False, "by": "SUM(sales)"}}
assert sort_op in queries[0]["post_processing"]
```
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5902614723
@aminghadersohi / @gabotorresruiz please take another spin and see how it looks now :D
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5857868771
Thanks @aminghadersohi and @gabotorresruiz!
All feedback from review [pullrequestreview-5330791205](https://github.com/apache/superset/pull/44464#pullrequestreview-5330791205) has been addressed:
- **Schema Size & Budget**: In commit `81a94cc`, the `sort_by` field description in `XYChartConfig` was updated to the concise wording suggested by @aminghadersohi. This brought the `generate_chart` schema to 49,978 B (within the 50,000 B threshold in `test_tool_inventory.py`).
- **CI Status**: All 71 CI check runs (including `unit-tests (current)`, `pre-commit`, `cypress-matrix`, `playwright-tests`, and DB test suites) are now **100% green**.
- **Automated Reviews**: Both CodeAnt and Bito review bots report 0 actionable suggestions remaining.
The branch is clean, up to date with `master`, and ready for re-review and approval when you have a moment. Thanks again for the detailed guidance throughout this 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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4116821671
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2569,6 +2588,34 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), a "
+ "[column, ascending] pair, or a single-item list containing either. "
+ "Multi-column sorting is not supported for XY charts."
+ ),
Review Comment:
Thanks a lot for spotting this and for the suggestion @aminghadersohi! Applied in commit \81a94cc\ and verified: all unit-tests and the full CI suite are now 100% green.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2569,6 +2588,34 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), a "
+ "[column, ascending] pair, or a single-item list containing either. "
+ "Multi-column sorting is not supported for XY charts."
+ ),
Review Comment:
Thanks a lot for spotting this and for the suggestion @aminghadersohi! Applied in commit `81a94cc` and verified: all unit-tests and the full CI suite are now 100% green.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-6083847268
Thanks @aminghadersohi for catching both merge leftovers.
Both points have been resolved in commit d8a08f65f8:
1. Removed the extra third blank line in `superset/mcp_service/chart/chart_helpers.py`, bringing it back to standard PEP 8 spacing so `ruff format --check` passes cleanly.
2. Updated `test_build_query_dicts_from_form_data_xy_bar_explore_default_grain` in `tests/unit_tests/mcp_service/chart/test_chart_helpers.py` to validate against the post-PR #43771 contract, checking the axis dict column structure and the sort operator inside `post_processing`.
Regarding `build_single_query_dict`: good catch that XY charts now flow entirely through `form_data_query_context` via `build_query_objects_from_form_data`. If you prefer pruning that branch and helper from `build_single_query_dict` completely, let me know and I will gladly drop it.
All relevant unit tests (337 schema and util tests, plus all sort tests) pass locally.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5967902846
Addressed review suggestions in commit [fca3591](https://github.com/apache/superset/commit/fca3591ae6eb722b5e28a9b2c342f567b57da001):
1. `superset/mcp_service/chart/schemas.py`: updated `XYChartConfig.sort_by` annotation to `SortByConfig | str | Tuple[str, bool] | Annotated[List[SortByConfig | str], Field(max_length=1)] | None`. This ensures `[col, asc]` validates cleanly against `Draft202012Validator` and advertises the single-sort-column constraint (`maxItems=1`) in the generated JSON schema.
2. `tests/unit_tests/mcp_service/chart/test_chart_schemas.py`: added `test_sort_by_pair_format_valid_against_json_schema` to verify schema validation for the pair format.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4173108094
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1095,6 +1095,10 @@ class SortByConfig(UnknownFieldCheckMixin):
description="Sort ascending. Defaults to False (descending) to match "
"the typical sort-by-metric top-N use case.",
)
+ saved_metric: bool | None = Field(
Review Comment:
Required `unit-tests` fails: https://github.com/apache/superset/pull/44573 (on master) bans null schema defaults and this one lands in `$defs/SortByConfig`. After merging master, add `OmittedMeansUnchanged` to `SortByConfig`'s bases (no fence: that line is outside the diff).
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4112763601
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `fdb3ea5`.
`_normalize_xy_sort_by` now passes `sort_by` through `_update_sort_by_column`, which resolves canonical names and saved-metric status before further processing.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
+ if isinstance(first, str):
+ return SortByConfig(column=first, ascending=False)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `fdb3ea5`.
The `sort_by` validator now raises `ValueError` when a list contains more than one sort entry, preventing additional criteria from being silently discarded.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5851117273
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115310541
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `a8ab8a7`.
`sort_by` is now normalized through `_update_sort_by_column`, which resolves canonical names and marks saved metrics before downstream validation.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2350,6 +2350,38 @@ class XYChartConfig(BaseChartConfig):
ge=1,
le=10000,
)
+ sort_by: SortByConfig | str | List[SortByConfig | str] | None = Field(
+ None,
+ description=(
+ "Sort specification for the chart. Accepts a SortByConfig object, "
+ "a bare column/metric name string (defaults to descending), or a "
+ "single-item list containing either."
+ ),
+ validation_alias=AliasChoices("sort_by", "x_axis_sort", "order_by"),
+ )
+
+ @field_validator("sort_by", mode="before")
+ @classmethod
+ def coerce_sort_by(cls, v: Any) -> Any:
+ """Coerce bare string, dict, or single-item list into SortByConfig."""
+ if v is None:
+ return None
+ if isinstance(v, str):
+ return SortByConfig(column=v, ascending=False)
+ if isinstance(v, list):
+ if not v:
+ return None
+ first = v[0]
+ if isinstance(first, str):
+ return SortByConfig(column=first, ascending=False)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `a8ab8a7`.
The validator now rejects lists with more than one sort entry by raising `ValueError`, preventing additional criteria from being silently discarded.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115334811
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `bc6d9ff`.
Added `_normalize_xy_sort_by`, which resolves each scalar or first list sort entry through `_update_sort_by_column` so saved metrics and canonical Y-axis names are normalized before validation.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5752511826
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44464?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 `10.14493%` with `62 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 56.34%. Comparing base ([`4511c13`](https://app.codecov.io/gh/apache/superset/commit/4511c1381930ec53cd74c1e25c3ad4397768e2b4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ccf02c4`](https://app.codecov.io/gh/apache/superset/commit/ccf02c4ef2d37405a322c6a4e1ac84fbc8e05957?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44464?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/chart/plugins/xy.py](https://app.codecov.io/gh/apache/superset/pull/44464?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fplugins%2Fxy.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvcGx1Z2lucy94eS5weQ==) | 7.69% | [24 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/chart\_utils.py](https://app.codecov.io/gh/apache/superset/pull/44464?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fchart_utils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvY2hhcnRfdXRpbHMucHk=) | 4.76% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/schemas.py](https://app.codecov.io/gh/apache/superset/pull/44464?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvc2NoZW1hcy5weQ==) | 18.18% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44464?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 (4511c13) and HEAD (ccf02c4). Click for more details.
>
> <details><summary>HEAD has 119 uploads less than BASE</summary>
>
>| Flag | BASE (4511c13) | HEAD (ccf02c4) |
>|------|------|------|
>|python|104|1|
>|hive|17|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44464 +/- ##
===========================================
- Coverage 80.65% 56.34% -24.31%
===========================================
Files 2942 2942
Lines 175692 175760 +68
Branches 40788 40811 +23
===========================================
- Hits 141705 99033 -42672
- Misses 31325 76212 +44887
+ Partials 2662 515 -2147
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44464/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/44464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.13% <10.14%> (-0.03%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44464/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/44464/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/44464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.13% <10.14%> (-47.84%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44464/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/44464/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/44464?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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5890214702
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4113849706
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -701,6 +723,11 @@ def _build_single_query_dict(
# an unordered result (dropping the heaviest rows rather than the top-N).
if form_data.get("sort_by_metric") and metrics and not qd.get("orderby"):
qd["orderby"] = [(metrics[0], False)]
+ elif form_data.get("x_axis_sort") and not qd.get("orderby"):
+ sort_col = form_data["x_axis_sort"]
+ sort_asc = bool(form_data.get("x_axis_sort_asc", False))
+ sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
+ qd["orderby"] = [(sort_target, sort_asc)]
Review Comment:
Saved multi-series charts store a series-sort type here (`sum`, `name`, `max`…), which becomes `orderby=[("sum", …)]`; `get_chart_preview` then fails with `Unknown column used in orderby: sum` (fine at the merge base). Gate on what `sortOperator` can sort by:
```suggestion
elif (
form_data.get("x_axis_sort")
and not form_data.get("groupby")
and not qd.get("orderby")
):
sort_col = form_data["x_axis_sort"]
sort_target = _resolve_x_axis_sort_target(sort_col, metrics)
if sort_target in metrics or sort_target in columns:
sort_asc = bool(form_data.get("x_axis_sort_asc", False))
qd["orderby"] = [(sort_target, sort_asc)]
```
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: resolves the sort target to the corresponding metric label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0], ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
Review Comment:
With `group_by`, `sortOperator` skips the sort (`isEmpty(groupby)`), so `sort_by` is a silent no-op in Explore while the MCP query still orders by it. Warn like the temporal case:
```suggestion
if form_data.get("groupby"):
form_data.setdefault("_mcp_warnings", []).append(
f"sort_by='{sort_entry.column}' was ignored because group_by is "
"set; the x-axis sort applies only to single-series charts."
)
return
```
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: resolves the sort target to the corresponding metric label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0], ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
+ sort_lower = sort_entry.column.lower()
+ x_name = (config.x.name or "").lower() if config.x else None
+ x_label = (config.x.label or "").lower() if config.x else None
+
+ # If sorting by the x-axis dimension itself (case-insensitive check)
+ if config.x and (
+ (x_name and sort_lower == x_name)
+ or (x_label and sort_lower == x_label)
+ ):
+ sort_target = config.x.label or config.x.name
+ else:
+ # Match against y metrics (by column name, metric label, agg expr, or sql)
+ matched_label = _match_y_metric_label(config.y, sort_lower)
+ sort_target = matched_label or sort_entry.column
Review Comment:
A saved metric or column not in `y` lands in `x_axis_sort`, but `sortOperator` only sorts by the x-axis or a query metric label, so the saved chart stays unsorted while the MCP query orders by it (`ORDER BY profit` on a grouped query). Warn and skip like the temporal case:
```suggestion
matched_label = _match_y_metric_label(config.y, sort_lower)
if not matched_label:
form_data.setdefault("_mcp_warnings", []).append(
f"sort_by='{sort_entry.column}' was ignored: XY charts can "
"only sort by the x-axis or a y-axis metric."
)
return
sort_target = matched_label
```
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5887559292
@rusackas db5e1b4e slipped in while re-checking the older review round and re-applied two suggestions from the 09-21 review whose threads were already superseded and resolved. It also rejected the `[column, ascending]` pair documented in the `sort_by` description, dropped the covered-name/`saved_metric` handling in `extract_column_refs`, and broke pre-commit (E501 + a ruff-format diff).
Reverted in 251ef2c, so the tree is identical to 81a94cc again. Scoped mcp_service suite: 552 passed, ruff clean. CI is rerunning on this head.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on code in PR #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4115334902
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: resolves the sort target to the corresponding metric label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0], ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
Review Comment:
Applied in commit \c6d9ff\. Added check \if form_data.get(\groupby\):\ with warning in \dd_xy_sort_config\, along with unit tests \ est_group_by_sort_by_ignored_with_warning\ and \ est_map_xy_config_with_group_by_ignores_sort_by\.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1110,6 +1110,99 @@ def add_orientation_config(form_data: Dict[str, Any], config: XYChartConfig) ->
form_data["orientation"] = config.orientation
+def _match_y_metric_label(y_cols: list[ColumnRef], sort_lower: str) -> str | None:
+ """Find matching metric label for sort_by among Y-axis metrics."""
+ for y_col in y_cols:
+ metric_obj = create_metric_object(y_col)
+ metric_label = (
+ metric_obj
+ if isinstance(metric_obj, str)
+ else (metric_obj.get("label") or "")
+ )
+ agg_expr = (
+ f"{y_col.aggregate}({y_col.name})".lower()
+ if (y_col.aggregate and y_col.name)
+ else None
+ )
+ col_name = (y_col.name or "").lower()
+ col_label = (y_col.label or "").lower()
+ sql_expr = (y_col.sql_expression or "").lower()
+ metric_label_lower = metric_label.lower()
+
+ if (
+ sort_lower == col_name
+ or sort_lower == metric_label_lower
+ or (col_label and sort_lower == col_label)
+ or (agg_expr and sort_lower == agg_expr)
+ or (sql_expr and sort_lower == sql_expr)
+ ):
+ return metric_label
+ return None
+
+
+def add_xy_sort_config(
+ form_data: Dict[str, Any], config: XYChartConfig, x_is_temporal: bool
+) -> None:
+ """Apply sort configuration to form_data for XY charts.
+
+ When ``config.sort_by`` is present:
+ - If ``x_is_temporal``: records a warning in ``form_data["_mcp_warnings"]``
+ and does not override temporal sorting.
+ - If non-temporal: resolves the sort target to the corresponding metric label
+ (or dimension column name) and sets ``form_data["x_axis_sort"]`` and
+ ``form_data["x_axis_sort_asc"]``.
+ When ``config.sort_by`` is not specified, maintains existing default behavior.
+ """
+ if not config.sort_by:
+ return
+
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, (list, tuple)):
+ if not sort_entry:
+ return
+ if (
+ len(sort_entry) == 2
+ and isinstance(sort_entry[0], str)
+ and isinstance(sort_entry[1], bool)
+ ):
+ sort_entry = SortByConfig(column=sort_entry[0], ascending=sort_entry[1])
+ else:
+ sort_entry = sort_entry[0]
+ if isinstance(sort_entry, str):
+ sort_entry = SortByConfig(column=sort_entry, ascending=False)
+ elif isinstance(sort_entry, dict):
+ sort_entry = SortByConfig(**sort_entry)
+
+ if x_is_temporal:
+ x_name = config.x.name if config.x else "x"
+ form_data.setdefault("_mcp_warnings", []).append(
+ f"sort_by='{sort_entry.column}' was ignored because the x-axis "
+ f"column '{x_name}' is temporal. Temporal charts sort "
+ f"chronologically by the time axis."
+ )
+ return
+
+ sort_lower = sort_entry.column.lower()
+ x_name = (config.x.name or "").lower() if config.x else None
+ x_label = (config.x.label or "").lower() if config.x else None
+
+ # If sorting by the x-axis dimension itself (case-insensitive check)
+ if config.x and (
+ (x_name and sort_lower == x_name)
+ or (x_label and sort_lower == x_label)
+ ):
+ sort_target = config.x.label or config.x.name
+ else:
+ # Match against y metrics (by column name, metric label, agg expr, or sql)
+ matched_label = _match_y_metric_label(config.y, sort_lower)
+ sort_target = matched_label or sort_entry.column
Review Comment:
Applied in commit \c6d9ff\. If \sort_by\ targets a column not matching the x-axis or a y-axis metric, \dd_xy_sort_config\ now warns and skips setting \x_axis_sort\, covered by unit tests \ est_unmatched_sort_by_ignored_with_warning\ and \ est_map_xy_config_unmatched_sort_by_ignores_and_warns\.
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5857355526
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5859034593
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): add sort_by option to XYChartConfig (#42627) [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 #44464:
URL: https://github.com/apache/superset/pull/44464#discussion_r4173196762
##########
superset/mcp_service/chart/plugins/xy.py:
##########
@@ -103,6 +128,21 @@ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
if config.filters:
for f in config.filters:
refs.append(ColumnRef(name=f.column))
+ if config.sort_by:
+ sort_entry = config.sort_by
+ if isinstance(sort_entry, list) and sort_entry:
+ sort_entry = sort_entry[0]
+ sort_col = (
+ sort_entry.column
+ if isinstance(sort_entry, SortByConfig)
+ else sort_entry
+ if isinstance(sort_entry, str)
+ else sort_entry.get("column")
+ if isinstance(sort_entry, dict)
+ else None
+ )
+ if sort_col:
+ refs.append(ColumnRef(name=sort_col))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `ee9a0b4`.
`sort_by` is now normalized through `_update_sort_by_column`, which resolves the canonical column or metric name and preserves saved-metric status before further processing.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): add sort_by option to XYChartConfig (#42627) [superset]
Posted by "FrancescoCastaldi (via GitHub)" <gi...@apache.org>.
FrancescoCastaldi commented on PR #44464:
URL: https://github.com/apache/superset/pull/44464#issuecomment-5969253973
@aminghadersohi merged master to bring in #44573 and updated SortByConfig bases to include OmittedMeansUnchanged in commit ee9a0b4c61.
--
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