codeant-ai-for-open-source[bot] commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r3906299302
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1065,6 +1066,155 @@ def map_histogram_config(config:
"HistogramChartConfig") -> Dict[str, Any]:
return form_data
+def _bullet_token_list(values: Sequence[str | float]) -> str:
+ """Serialize typed Bullet controls to the frontend's comma-separated
form."""
+ return ",".join(
+ format(value, "g") if isinstance(value, float) else value for value in
values
+ )
+
+
+def map_bullet_config(config: BulletChartConfig) -> Dict[str, Any]: # noqa:
C901
+ """Map typed Bullet config to ``Bullet/buildQuery`` and transformProps.
+
+ The frontend buildQuery replaces the generic query fields with exactly one
+ metric and the groupby hierarchy. Presentation controls stay in native
+ snake_case form_data; the chart plugin camelizes them for transformProps.
+ """
+ metric = create_metric_object(config.metric)
+ form_data: Dict[str, Any] = {
+ "viz_type": "bullet",
+ "metric": metric,
+ }
+
+ # Optional semantic/query fields are emitted only when explicitly supplied.
+ # This lets update_chart and update_chart_preview preserve native saved
state,
+ # while an explicit empty value still clears it through the generic merge
path.
+ if "dimensions" in config.model_fields_set:
+ form_data["groupby"] = [dimension.name for dimension in
config.dimensions or []]
+ if "row_limit" in config.model_fields_set:
+ form_data["row_limit"] = config.row_limit
+ if "time_range" in config.model_fields_set:
+ form_data["time_range"] = config.time_range
+
+ if config.order_by:
+ metric_output = metric if isinstance(metric, str) else
metric.get("label")
+ metric_targets = {
+ candidate.casefold()
+ for candidate in (
+ config.metric.name,
+ config.metric.label,
+ metric_output,
+ )
+ if candidate
+ }
+ dimensions = config.dimensions or []
+ dimension_targets = {
+ candidate.casefold(): dimension.name
+ for dimension in dimensions
+ for candidate in (dimension.name, dimension.label)
+ if candidate and dimension.name
+ }
+ form_data["orderby"] = [
+ [
+ (
+ metric
+ if order.column.casefold() in metric_targets
+ else dimension_targets[order.column.casefold()]
+ ),
+ order.ascending,
+ ]
+ for order in config.order_by
+ ]
+ elif "order_by" in config.model_fields_set:
+ form_data["orderby"] = []
+
+ presentation_fields: dict[str, tuple[str, Any]] = {
+ "ranges": ("ranges", _bullet_token_list(config.ranges)),
+ "range_labels": (
+ "range_labels",
+ _bullet_token_list(config.range_labels),
+ ),
+ "markers": ("markers", _bullet_token_list(config.markers)),
+ "marker_labels": (
+ "marker_labels",
+ _bullet_token_list(config.marker_labels),
+ ),
+ "marker_lines": (
+ "marker_lines",
+ _bullet_token_list(config.marker_lines),
+ ),
+ "marker_line_labels": (
+ "marker_line_labels",
+ _bullet_token_list(config.marker_line_labels),
+ ),
+ "y_axis_format": ("y_axis_format", config.y_axis_format),
+ "show_labels": ("show_labels", config.show_labels),
+ "show_legend": ("show_legend", config.show_legend),
+ }
+ for field_name, (form_key, value) in presentation_fields.items():
+ if field_name in config.model_fields_set:
+ form_data[form_key] = value
+
+ _add_adhoc_filters(form_data, config.filters)
+ if config.filters == [] and "filters" in config.model_fields_set:
+ form_data["adhoc_filters"] = []
+ if config.time_range and config.temporal_column:
+ _ensure_temporal_adhoc_filter(form_data, config.temporal_column)
+ for filter_ in form_data.get("adhoc_filters", []):
+ if (
+ isinstance(filter_, dict)
+ and filter_.get("operator") ==
FilterOperator.TEMPORAL_RANGE.value
+ and filter_.get("subject") == config.temporal_column
+ and filter_.get("comparator") == NO_TIME_RANGE
+ ):
+ filter_["comparator"] = config.time_range
+ return form_data
+
+
+def merge_bullet_form_data(
+ existing_form_data: Mapping[str, Any], new_form_data: Dict[str, Any]
+) -> None:
+ """Preserve omitted native Bullet controls across update tool paths.
+
+ Query roles and every UI control have an explicit typed representation.
+ Mappers emit optional fields only when the caller supplied them, so copying
+ the bounded native keys below preserves omitted state while explicit empty,
+ false, null, and zero-like values remain authoritative.
+ """
+ if (
+ existing_form_data.get("viz_type") != "bullet"
+ or new_form_data.get("viz_type") != "bullet"
+ ):
+ return
+ preserved_keys = {
+ "groupby",
+ "adhoc_filters",
+ "time_range",
+ "row_limit",
+ "orderby",
+ "ranges",
+ "range_labels",
+ "markers",
+ "marker_labels",
+ "marker_lines",
+ "marker_line_labels",
+ "y_axis_format",
+ "show_labels",
+ "show_legend",
+ MCP_DASHBOARD_TIME_FILTER_SUBJECT,
+ }
+ for key in preserved_keys:
+ if (
+ key == MCP_DASHBOARD_TIME_FILTER_SUBJECT
+ and "adhoc_filters" in new_form_data
+ ):
+ # The marker describes a mapper-generated temporal filter. Do not
+ # retain stale provenance when an explicit filter update removed
it.
+ continue
+ if key in existing_form_data and key not in new_form_data:
+ new_form_data[key] = existing_form_data[key]
Review Comment:
**Suggestion:** When only `time_range` changes and `temporal_column` is
omitted, this preserves the old temporal adhoc filter, so the query continues
using the previous time range. [logic error]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e45ee7c0599d4b999c77b048f5a1e2f2&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=e45ee7c0599d4b999c77b048f5a1e2f2&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/chart_utils.py
**Line:** 1214:1215
**Comment:**
*Logic Error: When only `time_range` changes and `temporal_column` is
omitted, this preserves the old temporal adhoc filter, so the query continues
using the previous time range.
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%2F43770&comment_hash=e7b59bb2eec1dd81f5256a9fa6f7c91bc1dbe04109613754c24dcaeb41f134fb&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43770&comment_hash=e7b59bb2eec1dd81f5256a9fa6f7c91bc1dbe04109613754c24dcaeb41f134fb&reaction=dislike'>๐</a>
##########
superset/mcp_service/chart/plugins/bullet.py:
##########
@@ -0,0 +1,272 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""ECharts Bullet chart type plugin."""
+
+from __future__ import annotations
+
+from collections.abc import Mapping
+from typing import Any, ClassVar
+
+from superset.mcp_service.chart.chart_utils import (
+ _summarize_filters,
+ map_bullet_config,
+)
+from superset.mcp_service.chart.plugin import BaseChartPlugin
+from superset.mcp_service.chart.schemas import BulletChartConfig, ColumnRef
+from superset.mcp_service.chart.validation.dataset_validator import (
+ DatasetValidator,
+ is_numeric_column,
+)
+from superset.mcp_service.common.error_schemas import (
+ ChartGenerationError,
+ DatasetContext,
+)
+
+
+def _canonical_reference(
+ name: str,
+ candidates: list[str],
+ role: str,
+) -> str:
+ """Resolve exact/casefold matches without silently choosing ambiguity."""
+ if name in candidates:
+ return name
+ matches = [
+ candidate for candidate in candidates if candidate.casefold() ==
name.casefold()
+ ]
+ if len(matches) > 1:
+ raise ValueError(
+ f"Ambiguous Bullet {role} {name!r}; exact-case matches are: "
+ f"{', '.join(matches)}"
+ )
+ return matches[0] if matches else name
+
+
+class BulletChartPlugin(BaseChartPlugin):
+ """Plugin matching ``plugin-chart-echarts/src/Bullet``."""
+
+ chart_type = "bullet"
+ display_name = "Bullet Chart"
+ native_viz_types: ClassVar[Mapping[str, str]] = {
+ "bullet": "Bullet Chart",
+ }
+
+ def pre_validate(self, config: dict[str, Any]) -> ChartGenerationError |
None:
+ if "metric" in config:
+ return None
+ return ChartGenerationError(
+ error_type="missing_bullet_fields",
+ message="Bullet chart missing required field: metric",
+ details=(
+ "A Bullet chart measures one numeric aggregate or saved/SQL
metric; "
+ "optional dimensions split it into one row per group."
+ ),
+ suggestions=[
+ "Add metric: {'name': 'revenue', 'aggregate': 'SUM'}",
+ "For a saved metric use {'name': 'revenue', 'saved_metric':
true}",
+ "Add dimensions: [{'name': 'region'}] for grouped bullet rows",
+ ],
+ error_code="MISSING_BULLET_FIELDS",
+ )
+
+ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
+ if not isinstance(config, BulletChartConfig):
+ return []
+ refs = [config.metric, *(config.dimensions or [])]
+ refs.extend(ColumnRef(name=filter_.column) for filter_ in
config.filters or [])
+ # order_by is constrained to role outputs by the schema, so those names
+ # are already represented by metric/dimension refs and must not be
+ # reinterpreted as physical columns.
+ return refs
+
+ def to_form_data(
+ self, config: Any, dataset_id: int | str | None = None
+ ) -> dict[str, Any]:
+ if not isinstance(config, BulletChartConfig):
+ raise TypeError("BulletChartPlugin requires BulletChartConfig")
+ return map_bullet_config(config)
+
+ def post_map_validate(
+ self,
+ config: Any,
+ form_data: dict[str, Any],
+ dataset_id: int | str | None = None,
+ ) -> ChartGenerationError | None:
+ """Require an unambiguous numeric metric output for Number(...)."""
+ if not isinstance(config, BulletChartConfig) or dataset_id is None:
+ return None
+ dataset_context = DatasetValidator._get_dataset_context(dataset_id)
+ if dataset_context is None:
+ return None
+
+ columns = [column["name"] for column in
dataset_context.available_columns]
+ metrics = [metric["name"] for metric in
dataset_context.available_metrics]
+ requested: list[tuple[str, list[str], str]] = []
+ if config.metric.name and not config.metric.sql_expression:
+ requested.append(
+ (
+ config.metric.name,
+ metrics if config.metric.saved_metric else columns,
+ "saved metric" if config.metric.saved_metric else "metric
column",
+ )
+ )
+ requested.extend(
+ (dimension.name or "", columns, "dimension")
+ for dimension in config.dimensions or []
+ if dimension.name
+ )
+ requested.extend(
+ (filter_.column, columns, "filter column")
+ for filter_ in config.filters or []
+ )
+ if config.temporal_column:
+ requested.append((config.temporal_column, columns, "temporal
column"))
+
+ for name, candidates, role in requested:
+ if (
+ name not in candidates
+ and sum(
+ candidate.casefold() == name.casefold() for candidate in
candidates
+ )
+ > 1
+ ):
+ return ChartGenerationError(
+ error_type="ambiguous_bullet_reference",
+ message=f"Bullet {role} {name!r} is ambiguous by case",
+ details=(
+ "Multiple dataset fields differ only by case. The
query and "
+ "frontend require an exact canonical field name."
+ ),
+ suggestions=[
+ "Use get_dataset_info and copy the exact-case field
name"
+ ],
+ error_code="AMBIGUOUS_BULLET_REFERENCE",
+ )
+
+ metric = config.metric
+ if metric.saved_metric or metric.sql_expression:
+ # Saved/SQL metric result types are determined by their
expressions;
+ # Tier-2 compile validation remains authoritative.
+ return None
+ if (metric.aggregate or "SUM") in {"COUNT", "COUNT_DISTINCT"}:
+ return None
+ column = next(
+ (
+ column
+ for column in dataset_context.available_columns
+ if column["name"].casefold() == (metric.name or "").casefold()
+ ),
+ None,
+ )
+ if column is None or is_numeric_column(column):
+ return None
Review Comment:
**Suggestion:** The numeric check matches only by casefolded name, so an
exact-case metric can be validated against the wrong case-variant column when
the dataset contains ambiguous names. [incorrect variable usage]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5b29a08035b14abdacef94ad70250107&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=5b29a08035b14abdacef94ad70250107&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/bullet.py
**Line:** 169:177
**Comment:**
*Incorrect Variable Usage: The numeric check matches only by casefolded
name, so an exact-case metric can be validated against the wrong case-variant
column when the dataset contains ambiguous names.
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%2F43770&comment_hash=55f54e7f735580b061cce00f7b9e8183cf7ec82c764158526d62ce48bac7c7e3&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43770&comment_hash=55f54e7f735580b061cce00f7b9e8183cf7ec82c764158526d62ce48bac7c7e3&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
