codeant-ai-for-open-source[bot] commented on code in PR #38403: URL: https://github.com/apache/superset/pull/38403#discussion_r2885159194
########## tests/unit_tests/mcp_service/chart/test_big_number_chart.py: ########## @@ -0,0 +1,471 @@ +# 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. + +"""Tests for Big Number chart type support in MCP service.""" + +import pytest + +from superset.mcp_service.chart.chart_utils import ( + _resolve_viz_type, + analyze_chart_capabilities, + analyze_chart_semantics, + generate_chart_name, + map_big_number_config, + map_config_to_form_data, +) +from superset.mcp_service.chart.schemas import ( + BigNumberChartConfig, + ColumnRef, + FilterConfig, +) +from superset.mcp_service.chart.validation.schema_validator import ( + SchemaValidator, +) + + +class TestBigNumberChartConfig: + """Test BigNumberChartConfig Pydantic schema.""" + + def test_minimal_config(self) -> None: + config = BigNumberChartConfig( + chart_type="big_number", + metric=ColumnRef(name="revenue", aggregate="SUM"), + ) + assert config.chart_type == "big_number" + assert config.metric.name == "revenue" + assert config.metric.aggregate == "SUM" + assert config.show_trendline is False + assert config.header_only is True + + def test_with_trendline(self) -> None: + config = BigNumberChartConfig( + chart_type="big_number", + metric=ColumnRef(name="revenue", aggregate="SUM"), + temporal_column="ds", + show_trendline=True, + ) + assert config.show_trendline is True + assert config.header_only is False + assert config.temporal_column == "ds" + + def test_trendline_without_temporal_column_fails(self) -> None: + with pytest.raises(ValueError, match="requires 'temporal_column'"): + BigNumberChartConfig( + chart_type="big_number", + metric=ColumnRef(name="revenue", aggregate="SUM"), + show_trendline=True, + ) + + def test_metric_without_aggregate_fails(self) -> None: Review Comment: **Suggestion:** The BigNumberChartConfig constructor raises a Pydantic ValidationError when validation fails (missing temporal_column, missing aggregate, invalid compare_lag, or extra fields), but these tests are asserting for ValueError, so they will not catch the actual exception and will fail; update the tests to expect pydantic.ValidationError and import it accordingly. [type error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ BigNumberChartConfig negative-path tests fail immediately. - ⚠️ MCP big number chart test suite cannot pass CI. - ⚠️ Validation semantics for invalid configs not correctly asserted. ``` </details> ```suggestion from pydantic import ValidationError from superset.mcp_service.chart.chart_utils import ( _resolve_viz_type, analyze_chart_capabilities, analyze_chart_semantics, generate_chart_name, map_big_number_config, map_config_to_form_data, ) from superset.mcp_service.chart.schemas import ( BigNumberChartConfig, ColumnRef, FilterConfig, ) from superset.mcp_service.chart.validation.schema_validator import ( SchemaValidator, ) class TestBigNumberChartConfig: """Test BigNumberChartConfig Pydantic schema.""" def test_minimal_config(self) -> None: config = BigNumberChartConfig( chart_type="big_number", metric=ColumnRef(name="revenue", aggregate="SUM"), ) assert config.chart_type == "big_number" assert config.metric.name == "revenue" assert config.metric.aggregate == "SUM" assert config.show_trendline is False assert config.header_only is True def test_with_trendline(self) -> None: config = BigNumberChartConfig( chart_type="big_number", metric=ColumnRef(name="revenue", aggregate="SUM"), temporal_column="ds", show_trendline=True, ) assert config.show_trendline is True assert config.header_only is False assert config.temporal_column == "ds" def test_trendline_without_temporal_column_fails(self) -> None: with pytest.raises(ValidationError, match="requires 'temporal_column'"): BigNumberChartConfig( chart_type="big_number", metric=ColumnRef(name="revenue", aggregate="SUM"), show_trendline=True, ) def test_metric_without_aggregate_fails(self) -> None: with pytest.raises(ValidationError, match="must have an aggregate function"): ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `superset/mcp_service/chart/schemas.py:488-592` and note that `BigNumberChartConfig.validate_trendline_fields` and `validate_metric_aggregate` are `@model_validator(mode="after")` methods that `raise ValueError(...)` on invalid configs. 2. In Pydantic v2, constructing `BigNumberChartConfig(...)` wraps any `ValueError` raised in validators into a `pydantic.ValidationError` (not a bare `ValueError`) before propagating to the caller. 3. Open `tests/unit_tests/mcp_service/chart/test_big_number_chart.py:65-78` and see tests `test_trendline_without_temporal_column_fails` and `test_metric_without_aggregate_fails` using `with pytest.raises(ValueError, ...)` around invalid `BigNumberChartConfig(...)` constructions. 4. Run `pytest tests/unit_tests/mcp_service/chart/test_big_number_chart.py -v`; the invalid constructions raise `pydantic.ValidationError`, so the `pytest.raises(ValueError)` contexts fail, causing these tests (and thus the test suite for this feature) to error. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/chart/test_big_number_chart.py **Line:** 21:73 **Comment:** *Type Error: The BigNumberChartConfig constructor raises a Pydantic ValidationError when validation fails (missing temporal_column, missing aggregate, invalid compare_lag, or extra fields), but these tests are asserting for ValueError, so they will not catch the actual exception and will fail; update the tests to expect pydantic.ValidationError and import it accordingly. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=1c806cb477c92294e21649c18b582c1cf14f9da207ea7c09c4da931ad3125451&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=1c806cb477c92294e21649c18b582c1cf14f9da207ea7c09c4da931ad3125451&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -567,6 +570,53 @@ def map_xy_config( return form_data +def map_big_number_config(config: BigNumberChartConfig) -> Dict[str, Any]: + """Map big number chart config to Superset form_data.""" + # Determine viz_type: big_number (with trendline) or big_number_total + if config.show_trendline and config.temporal_column: + viz_type = "big_number" + else: + viz_type = "big_number_total" + + metric = create_metric_object(config.metric) + form_data: Dict[str, Any] = { + "viz_type": viz_type, + "metric": metric, + } + + if config.subheader: + form_data["subheader"] = config.subheader + + if config.y_axis_format: + form_data["y_axis_format"] = config.y_axis_format + + # Trendline-specific fields + if viz_type == "big_number": + form_data["x_axis"] = config.temporal_column + form_data["start_y_axis_at_zero"] = config.start_y_axis_at_zero + + if config.time_grain: + form_data["time_grain_sqla"] = config.time_grain + + if config.compare_lag is not None: + form_data["compare_lag"] = config.compare_lag + Review Comment: **Suggestion:** For big number charts with a trendline, the user-specified temporal column is written to the `x_axis` key in `form_data`, but Superset's big_number/big_number_total visualizations use `granularity_sqla` to determine the time column; this means the provided `temporal_column` will be ignored and the trendline will not respect the requested time field, or will fall back to the dataset default, producing incorrect behavior. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ MCP big_number trendline ignores configured temporal_column. - ⚠️ AI-created dashboards show misleading historical trends. ``` </details> ```suggestion # Trendline-specific fields if viz_type == "big_number": # Big Number with trendline uses granularity_sqla to select the temporal column form_data["granularity_sqla"] = config.temporal_column form_data["start_y_axis_at_zero"] = config.start_y_axis_at_zero if config.time_grain: form_data["time_grain_sqla"] = config.time_grain if config.compare_lag is not None: form_data["compare_lag"] = config.compare_lag ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In Python, import MCP chart utilities: `from superset.mcp_service.chart.chart_utils import map_config_to_form_data` and config schema: `from superset.mcp_service.chart.schemas import BigNumberChartConfig` (schema imported at `superset/mcp_service/chart/chart_utils.py:29-36`). 2. Construct a `BigNumberChartConfig` instance (defined in `superset/mcp_service/chart/schemas.py`) with `chart_type="big_number"`, `show_trendline=True`, `temporal_column="ds"` (a non-default datetime column), a valid `metric`, and optional `time_grain`, then call `map_config_to_form_data(config, dataset_id=<your_dataset_id>)` which dispatches to `map_big_number_config()` in `chart_utils.py:573`. 3. Inside `map_big_number_config` at `chart_utils.py:593-603`, observe that for `viz_type == "big_number"` the function sets `form_data["x_axis"] = config.temporal_column` but never sets `form_data["granularity_sqla"]`, so the returned `form_data` includes `x_axis` only and lacks `granularity_sqla`. 4. Use this `form_data` to generate an explore link via `generate_explore_link(dataset_id, form_data)` in `chart_utils.py:88-139` and open it in Superset; the Big Number chart backend (BigNumberViz in Superset core) reads `granularity_sqla` to choose the temporal column and ignores `x_axis`, so the trendline uses the dataset's default time column instead of `"ds"`, demonstrating that the configured `temporal_column` is not honored. ``` </details> <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:** 593:603 **Comment:** *Logic Error: For big number charts with a trendline, the user-specified temporal column is written to the `x_axis` key in `form_data`, but Superset's big_number/big_number_total visualizations use `granularity_sqla` to determine the time column; this means the provided `temporal_column` will be ignored and the trendline will not respect the requested time field, or will fall back to the dataset default, producing incorrect behavior. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=e8effdbcddaea47fd5c497ce49e7d5dfd3750bf86490d270b2bb2103f3443632&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=e8effdbcddaea47fd5c497ce49e7d5dfd3750bf86490d270b2bb2103f3443632&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]
