bito-code-review[bot] commented on code in PR #44603: URL: https://github.com/apache/superset/pull/44603#discussion_r4162530494
########## tests/unit_tests/mcp_service/chart/tool/test_column_suggestions.py: ########## @@ -0,0 +1,481 @@ +# 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. + +"""Column-error guidance uses only bounded, authorized dataset metadata.""" + +from contextlib import nullcontext +from typing import Any +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from superset.mcp_service.chart.schemas import ColumnRef, GenerateChartRequest +from superset.mcp_service.chart.tool.generate_chart import generate_chart +from superset.mcp_service.chart.validation.dataset_validator import ( + DatasetValidator, + MAX_ERROR_CONTEXT_COLUMNS, +) +from superset.mcp_service.common.error_schemas import DatasetContext +from superset.mcp_service.utils.error_builder import ChartErrorBuilder + +GET_DATASET_INFO = "Use get_dataset_info to see available columns" + + +def _orm_dataset(names: list[str]) -> Mock: Review Comment: <!-- Bito Reply --> The suggestion to add a docstring to the helper function is appropriate and improves code documentation. Since the change has been implemented and verified with tests, no further action is required. ########## tests/unit_tests/mcp_service/chart/tool/test_column_suggestions.py: ########## @@ -0,0 +1,481 @@ +# 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. + +"""Column-error guidance uses only bounded, authorized dataset metadata.""" + +from contextlib import nullcontext +from typing import Any +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from superset.mcp_service.chart.schemas import ColumnRef, GenerateChartRequest +from superset.mcp_service.chart.tool.generate_chart import generate_chart +from superset.mcp_service.chart.validation.dataset_validator import ( + DatasetValidator, + MAX_ERROR_CONTEXT_COLUMNS, +) +from superset.mcp_service.common.error_schemas import DatasetContext +from superset.mcp_service.utils.error_builder import ChartErrorBuilder + +GET_DATASET_INFO = "Use get_dataset_info to see available columns" + + +def _orm_dataset(names: list[str]) -> Mock: + return Mock( + id=268, + table_name="sales_fixture", + schema=None, + database=Mock(database_name="fixture", db_engine_spec=None), + columns=[ + Mock( + column_name=name, + type="VARCHAR", + is_temporal=False, + is_numeric=name == "revenue", + ) + for name in names + ], + metrics=[ + Mock( + metric_name="gross_margin", + expression="SUM(revenue)", + description="not a physical column", + ) + ], + ) + + +def _bar_request(column: str) -> GenerateChartRequest: Review Comment: <!-- Bito Reply --> The suggestion to add a docstring to the `_bar_request` helper is appropriate and aligns with the project's coding standards. Adding this documentation improves code maintainability and consistency with existing patterns in the file. **tests/unit_tests/mcp_service/chart/tool/test_column_suggestions.py** ``` def _bar_request(column: str) -> GenerateChartRequest: """Create an unsaved bar-chart request for the specified column.""" ``` ########## tests/unit_tests/mcp_service/chart/tool/test_column_suggestions.py: ########## @@ -0,0 +1,481 @@ +# 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. + +"""Column-error guidance uses only bounded, authorized dataset metadata.""" + +from contextlib import nullcontext +from typing import Any +from unittest.mock import AsyncMock, Mock, patch + +import pytest + +from superset.mcp_service.chart.schemas import ColumnRef, GenerateChartRequest +from superset.mcp_service.chart.tool.generate_chart import generate_chart +from superset.mcp_service.chart.validation.dataset_validator import ( + DatasetValidator, + MAX_ERROR_CONTEXT_COLUMNS, +) +from superset.mcp_service.common.error_schemas import DatasetContext +from superset.mcp_service.utils.error_builder import ChartErrorBuilder + +GET_DATASET_INFO = "Use get_dataset_info to see available columns" + + +def _orm_dataset(names: list[str]) -> Mock: + return Mock( + id=268, + table_name="sales_fixture", + schema=None, + database=Mock(database_name="fixture", db_engine_spec=None), + columns=[ + Mock( + column_name=name, + type="VARCHAR", + is_temporal=False, + is_numeric=name == "revenue", + ) + for name in names + ], + metrics=[ + Mock( + metric_name="gross_margin", + expression="SUM(revenue)", + description="not a physical column", + ) + ], + ) + + +def _bar_request(column: str) -> GenerateChartRequest: + return GenerateChartRequest.model_validate( + { + "dataset_id": 268, + "save_chart": False, + "config": { + "chart_type": "xy", + "kind": "bar", + "x": {"name": column}, + "y": [{"name": "revenue", "aggregate": "SUM"}], + }, + } + ) + + +def _ctx() -> Mock: Review Comment: <!-- Bito Reply --> The suggestion to add a one-line docstring to the `_ctx` helper function is valid and improves code clarity. Adding a brief description of what the mock simulates ensures consistency with the organization's documentation standards and the existing `_run_generate_chart` helper. **tests/unit_tests/mcp_service/chart/tool/test_column_suggestions.py** ``` def _ctx() -> Mock: """Mock the asynchronous MCP logging and progress context.""" ``` ########## superset/mcp_service/chart/validation/dataset_validator.py: ########## @@ -45,6 +50,11 @@ r"DOUBLE(?:\s+PRECISION)?|DECIMAL|NUMERIC|REAL|NUMBER|(?:SMALL)?MONEY)\b" ) +# How many dataset names a column error may carry as context. Callers that need +# the full schema are pointed at ``get_dataset_info``. +MAX_ERROR_CONTEXT_COLUMNS = 10 +MAX_ERROR_CONTEXT_METRICS = 10 Review Comment: <!-- Bito Reply --> The reviewer's suggestion to consolidate the response-size caps is valid and has been addressed. The duplicate local caps were removed, and the shared `MAX_ERROR_SUGGESTIONS` constant is now used consistently to govern error-response list limits. This change ensures that tuning the budget in the central `error_builder` module correctly propagates to all error responses, including saved-metric guidance and template list formatting. **superset/mcp_service/chart/validation/dataset_validator.py** ``` +# How many dataset names a column error may carry as context. Callers that need +# the full schema are pointed at ``get_dataset_info``. +MAX_ERROR_CONTEXT_COLUMNS = 10 +MAX_ERROR_CONTEXT_METRICS = 10 ``` -- 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]
