This is an automated email from the ASF dual-hosted git repository.
alexandrusoare pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new fb276b08ddd fix(mcp): Skip misleading trend analysis for categorical
ASCII charts (#39761)
fb276b08ddd is described below
commit fb276b08ddda9bcf9f4cdb358354b05339b0a3ff
Author: Alexandru Soare <[email protected]>
AuthorDate: Wed May 20 18:04:21 2026 +0300
fix(mcp): Skip misleading trend analysis for categorical ASCII charts
(#39761)
---
superset/mcp_service/chart/ascii_charts.py | 65 +++++++++++------
.../mcp_service/chart/test_ascii_charts.py | 83 +++++++++++++++++++++-
2 files changed, 123 insertions(+), 25 deletions(-)
diff --git a/superset/mcp_service/chart/ascii_charts.py
b/superset/mcp_service/chart/ascii_charts.py
index d8af0be3d6e..62107815f4e 100644
--- a/superset/mcp_service/chart/ascii_charts.py
+++ b/superset/mcp_service/chart/ascii_charts.py
@@ -251,7 +251,7 @@ def _generate_ascii_line_chart(data: list[Any], width: int,
height: int) -> str:
lines.append("═" * min(width, 60))
# Extract values and labels for plotting
- values, labels = _extract_time_series_data(data)
+ values, labels, is_temporal = _extract_time_series_data(data)
if not values:
return "No numeric data found for line chart"
@@ -265,7 +265,7 @@ def _generate_ascii_line_chart(data: list[Any], width: int,
height: int) -> str:
lines.extend(sparkline_data)
# Add trend analysis
- trend_analysis = _analyze_trend(values)
+ trend_analysis = _analyze_trend(values, is_temporal=is_temporal)
lines.append("")
lines.append("📊 Trend Analysis:")
lines.extend(trend_analysis)
@@ -273,10 +273,17 @@ def _generate_ascii_line_chart(data: list[Any], width:
int, height: int) -> str:
return "\n".join(lines)
-def _extract_time_series_data(data: list[Any]) -> tuple[list[float],
list[str]]:
- """Extract time series data with labels."""
+def _extract_time_series_data(
+ data: list[Any],
+) -> tuple[list[float], list[str], bool]:
+ """Extract time series data with labels.
+
+ Returns a tuple of (values, labels, is_temporal) where is_temporal
indicates
+ whether the label column appears to represent temporal data based on its
name.
+ """
values = []
labels = []
+ is_temporal = False
for row in data[:20]: # Limit points for readability
if isinstance(row, dict):
@@ -299,6 +306,7 @@ def _extract_time_series_data(data: list[Any]) ->
tuple[list[float], list[str]]:
for date_word in ["date", "time", "month", "day",
"year"]
):
label_val = str(val)[:10] # Truncate long dates
+ is_temporal = True
else:
label_val = str(val)[:8] # Truncate long strings
@@ -306,7 +314,7 @@ def _extract_time_series_data(data: list[Any]) ->
tuple[list[float], list[str]]:
values.append(numeric_val)
labels.append(label_val or f"P{len(values)}")
- return values, labels
+ return values, labels, is_temporal
def _create_enhanced_line_chart(
@@ -407,8 +415,15 @@ def _draw_line_segment(
grid[y][x] = "│"
-def _analyze_trend(values: list[float]) -> list[str]:
- """Analyze trend in the data."""
+def _analyze_trend(values: list[float], *, is_temporal: bool = True) ->
list[str]:
+ """Analyze trend in the data.
+
+ Args:
+ values: Numeric data points to analyze.
+ is_temporal: Whether the data represents a time series. When False,
+ directional trend analysis is skipped because the ordering of
+ categorical data is arbitrary.
+ """
if len(values) < 2:
return ["• Insufficient data for trend analysis"]
@@ -421,24 +436,28 @@ def _analyze_trend(values: list[float]) -> list[str]:
max_val = max(values)
avg_val = sum(values) / len(values)
- # Overall trend
- if last_val > first_val * 1.1:
- trend_icon = "📈"
- trend_desc = "Strong upward trend"
- elif last_val > first_val * 1.05:
- trend_icon = "📊"
- trend_desc = "Moderate upward trend"
- elif last_val < first_val * 0.9:
- trend_icon = "📉"
- trend_desc = "Strong downward trend"
- elif last_val < first_val * 0.95:
- trend_icon = "📊"
- trend_desc = "Moderate downward trend"
+ if is_temporal:
+ # Overall trend (only meaningful for temporally ordered data)
+ if last_val > first_val * 1.1:
+ trend_icon = "📈"
+ trend_desc = "Strong upward trend"
+ elif last_val > first_val * 1.05:
+ trend_icon = "📊"
+ trend_desc = "Moderate upward trend"
+ elif last_val < first_val * 0.9:
+ trend_icon = "📉"
+ trend_desc = "Strong downward trend"
+ elif last_val < first_val * 0.95:
+ trend_icon = "📊"
+ trend_desc = "Moderate downward trend"
+ else:
+ trend_icon = "➡️"
+ trend_desc = "Relatively stable"
+
+ analysis.append(f"• {trend_icon} {trend_desc}")
else:
- trend_icon = "➡️"
- trend_desc = "Relatively stable"
+ analysis.append("• ⚠️ Data is categorical — directional trend not
meaningful")
- analysis.append(f"• {trend_icon} {trend_desc}")
analysis.append(f"• Range: {min_val:.1f} to {max_val:.1f} (avg:
{avg_val:.1f})")
# Volatility
diff --git a/tests/unit_tests/mcp_service/chart/test_ascii_charts.py
b/tests/unit_tests/mcp_service/chart/test_ascii_charts.py
index 45371f0f07a..65b9a9e116a 100644
--- a/tests/unit_tests/mcp_service/chart/test_ascii_charts.py
+++ b/tests/unit_tests/mcp_service/chart/test_ascii_charts.py
@@ -15,9 +15,88 @@
# specific language governing permissions and limitations
# under the License.
-"""Unit tests for ascii_charts.py NaN/null value handling."""
+"""Unit tests for ASCII charts: trend analysis, NaN/null handling,
+and boolean skipping."""
-from superset.mcp_service.chart.ascii_charts import generate_ascii_chart
+from superset.mcp_service.chart.ascii_charts import (
+ _analyze_trend,
+ _extract_time_series_data,
+ generate_ascii_chart,
+)
+
+
+def test_analyze_trend_temporal_upward():
+ values = [100.0, 110.0, 120.0, 130.0, 150.0]
+ result = _analyze_trend(values, is_temporal=True)
+ trend_line = result[0]
+ assert "upward trend" in trend_line.lower()
+ assert "categorical" not in trend_line.lower()
+
+
+def test_analyze_trend_temporal_downward():
+ values = [200.0, 180.0, 150.0, 120.0, 100.0]
+ result = _analyze_trend(values, is_temporal=True)
+ trend_line = result[0]
+ assert "downward trend" in trend_line.lower()
+
+
+def test_analyze_trend_temporal_stable():
+ values = [100.0, 101.0, 100.5, 100.2, 100.8]
+ result = _analyze_trend(values, is_temporal=True)
+ trend_line = result[0]
+ assert "stable" in trend_line.lower()
+
+
+def test_analyze_trend_categorical_skips_direction():
+ values = [200.0, 180.0, 150.0, 120.0, 100.0]
+ result = _analyze_trend(values, is_temporal=False)
+ trend_line = result[0]
+ assert "categorical" in trend_line.lower()
+ assert "downward" not in trend_line.lower()
+ assert "upward" not in trend_line.lower()
+
+
+def test_analyze_trend_categorical_keeps_range_and_volatility():
+ values = [200.0, 180.0, 150.0, 120.0, 100.0]
+ result = _analyze_trend(values, is_temporal=False)
+ full_text = "\n".join(result)
+ assert "Range:" in full_text
+ assert "Volatility:" in full_text
+
+
+def test_analyze_trend_defaults_to_temporal():
+ values = [100.0, 150.0, 200.0]
+ result = _analyze_trend(values)
+ trend_line = result[0]
+ assert "categorical" not in trend_line.lower()
+ assert "upward trend" in trend_line.lower()
+
+
+def test_extract_time_series_data_temporal_column():
+ data = [
+ {"date": "2024-01-01", "value": 100},
+ {"date": "2024-02-01", "value": 200},
+ ]
+ values, labels, is_temporal = _extract_time_series_data(data)
+ assert values == [100, 200]
+ assert is_temporal is True
+
+
+def test_extract_time_series_data_categorical_column():
+ data = [
+ {"country": "Argentina", "value": 500},
+ {"country": "Brazil", "value": 300},
+ ]
+ values, labels, is_temporal = _extract_time_series_data(data)
+ assert values == [500, 300]
+ assert is_temporal is False
+
+
+def test_extract_time_series_data_time_keyword_variants():
+ for key in ["timestamp", "created_time", "month_name", "day_of_week",
"year_end"]:
+ data = [{key: "val", "metric": 42}]
+ _, _, is_temporal = _extract_time_series_data(data)
+ assert is_temporal is True, f"Expected temporal for column '{key}'"
def test_bar_chart_with_null_values_does_not_raise() -> None:
