bito-code-review[bot] commented on code in PR #44603: URL: https://github.com/apache/superset/pull/44603#discussion_r4162412245
########## 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: <div> <div id="suggestion"> <div id="issue"><b>Missing helper docstring</b></div> <div id="fix"> New helper `_orm_dataset` carries no docstring, while sibling helper `_run_generate_chart` does. BITO adaptive rule 12147 requires docstrings on every newly introduced function, including test helpers. Adding a one-line docstring keeps the file consistent with the org standard and with its own established pattern. </div> </div> <small><i>Code Review Run #1a91c3</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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: <div> <div id="suggestion"> <div id="issue"><b>Missing helper docstring</b></div> <div id="fix"> New helper `_bar_request` lacks a docstring; BITO adaptive rule 12147 mandates docstrings for every newly introduced function, including test helpers. Sibling `_run_generate_chart` documents itself, so adding a one-liner here keeps the file aligned with both the org standard and its own pattern. </div> </div> <small><i>Code Review Run #1a91c3</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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: <div> <div id="suggestion"> <div id="issue"><b>Missing helper docstring</b></div> <div id="fix"> New helper `_ctx` lacks a docstring; BITO adaptive rule 12147 requires docstrings on all newly introduced functions, including terse test helpers. A one-line docstring naming what the Mock simulates keeps the file consistent with the org standard and with the documented `_run_generate_chart` helper. </div> </div> <small><i>Code Review Run #1a91c3</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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: <div> <div id="suggestion"> <div id="issue"><b>Duplicated response-size caps</b></div> <div id="fix"> Lines 55-56 add two local caps of 10 while `MAX_ERROR_SUGGESTIONS = 10` from `error_builder` is imported into this same module (lines 39-42) and bounds the `error.suggestions` list shipped in the same `ChartGenerationError` (line 839). Three same-valued caps now govern one error response; retuning the budget in `error_builder` silently leaves these copies stale. Consolidate or cross-reference one shared constant. </div> </div> <small><i>Code Review Run #1a91c3</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
