sadpandajoe commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r3936359708


##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -323,6 +370,536 @@ def _generate_safe_ascii_bar_chart(data: List[Dict[str, 
Any]]) -> str:
     return "\n".join(lines)
 
 
+def _form_metric_label(metric: Any) -> str | None:
+    """Return the result-column label for a native QueryFormMetric."""
+    if type(metric) is str:
+        return metric
+    if type(metric) is not dict:
+        return None
+    if label := dict.get(metric, "label"):
+        return label if type(label) is str else None
+    if dict.get(metric, "expressionType") == "SQL":
+        expression = dict.get(metric, "sqlExpression")
+        return expression if type(expression) is str and expression else None
+    column = dict.get(metric, "column")
+    column_name = dict.get(column, "column_name") if type(column) is dict else 
column
+    aggregate = dict.get(metric, "aggregate")
+    if type(column_name) is str and type(aggregate) is str:
+        return f"{aggregate}({column_name})"
+    return None
+
+
+def _form_column_label(column: Any) -> str | None:
+    """Return the result-column label for a native QueryFormColumn."""
+    if type(column) is str:
+        return column
+    if type(column) is not dict:
+        return None
+    for key in ("label", "column_name"):
+        if type(value := dict.get(column, key)) is str and value:
+            return value
+    return None
+
+
+def _canonical_result_field(label: str | None, row: Dict[str, Any]) -> str | 
None:
+    """Resolve an exact or one unambiguous casefold result-field match."""
+    if label is None:
+        return None
+    if label in dict.keys(row):
+        return label
+    matches = [
+        field
+        for field in dict.keys(row)
+        if type(field) is str and field.casefold() == label.casefold()
+    ]
+    return matches[0] if len(matches) == 1 else None
+
+
+def _require_result_field(label: str | None, row: dict[str, Any], role: str) 
-> str:
+    """Resolve a role without falling back to an unrelated result field."""
+    if not label:
+        raise BulletOutputError(f"Bullet {role} has no declared result alias")
+    if label in dict.keys(row):
+        return label
+    matches = sorted(
+        field
+        for field in dict.keys(row)
+        if type(field) is str and field.casefold() == label.casefold()
+    )
+    if len(matches) == 1:
+        return matches[0]
+    if matches:
+        raise BulletOutputError(
+            f"Bullet {role} alias {label!r} is ambiguous; candidates: "
+            f"{', '.join(matches)}"
+        )
+    raise BulletOutputError(
+        f"Bullet {role} alias {label!r} is missing from query output"
+    )
+
+
+def _safe_enum_backing(value: Any) -> Any:
+    """Extract Enum's stored value without public descriptors/conversions."""
+    value_type = type(value)
+    try:
+        mro = type.__getattribute__(value_type, "__mro__")
+    except (AttributeError, TypeError):  # pragma: no cover - normal types 
have MRO
+        return value
+    if type(mro) is not tuple or not any(base is Enum for base in mro):
+        return value
+    try:
+        backing = object.__getattribute__(value, "_value_")
+    except Exception as ex:
+        raise BulletOutputError("Bullet output contains an unreadable enum") 
from ex
+    if not any(type(backing) is allowed for allowed in _ENUM_SCALAR_TYPES):
+        raise BulletOutputError("Bullet output contains an unsupported enum 
value")
+    return backing
+
+
+def _decimal_javascript_string(value: Decimal) -> str:
+    """Render an exact binary64 spelling with JavaScript Number thresholds."""
+    sign, digits_tuple, exponent = Decimal.as_tuple(value)
+    if type(exponent) is not int:  # finite Decimals always have an integer 
exponent
+        raise BulletOutputError("Bullet dimension contains a non-finite 
Decimal")
+    if not any(digits_tuple):
+        return "0"
+
+    digits = "".join(str(digit) for digit in digits_tuple)
+    adjusted = len(digits) + exponent - 1
+    prefix = "-" if sign else ""
+    if -6 <= adjusted < 21:
+        point = len(digits) + exponent
+        if point <= 0:
+            text = f"0.{('0' * -point)}{digits}"
+        elif point >= len(digits):
+            text = digits + ("0" * (point - len(digits)))
+        else:
+            text = f"{digits[:point]}.{digits[point:]}"
+        if "." in text:
+            text = text.rstrip("0").rstrip(".")
+        return prefix + text
+
+    fraction = digits[1:].rstrip("0")
+    coefficient = digits[0] + (f".{fraction}" if fraction else "")
+    exponent_text = f"+{adjusted}" if adjusted >= 0 else str(adjusted)
+    return f"{prefix}{coefficient}e{exponent_text}"
+
+
+def _javascript_number_string(value: int | float | Decimal) -> str:
+    """Apply JSON-number -> IEEE-754 Number -> JavaScript String semantics.
+
+    Exact result scalars can retain precision that the frontend cannot: JSON
+    parsing first rounds a numeric token to binary64, and ``String`` then emits
+    the shortest round-tripping decimal with fixed notation for exponents in
+    [-6, 20].  Converting exact builtin scalars to an exact builtin float keeps
+    the path hook-free.  Python and JavaScript use the same shortest
+    round-tripping binary64 digits; ``_decimal_javascript_string`` only adjusts
+    the notation thresholds and exponent spelling.
+
+    A finite integer or Decimal outside binary64's range becomes an infinity
+    after JSON parsing, matching JavaScript.  Non-finite source values are
+    rejected by the trusted scalar normalizer before this helper is called.
+    """
+    value_type = type(value)
+    if value_type not in {int, float, Decimal}:
+        raise BulletOutputError("Bullet dimension contains an unsupported 
number")
+    if value_type is float and not math.isfinite(value):
+        raise BulletOutputError("Bullet dimension contains a non-finite 
number")
+    if isinstance(value, Decimal) and not Decimal.is_finite(value):
+        raise BulletOutputError("Bullet dimension contains a non-finite 
Decimal")
+    try:
+        number = float(value)
+    except OverflowError:
+        number = -math.inf if value < 0 else math.inf
+
+    if math.isinf(number):
+        return "-Infinity" if number < 0 else "Infinity"
+    if number == 0:
+        # String(-0) is "0" even though JSON.parse preserves negative zero.
+        return "0"
+    return _decimal_javascript_string(Decimal(float.__repr__(number)))
+
+
+def _bullet_category_value(  # noqa: C901
+    value: Any, dimension: str, row_index: int
+) -> tuple[Any, str]:
+    """Return a JSON-safe value and bounded frontend ``String(value)`` text.
+
+    The trusted scalar normalizer is type-exact and does not dispatch through
+    application hooks.  Vega data retains the normalized Chart Data wire value
+    (including epoch-ms temporal numbers); only the derived category key and
+    ASCII label use the JavaScript-compatible text.
+    """
+    from superset.mcp_service.chart.query_result import (
+        _bounded_utf8_length,
+        _chart_data_duration_text,
+        _chart_data_temporal_number,
+        _is_chart_data_duration_scalar,
+        _is_chart_data_temporal_scalar,
+        _normalize_trusted_scalar,
+    )
+
+    normalized: Any
+    reason: str | None
+    if _is_chart_data_temporal_scalar(value):
+        normalized, reason = _chart_data_temporal_number(value)
+    elif _is_chart_data_duration_scalar(value):
+        normalized, reason = _chart_data_duration_text(value)
+    else:
+        normalized, reason = _normalize_trusted_scalar(
+            value, max_string_bytes=_MAX_BULLET_TEXT_BYTES
+        )
+    if reason is not None:
+        if reason == "contains an unsupported or subclassed value":
+            reason = "has an unsupported value type"
+        elif "oversized string" in reason:
+            reason = "exceeds the size limit"
+        raise BulletOutputError(
+            f"Bullet dimension {dimension!r} row {row_index} {reason}"
+        )
+
+    value_type = type(normalized)
+    if normalized is None:
+        text = "null"
+    elif value_type is str:
+        text = normalized
+    elif value_type is bool:
+        text = "true" if normalized else "false"
+    elif value_type is int or value_type is float or value_type is Decimal:
+        text = _javascript_number_string(normalized)
+    else:
+        raise BulletOutputError(
+            f"Bullet dimension {dimension!r} row {row_index} has an "
+            "unsupported value type"
+        )
+
+    if _bounded_utf8_length(text, _MAX_BULLET_TEXT_BYTES) is None:
+        raise BulletOutputError(
+            f"Bullet dimension {dimension!r} row {row_index} exceeds the size 
limit"
+        )
+    return normalized, text
+
+
+def _bullet_number(value: Any, row_index: int, metric_field: str) -> float:
+    """Apply the frontend's useful ``Number(value ?? 0)`` numeric subset."""
+    value = _safe_enum_backing(value)
+    if value is None:
+        number = 0.0
+    elif type(value) is bool:
+        raise BulletOutputError(
+            f"Bullet metric {metric_field!r} row {row_index} returned a 
boolean"
+        )
+    elif type(value) is int or type(value) is float or type(value) is Decimal:
+        try:
+            number = float(value)
+        except (TypeError, ValueError, OverflowError) as ex:
+            raise BulletOutputError(
+                f"Bullet metric {metric_field!r} row {row_index} is not 
numeric"
+            ) from ex
+    elif type(value) is str:
+        if len(value) > _MAX_BULLET_TEXT_BYTES:
+            raise BulletOutputError(
+                f"Bullet metric {metric_field!r} row {row_index} is not 
numeric"
+            )
+        stripped = value.strip()
+        if not stripped:
+            raise BulletOutputError(
+                f"Bullet metric {metric_field!r} row {row_index} is not 
numeric"
+            )
+        try:
+            number = float(Decimal(stripped))
+        except (InvalidOperation, ValueError, OverflowError) as ex:
+            raise BulletOutputError(
+                f"Bullet metric {metric_field!r} row {row_index} returned "
+                f"non-numeric text"
+            ) from ex
+    else:
+        raise BulletOutputError(
+            f"Bullet metric {metric_field!r} row {row_index} is not numeric"
+        )
+    if not math.isfinite(number):
+        raise BulletOutputError(
+            f"Bullet metric {metric_field!r} row {row_index} is NaN or 
infinite"
+        )
+    return number
+
+
+def _bullet_string_tokens(value: Any) -> list[str]:
+    """Parse labels exactly like the frontend's comma tokenizer."""
+    from superset.mcp_service.chart.query_result import _truncate_utf8
+
+    value = _safe_enum_backing(value)
+    if value is None:
+        return []
+    if type(value) is not str or len(value) > _MAX_BULLET_TEXT_BYTES:
+        raise BulletOutputError("Bullet labels must be a bounded 
comma-separated list")
+    if not value.strip():
+        return []
+    tokens = value.split(",")
+    if len(tokens) > _MAX_BULLET_TOKENS:
+        raise BulletOutputError("Bullet labels exceed the item limit")
+    return [_truncate_utf8(token.strip(), _MAX_BULLET_TEXT_BYTES) for token in 
tokens]
+
+
+def _unique_bullet_derived_field(
+    rows: list[dict[str, Any]], base: str, reserved: tuple[str, ...] = ()
+) -> str:
+    """Return one internal key absent from result rows and prior derived 
keys."""
+    occupied = {key for row in rows for key in dict.keys(row)}
+    occupied.update(reserved)
+    candidate = base
+    suffix = 0
+    while candidate in occupied:
+        suffix += 1
+        candidate = f"{base}_{suffix}"
+    return candidate
+
+
+def _unique_bullet_category_field(rows: list[dict[str, Any]]) -> str:
+    """Return an internal category key absent from every query-result row."""
+    return _unique_bullet_derived_field(rows, "__mcp_bullet_category")
+
+
+def _validate_bullet_format(format_: Any, values: list[float]) -> str:
+    """Reject a presentation format the backend cannot reproduce."""
+    format_ = _safe_enum_backing(format_)
+    if format_ is None or format_ == "":
+        format_ = "SMART_NUMBER"
+    if type(format_) is not str or len(format_) > 50:
+        raise BulletOutputError(
+            "Bullet number format is unsupported by previews",
+            error_type="UnsupportedFormat",
+        )
+    try:
+        for value in values:
+            _format_bullet_number(format_, value)
+    except (TypeError, ValueError, OverflowError) as ex:
+        raise BulletOutputError(
+            f"Bullet number format {format_!r} is unsupported by previews",
+            error_type="UnsupportedFormat",
+        ) from ex
+    return format_
+
+
+def _format_bullet_number(format_: str, value: float) -> str:
+    """Format finite Bullet values, including the full binary-float range."""
+    from superset.utils.number_format import format_numeric
+
+    try:
+        return format_numeric(format_, value)
+    except OverflowError:
+        # SMART_NUMBER's significant-digit rounding can overflow a finite float
+        # near DBL_MAX. Scientific repr remains deterministic and informative.
+        if format_ in {"SMART_NUMBER", "SMART_NUMBER_SIGNED"} and 
math.isfinite(value):
+            prefix = "+" if format_ == "SMART_NUMBER_SIGNED" and value > 0 
else ""
+            return prefix + repr(value)
+        raise
+
+
+def _containing_bullet_range_label(
+    measure: float, ranges: list[float], labels: list[str]
+) -> str | None:
+    """Match the frontend's labelled containing-range tooltip selection."""
+    ascending = sorted(zip(ranges, labels, strict=True), key=lambda entry: 
entry[0])
+    for threshold, label in ascending:
+        if measure <= threshold:
+            return label or None
+    if ascending and ascending[-1][1]:
+        return f"> {ascending[-1][1]}"
+    return None
+
+
+def resolve_bullet_render_model(  # noqa: C901
+    data: List[Dict[str, Any]], form_data: Dict[str, Any]
+) -> BulletRenderModel:
+    """Resolve and validate every Bullet row and presentation control."""
+    if type(data) is not list:
+        raise BulletOutputError("Bullet query output must be an array of 
objects")
+    for row_index in range(list.__len__(data)):
+        row = list.__getitem__(data, row_index)
+        if type(row) is not dict:
+            raise BulletOutputError("Bullet query output must be an array of 
objects")
+        if dict.__len__(row) > _MAX_BULLET_FIELDS:
+            raise BulletOutputError("Bullet query row exceeds the field limit")
+        for key in dict.keys(row):
+            if type(key) is not str:
+                raise BulletOutputError("Bullet query row keys must be 
strings")
+            if len(key) > _MAX_BULLET_FIELD_BYTES:
+                raise BulletOutputError("Bullet query row key exceeds the size 
limit")
+
+    if type(form_data) is not dict:
+        raise BulletOutputError("Bullet form data must be an object")
+
+    metric_label = _form_metric_label(dict.get(form_data, "metric"))
+    if not metric_label:
+        raise BulletOutputError("Bullet metric has no declared result alias")
+    raw_groupby = dict.get(form_data, "groupby")
+    if raw_groupby is None:
+        raw_groupby = []
+    if type(raw_groupby) is not list:
+        raise BulletOutputError("Bullet dimensions must be an array")
+    dimension_labels = [
+        _form_column_label(list.__getitem__(raw_groupby, index))
+        for index in range(list.__len__(raw_groupby))
+    ]
+    if any(not label for label in dimension_labels):
+        raise BulletOutputError("Bullet dimension has no declared result 
alias")
+
+    if data:
+        first_row = list.__getitem__(data, 0)
+        metric_field = _require_result_field(metric_label, first_row, "metric")
+        dimensions = [
+            _require_result_field(label, first_row, "dimension")
+            for label in dimension_labels
+        ]
+    else:
+        # The frontend accepts empty results. Ungrouped charts retain one
+        # zero-valued measure; grouped charts retain the declared roles but no
+        # categories or rows are fabricated.
+        metric_field = metric_label
+        dimensions = [label for label in dimension_labels if label is not None]
+
+    measures: list[float] = []
+    copied_rows: list[dict[str, Any]] = []
+    for index in range(list.__len__(data)):
+        row = list.__getitem__(data, index)
+        row_metric_field = _require_result_field(
+            metric_label, row, f"metric row {index}"
+        )
+        measure = _bullet_number(
+            dict.__getitem__(row, row_metric_field), index, metric_field
+        )
+        # Reserve every exact output key so the internal Vega category alias
+        # cannot collide with an unselected result field. Unselected values are
+        # deliberately replaced with None rather than converted or serialized.
+        copied: dict[str, Any] = dict.fromkeys(dict.keys(row))
+        copied[metric_field] = measure
+        for label, dimension in zip(dimension_labels, dimensions, strict=True):
+            row_dimension = _require_result_field(label, row, f"dimension row 
{index}")
+            dimension_value, _ = _bullet_category_value(
+                dict.__getitem__(row, row_dimension), dimension, index
+            )
+            copied[dimension] = dimension_value
+        copied_rows.append(copied)
+        measures.append(measure)
+
+    # The frontend validates/coerces the whole result array but renders only
+    # the first row for an ungrouped aggregate.
+    if not dimensions:
+        copied_rows = copied_rows[:1]
+        measures = measures[:1]
+        if not copied_rows:
+            copied_rows = [{metric_field: 0.0}]
+            measures = [0.0]
+
+    ranges = _strict_bullet_numeric_tokens(dict.get(form_data, "ranges"), 
"ranges")
+    if not ranges:
+        # Match Bullet/transformProps.ts: the largest measure drives one
+        # qualitative band whose upper threshold is 110% of that measure.
+        ranges = [0.0, max(measures, default=0.0) * 1.1]
+    markers = _strict_bullet_numeric_tokens(dict.get(form_data, "markers"), 
"markers")
+    marker_lines = _strict_bullet_numeric_tokens(
+        dict.get(form_data, "marker_lines"), "marker lines"
+    )
+    all_numbers = [*measures, *ranges, *markers, *marker_lines]
+    if any(not math.isfinite(value) for value in all_numbers):
+        raise BulletOutputError("Bullet presentation values must be finite")
+
+    show_labels = dict.get(form_data, "show_labels", False)
+    show_legend = dict.get(form_data, "show_legend", False)
+    if type(show_labels) is not bool or type(show_legend) is not bool:
+        raise BulletOutputError("Bullet label and legend controls must be 
booleans")
+
+    range_labels = _bullet_string_tokens(dict.get(form_data, "range_labels"))
+    marker_labels = _bullet_string_tokens(dict.get(form_data, "marker_labels"))
+    marker_line_labels = _bullet_string_tokens(
+        dict.get(form_data, "marker_line_labels")
+    )
+    for role, labels, values in (
+        ("range", range_labels, ranges),
+        ("marker", marker_labels, markers),
+        ("marker line", marker_line_labels, marker_lines),
+    ):
+        if labels and len(labels) != len(values):

Review Comment:
   This rejects a saved Bullet chart when its label list is shorter than its 
ranges or markers, although `Bullet/transformProps.ts` intentionally renders 
missing labels as blank or as the formatted value. That makes MCP previews fail 
for charts that still render in Explore. Could the preview use the same 
per-index fallback instead of requiring one label per value?



-- 
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]

Reply via email to