codeant-ai-for-open-source[bot] commented on code in PR #39922: URL: https://github.com/apache/superset/pull/39922#discussion_r3501359647
########## superset/mcp_service/chart/plugins/pivot_table.py: ########## @@ -0,0 +1,164 @@ +# 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. + +"""Pivot table chart type plugin.""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _pivot_table_what, + _summarize_filters, + map_pivot_table_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, PivotTableChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class PivotTableChartPlugin(BaseChartPlugin): + """Plugin for pivot_table chart type.""" + + chart_type = "pivot_table" + display_name = "Pivot Table" + native_viz_types: ClassVar[Mapping[str, str]] = { + "pivot_table_v2": "Pivot Table", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if not (config.get("rows") or config.get("groupby") or config.get("dimension")): + missing_fields.append("'rows' (row grouping columns)") + if not config.get("metrics"): + missing_fields.append("'metrics' (aggregation metrics)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_pivot_fields", + message=( + f"Pivot table missing required fields: {', '.join(missing_fields)}" + ), + details="Pivot tables require row groupings and metrics", + suggestions=[ + "Add 'rows' field: [{'name': 'category'}]", + "Add 'metrics' field: [{'name': 'sales', 'aggregate': 'SUM'}]", + "Optional 'columns' for cross-tabulation: [{'name': 'region'}]", + ], + error_code="MISSING_PIVOT_FIELDS", + ) + + rows_val = ( + config.get("rows") or config.get("groupby") or config.get("dimension") or [] + ) Review Comment: **Suggestion:** Add a type annotation for this derived variable to make the expected structure explicit and satisfy variable type-hint coverage. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This derived variable is used as a list-like value in a type check, but it is declared without any type hint. It is a relevant local variable that could be annotated. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1cf9f6b468a9452dafc3dc70991c5deb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1cf9f6b468a9452dafc3dc70991c5deb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/plugins/pivot_table.py **Line:** 71:73 **Comment:** *Custom Rule: Add a type annotation for this derived variable to make the expected structure explicit and satisfy variable type-hint coverage. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=ef4e83afd14272182a268adf08834df523728880a4731e9f2fe408f21f839f85&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=ef4e83afd14272182a268adf08834df523728880a4731e9f2fe408f21f839f85&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/plugins/table.py: ########## @@ -0,0 +1,136 @@ +# 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. + +"""Table chart type plugin.""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _summarize_filters, + _table_chart_what, + map_table_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, TableChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class TableChartPlugin(BaseChartPlugin): + """Plugin for table chart type.""" + + chart_type = "table" + display_name = "Table" Review Comment: **Suggestion:** Add an explicit type annotation for this class-level string attribute (for example as a class variable) to satisfy the required type-hint rule. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new class-level string attribute is also unannotated, so it violates the rule requiring type hints for newly added Python variables that can be annotated. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ff91b291a9684969abb4dd36c6b6658b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ff91b291a9684969abb4dd36c6b6658b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/plugins/table.py **Line:** 40:40 **Comment:** *Custom Rule: Add an explicit type annotation for this class-level string attribute (for example as a class variable) to satisfy the required type-hint rule. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=eb27f40eaa57626cd8c11aac9fca9a162f40b6861a81d77d70d649b2bddbd891&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=eb27f40eaa57626cd8c11aac9fca9a162f40b6861a81d77d70d649b2bddbd891&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/plugins/pivot_table.py: ########## @@ -0,0 +1,163 @@ +# 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. + +"""Pivot table chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _pivot_table_what, + _summarize_filters, + map_pivot_table_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, PivotTableChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class PivotTableChartPlugin(BaseChartPlugin): + """Plugin for pivot_table chart type.""" + + chart_type = "pivot_table" + display_name = "Pivot Table" + native_viz_types = { + "pivot_table_v2": "Pivot Table", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if not (config.get("rows") or config.get("groupby") or config.get("dimension")): + missing_fields.append("'rows' (row grouping columns)") + if not config.get("metrics"): + missing_fields.append("'metrics' (aggregation metrics)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_pivot_fields", + message=( + f"Pivot table missing required fields: {', '.join(missing_fields)}" + ), + details="Pivot tables require row groupings and metrics", + suggestions=[ + "Add 'rows' field: [{'name': 'category'}]", + "Add 'metrics' field: [{'name': 'sales', 'aggregate': 'SUM'}]", + "Optional 'columns' for cross-tabulation: [{'name': 'region'}]", + ], + error_code="MISSING_PIVOT_FIELDS", + ) + + rows_val = ( + config.get("rows") or config.get("groupby") or config.get("dimension") or [] + ) + if not isinstance(rows_val, list): + return ChartGenerationError( + error_type="invalid_rows_format", + message="Rows must be a list of columns", + details="The 'rows' field must be an array of column specifications", + suggestions=[ + "Wrap row columns in array: 'rows': [{'name': 'category'}]", + ], + error_code="INVALID_ROWS_FORMAT", + ) + + if not isinstance(config.get("metrics", []), list): + return ChartGenerationError( + error_type="invalid_metrics_format", + message="Metrics must be a list", + details="The 'metrics' field must be an array of metric specifications", + suggestions=[ + "Wrap metrics in array: 'metrics': [{'name': 'sales', " + "'aggregate': 'SUM'}]", + ], + error_code="INVALID_METRICS_FORMAT", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, PivotTableChartConfig): + return [] Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not require docstrings for methods whose behavior is already self-evident from the method name and type/protocol definition; only flag missing docstrings when the 'why' or contract is non-obvious. **Applied to:** - `**/*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/mcp_service/chart/plugins/handlebars.py: ########## @@ -0,0 +1,193 @@ +# 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. + +"""Handlebars chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _handlebars_chart_what, + _summarize_filters, + map_handlebars_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, HandlebarsChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class HandlebarsChartPlugin(BaseChartPlugin): + """Plugin for handlebars chart type (custom HTML template charts).""" + + chart_type = "handlebars" + display_name = "Handlebars (Custom Template)" + native_viz_types = { + "handlebars": "Custom Template Chart", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + if "handlebars_template" not in config: + return ChartGenerationError( + error_type="missing_handlebars_template", + message="Handlebars chart missing required field: handlebars_template", + details=( + "Handlebars charts require a 'handlebars_template' string " + "containing Handlebars HTML template markup" + ), + suggestions=[ + "Add 'handlebars_template' with a Handlebars HTML template", + "Data is available as {{data}} array in the template", + "Example: '<ul>{{#each data}}<li>{{this.name}}: " + "{{this.value}}</li>{{/each}}</ul>'", + ], + error_code="MISSING_HANDLEBARS_TEMPLATE", + ) + + template = config.get("handlebars_template") + if not isinstance(template, str) or not template.strip(): + return ChartGenerationError( + error_type="invalid_handlebars_template", + message="Handlebars template must be a non-empty string", + details=( + "The 'handlebars_template' field must be a non-empty string " + "containing valid Handlebars HTML template markup" + ), + suggestions=[ + "Ensure handlebars_template is a non-empty string", + "Example: '<ul>{{#each data}}<li>{{this.name}}</li>{{/each}}</ul>'", + ], + error_code="INVALID_HANDLEBARS_TEMPLATE", + ) + + query_mode = config.get("query_mode", "aggregate") + if query_mode not in ("aggregate", "raw"): + return ChartGenerationError( + error_type="invalid_query_mode", + message="Invalid query_mode for handlebars chart", + details="query_mode must be either 'aggregate' or 'raw'", + suggestions=[ + "Use 'aggregate' for aggregated data (default)", + "Use 'raw' for individual rows", + ], + error_code="INVALID_QUERY_MODE", + ) + + if query_mode == "raw" and not config.get("columns"): + return ChartGenerationError( + error_type="missing_raw_columns", + message="Handlebars chart in 'raw' mode requires 'columns'", + details=( + "When query_mode is 'raw', you must specify which columns " + "to include in the query results" + ), + suggestions=[ + "Add 'columns': [{'name': 'column_name'}] for raw mode", + "Or use query_mode='aggregate' with 'metrics' and optional 'groupby'", # noqa: E501 + ], + error_code="MISSING_RAW_COLUMNS", + ) + + if query_mode == "aggregate" and not config.get("metrics"): + return ChartGenerationError( + error_type="missing_aggregate_metrics", + message="Handlebars chart in 'aggregate' mode requires 'metrics'", + details=( + "When query_mode is 'aggregate' (default), you must specify " + "at least one metric with an aggregate function" + ), + suggestions=[ + "Add 'metrics': [{'name': 'column', 'aggregate': 'SUM'}]", + "Or use query_mode='raw' with 'columns' for individual rows", + ], + error_code="MISSING_AGGREGATE_METRICS", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, HandlebarsChartConfig): + return [] + refs: list[ColumnRef] = [] + if config.columns: + refs.extend(config.columns) + if config.metrics: + refs.extend(config.metrics) + if config.groupby: + refs.extend(config.groupby) + if config.filters: + for f in config.filters: + refs.append(ColumnRef(name=f.column)) + return refs + + def to_form_data( + self, config: Any, dataset_id: int | str | None = None + ) -> dict[str, Any]: + return map_handlebars_config(config) + + def generate_name(self, config: Any, dataset_name: str | None = None) -> str: + what = _handlebars_chart_what(config) + context = _summarize_filters(getattr(config, "filters", None)) + return self._with_context(what, context) + + def resolve_viz_type(self, config: Any) -> str: + return "handlebars" + + def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any: + config_dict = config.model_dump() + + def _norm_list(key: str) -> None: Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not flag missing docstrings for short, self-evident Python methods whose behavior is clear from their names and surrounding protocol/contract definitions. **Applied to:** - `**/*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/mcp_service/chart/plugins/pivot_table.py: ########## @@ -0,0 +1,163 @@ +# 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. + +"""Pivot table chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _pivot_table_what, + _summarize_filters, + map_pivot_table_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, PivotTableChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class PivotTableChartPlugin(BaseChartPlugin): + """Plugin for pivot_table chart type.""" + + chart_type = "pivot_table" + display_name = "Pivot Table" + native_viz_types = { + "pivot_table_v2": "Pivot Table", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if not (config.get("rows") or config.get("groupby") or config.get("dimension")): + missing_fields.append("'rows' (row grouping columns)") + if not config.get("metrics"): + missing_fields.append("'metrics' (aggregation metrics)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_pivot_fields", + message=( + f"Pivot table missing required fields: {', '.join(missing_fields)}" + ), + details="Pivot tables require row groupings and metrics", + suggestions=[ + "Add 'rows' field: [{'name': 'category'}]", + "Add 'metrics' field: [{'name': 'sales', 'aggregate': 'SUM'}]", + "Optional 'columns' for cross-tabulation: [{'name': 'region'}]", + ], + error_code="MISSING_PIVOT_FIELDS", + ) + + rows_val = ( + config.get("rows") or config.get("groupby") or config.get("dimension") or [] + ) + if not isinstance(rows_val, list): + return ChartGenerationError( + error_type="invalid_rows_format", + message="Rows must be a list of columns", + details="The 'rows' field must be an array of column specifications", + suggestions=[ + "Wrap row columns in array: 'rows': [{'name': 'category'}]", + ], + error_code="INVALID_ROWS_FORMAT", + ) + + if not isinstance(config.get("metrics", []), list): + return ChartGenerationError( + error_type="invalid_metrics_format", + message="Metrics must be a list", + details="The 'metrics' field must be an array of metric specifications", + suggestions=[ + "Wrap metrics in array: 'metrics': [{'name': 'sales', " + "'aggregate': 'SUM'}]", + ], + error_code="INVALID_METRICS_FORMAT", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, PivotTableChartConfig): + return [] + refs: list[ColumnRef] = list(config.rows) + refs.extend(config.metrics) + if config.columns: + refs.extend(config.columns) + if config.filters: + for f in config.filters: + refs.append(ColumnRef(name=f.column)) + return refs + + def to_form_data( + self, config: Any, dataset_id: int | str | None = None + ) -> dict[str, Any]: + return map_pivot_table_config(config) Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not add short comments/docstrings for public methods when the intent is already self-evident from the method name and Protocol contract; only document methods when the 'why' or behavior is non-obvious. **Applied to:** - `**/*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/advanced_data_type/plugins/internet_address.py: ########## @@ -66,9 +66,9 @@ def cidr_func(req: AdvancedDataTypeRequest) -> AdvancedDataTypeResponse: else: resp["display_value"] = ", ".join( map( # noqa: C417 - lambda x: ( - f"{x['start']} - {x['end']}" if isinstance(x, dict) else str(x) - ), + lambda x: f"{x['start']} - {x['end']}" + if isinstance(x, dict) + else str(x), Review Comment: **Suggestion:** Replace the inline lambda with a named helper function that has explicit parameter and return type hints, then pass that typed function to `map`. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new lambda is an introduced Python function-like construct without any type hints. Under the rule requiring type hints on new or modified Python code where annotations are possible, this is a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=38ca74f36c9e4ce2a2b2cbbea32a0d59&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=38ca74f36c9e4ce2a2b2cbbea32a0d59&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/advanced_data_type/plugins/internet_address.py **Line:** 69:71 **Comment:** *Custom Rule: Replace the inline lambda with a named helper function that has explicit parameter and return type hints, then pass that typed function to `map`. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=f1b1607b48caa881d636008c21d74aef12e5b51188d53cf809171f484cc7094a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=f1b1607b48caa881d636008c21d74aef12e5b51188d53cf809171f484cc7094a&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -353,29 +353,44 @@ def map_config_to_form_data( | BigNumberChartConfig, dataset_id: int | str | None = None, ) -> Dict[str, Any]: - """Map chart config to Superset form_data.""" - if isinstance(config, TableChartConfig): - return map_table_config(config) - elif isinstance(config, XYChartConfig): - return map_xy_config(config, dataset_id=dataset_id) - elif isinstance(config, PieChartConfig): - return map_pie_config(config) - elif isinstance(config, PivotTableChartConfig): - return map_pivot_table_config(config) - elif isinstance(config, MixedTimeseriesChartConfig): - return map_mixed_timeseries_config(config, dataset_id=dataset_id) - elif isinstance(config, HandlebarsChartConfig): - return map_handlebars_config(config) - elif isinstance(config, BigNumberChartConfig): - if config.show_trendline and config.temporal_column: - if not is_column_truly_temporal(config.temporal_column, dataset_id): - raise ValueError( - f"Big Number trendline requires a temporal SQL column; " - f"'{config.temporal_column}' is not temporal." - ) - return map_big_number_config(config) - else: - raise ValueError(f"Unsupported config type: {type(config)}") + """Map chart config to Superset form_data via the plugin registry. + + The previous if/elif chain across all 7 chart types has been replaced by a + single registry lookup. Cross-field constraints (e.g. BigNumber trendline + temporal check) are now owned by each plugin's post_map_validate() method + rather than being baked into this dispatcher. + """ + # Local import: plugins call map_*_config from their to_form_data() methods, + # so chart_utils is loaded before plugins finish registering. A top-level + # import of registry here would trigger plugin loading mid-import = cycle. + from superset.mcp_service.chart.registry import get_registry + + chart_type = getattr(config, "chart_type", None) + plugin = get_registry().get(chart_type) if chart_type else None Review Comment: **Suggestion:** Add an explicit type annotation for this local registry lookup variable to satisfy the type-hint requirement for relevant variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is a newly added local variable in modified Python code, and it is untyped even though it can be annotated. That matches the type-hint rule for relevant variables. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4fb973d4cc9b44c39e1f75bea9865e02&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4fb973d4cc9b44c39e1f75bea9865e02&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <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:** 369:369 **Comment:** *Custom Rule: Add an explicit type annotation for this local registry lookup variable to satisfy the type-hint requirement for relevant variables. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=6620835c209a0c81b15c56810baa65d0ddb6cf826b91680f91d687884791d33d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=6620835c209a0c81b15c56810baa65d0ddb6cf826b91680f91d687884791d33d&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -353,29 +353,44 @@ def map_config_to_form_data( | BigNumberChartConfig, dataset_id: int | str | None = None, ) -> Dict[str, Any]: - """Map chart config to Superset form_data.""" - if isinstance(config, TableChartConfig): - return map_table_config(config) - elif isinstance(config, XYChartConfig): - return map_xy_config(config, dataset_id=dataset_id) - elif isinstance(config, PieChartConfig): - return map_pie_config(config) - elif isinstance(config, PivotTableChartConfig): - return map_pivot_table_config(config) - elif isinstance(config, MixedTimeseriesChartConfig): - return map_mixed_timeseries_config(config, dataset_id=dataset_id) - elif isinstance(config, HandlebarsChartConfig): - return map_handlebars_config(config) - elif isinstance(config, BigNumberChartConfig): - if config.show_trendline and config.temporal_column: - if not is_column_truly_temporal(config.temporal_column, dataset_id): - raise ValueError( - f"Big Number trendline requires a temporal SQL column; " - f"'{config.temporal_column}' is not temporal." - ) - return map_big_number_config(config) - else: - raise ValueError(f"Unsupported config type: {type(config)}") + """Map chart config to Superset form_data via the plugin registry. + + The previous if/elif chain across all 7 chart types has been replaced by a + single registry lookup. Cross-field constraints (e.g. BigNumber trendline + temporal check) are now owned by each plugin's post_map_validate() method + rather than being baked into this dispatcher. + """ + # Local import: plugins call map_*_config from their to_form_data() methods, + # so chart_utils is loaded before plugins finish registering. A top-level + # import of registry here would trigger plugin loading mid-import = cycle. + from superset.mcp_service.chart.registry import get_registry + + chart_type = getattr(config, "chart_type", None) + plugin = get_registry().get(chart_type) if chart_type else None + + if plugin is None: + if chart_type is None: + raise ValueError(f"Unsupported config type: {type(config)}") + raise ValueError( + f"Unsupported config type: {type(config)} (chart_type={chart_type!r})" + ) + + form_data = plugin.to_form_data(config, dataset_id=dataset_id) Review Comment: **Suggestion:** Add an explicit type annotation to this mapped form-data variable so the new logic keeps complete typing coverage. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This modified code introduces a local variable that can be annotated but is left untyped. It is a relevant variable in new Python logic, so the type-hint rule applies. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7963d3d3b0cb4627a39b4515b2b9974d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7963d3d3b0cb4627a39b4515b2b9974d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <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:** 378:378 **Comment:** *Custom Rule: Add an explicit type annotation to this mapped form-data variable so the new logic keeps complete typing coverage. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=ce676436246c5203003a519bc6fde7626659cd9525c937cdc86bcba16cc4651d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=ce676436246c5203003a519bc6fde7626659cd9525c937cdc86bcba16cc4651d&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -1279,87 +1294,32 @@ def _big_number_chart_what(config: BigNumberChartConfig) -> str: def generate_chart_name( - config: TableChartConfig - | XYChartConfig - | PieChartConfig - | PivotTableChartConfig - | MixedTimeseriesChartConfig - | HandlebarsChartConfig - | BigNumberChartConfig, + config: Any, dataset_name: str | None = None, ) -> str: """Generate a descriptive chart name following a standard format. - Format conventions (by chart type): - Aggregated (bar/scatter with group_by): [Metric] by [Dimension] - Time-series (line/area, no group_by): [Metric] Over Time - Table (no aggregates): [Dataset] Records - Table (with aggregates): [Metric] Summary - Pie: [Dimension] by [Metric] - Pivot Table: Pivot Table – [Row1, Row2] - Mixed Timeseries: [Primary] + [Secondary] - An en-dash followed by context (filters / time grain) is appended + Delegates to each plugin's ``generate_name()`` method. + See each plugin's ``generate_name`` for chart-type-specific format conventions. + An en-dash followed by context (filters / time grain) is appended by the plugin when such information is available. """ - if isinstance(config, TableChartConfig): - what = _table_chart_what(config, dataset_name) - context = _summarize_filters(config.filters) - elif isinstance(config, XYChartConfig): - what = _xy_chart_what(config) - context = _xy_chart_context(config) - elif isinstance(config, PieChartConfig): - what = _pie_chart_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, PivotTableChartConfig): - what = _pivot_table_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, MixedTimeseriesChartConfig): - what = _mixed_timeseries_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, HandlebarsChartConfig): - what = _handlebars_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - elif isinstance(config, BigNumberChartConfig): - what = _big_number_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - else: - return "Chart" + from superset.mcp_service.chart.registry import get_registry - name = what - if context: - name = f"{what} \u2013 {context}" - return _truncate(name) + plugin = get_registry().get(getattr(config, "chart_type", "")) Review Comment: **Suggestion:** Annotate this newly introduced local plugin variable with an explicit type instead of leaving it untyped. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new local lookup variable is unannotated even though it is type-annotatable. The rule explicitly asks for type hints on relevant variables in modified Python code. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aeb6e36ce9e14593abdb0d01901fe715&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aeb6e36ce9e14593abdb0d01901fe715&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <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:** 1309:1309 **Comment:** *Custom Rule: Annotate this newly introduced local plugin variable with an explicit type instead of leaving it untyped. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=935f2f6d607eb6c5de943167a6a0e57cb28a2b7bbe3721aa136fd7edbb9a5378&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=935f2f6d607eb6c5de943167a6a0e57cb28a2b7bbe3721aa136fd7edbb9a5378&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/chart_utils.py: ########## @@ -1279,87 +1294,32 @@ def _big_number_chart_what(config: BigNumberChartConfig) -> str: def generate_chart_name( - config: TableChartConfig - | XYChartConfig - | PieChartConfig - | PivotTableChartConfig - | MixedTimeseriesChartConfig - | HandlebarsChartConfig - | BigNumberChartConfig, + config: Any, dataset_name: str | None = None, ) -> str: """Generate a descriptive chart name following a standard format. - Format conventions (by chart type): - Aggregated (bar/scatter with group_by): [Metric] by [Dimension] - Time-series (line/area, no group_by): [Metric] Over Time - Table (no aggregates): [Dataset] Records - Table (with aggregates): [Metric] Summary - Pie: [Dimension] by [Metric] - Pivot Table: Pivot Table – [Row1, Row2] - Mixed Timeseries: [Primary] + [Secondary] - An en-dash followed by context (filters / time grain) is appended + Delegates to each plugin's ``generate_name()`` method. + See each plugin's ``generate_name`` for chart-type-specific format conventions. + An en-dash followed by context (filters / time grain) is appended by the plugin when such information is available. """ - if isinstance(config, TableChartConfig): - what = _table_chart_what(config, dataset_name) - context = _summarize_filters(config.filters) - elif isinstance(config, XYChartConfig): - what = _xy_chart_what(config) - context = _xy_chart_context(config) - elif isinstance(config, PieChartConfig): - what = _pie_chart_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, PivotTableChartConfig): - what = _pivot_table_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, MixedTimeseriesChartConfig): - what = _mixed_timeseries_what(config) - context = _summarize_filters(config.filters) - elif isinstance(config, HandlebarsChartConfig): - what = _handlebars_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - elif isinstance(config, BigNumberChartConfig): - what = _big_number_chart_what(config) - context = _summarize_filters(getattr(config, "filters", None)) - else: - return "Chart" + from superset.mcp_service.chart.registry import get_registry - name = what - if context: - name = f"{what} \u2013 {context}" - return _truncate(name) + plugin = get_registry().get(getattr(config, "chart_type", "")) + if plugin is None: + return "Chart" + return _truncate(plugin.generate_name(config, dataset_name)) def _resolve_viz_type(config: Any) -> str: """Resolve the Superset viz_type from a chart config object.""" - chart_type = getattr(config, "chart_type", "unknown") - if chart_type == "xy": - kind = getattr(config, "kind", "line") - viz_type_map = { - "line": "echarts_timeseries_line", - "bar": "echarts_timeseries_bar", - "area": "echarts_area", - "scatter": "echarts_timeseries_scatter", - } - return viz_type_map.get(kind, "echarts_timeseries_line") - elif chart_type == "table": - return getattr(config, "viz_type", "table") - elif chart_type == "pie": - return "pie" - elif chart_type == "pivot_table": - return "pivot_table_v2" - elif chart_type == "mixed_timeseries": - return "mixed_timeseries" - elif chart_type == "handlebars": - return "handlebars" - elif chart_type == "big_number": - show_trendline = getattr(config, "show_trendline", False) - temporal_column = getattr(config, "temporal_column", None) - return ( - "big_number" if show_trendline and temporal_column else "big_number_total" - ) - return "unknown" + from superset.mcp_service.chart.registry import get_registry + + plugin = get_registry().get(getattr(config, "chart_type", "")) Review Comment: **Suggestion:** Add a concrete type annotation for this local plugin lookup in viz-type resolution to comply with the type-hint rule. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This helper introduces another untyped local variable that could be annotated. That is a real type-hint omission under the provided rule. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f67e8aaafc31415c9d768beee2a7c1c3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f67e8aaafc31415c9d768beee2a7c1c3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <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:** 1319:1319 **Comment:** *Custom Rule: Add a concrete type annotation for this local plugin lookup in viz-type resolution to comply with the type-hint rule. 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. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=46dc03031660c112aa8b7568454753d75ff73e0adc3a3a5d394c9725efb915a8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=46dc03031660c112aa8b7568454753d75ff73e0adc3a3a5d394c9725efb915a8&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/plugins/pivot_table.py: ########## @@ -0,0 +1,163 @@ +# 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. + +"""Pivot table chart type plugin.""" + +from __future__ import annotations + +from typing import Any + +from superset.mcp_service.chart.chart_utils import ( + _pivot_table_what, + _summarize_filters, + map_pivot_table_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, PivotTableChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class PivotTableChartPlugin(BaseChartPlugin): + """Plugin for pivot_table chart type.""" + + chart_type = "pivot_table" + display_name = "Pivot Table" + native_viz_types = { + "pivot_table_v2": "Pivot Table", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + missing_fields = [] + + if not (config.get("rows") or config.get("groupby") or config.get("dimension")): + missing_fields.append("'rows' (row grouping columns)") + if not config.get("metrics"): + missing_fields.append("'metrics' (aggregation metrics)") + + if missing_fields: + return ChartGenerationError( + error_type="missing_pivot_fields", + message=( + f"Pivot table missing required fields: {', '.join(missing_fields)}" + ), + details="Pivot tables require row groupings and metrics", + suggestions=[ + "Add 'rows' field: [{'name': 'category'}]", + "Add 'metrics' field: [{'name': 'sales', 'aggregate': 'SUM'}]", + "Optional 'columns' for cross-tabulation: [{'name': 'region'}]", + ], + error_code="MISSING_PIVOT_FIELDS", + ) + + rows_val = ( + config.get("rows") or config.get("groupby") or config.get("dimension") or [] + ) + if not isinstance(rows_val, list): + return ChartGenerationError( + error_type="invalid_rows_format", + message="Rows must be a list of columns", + details="The 'rows' field must be an array of column specifications", + suggestions=[ + "Wrap row columns in array: 'rows': [{'name': 'category'}]", + ], + error_code="INVALID_ROWS_FORMAT", + ) + + if not isinstance(config.get("metrics", []), list): + return ChartGenerationError( + error_type="invalid_metrics_format", + message="Metrics must be a list", + details="The 'metrics' field must be an array of metric specifications", + suggestions=[ + "Wrap metrics in array: 'metrics': [{'name': 'sales', " + "'aggregate': 'SUM'}]", + ], + error_code="INVALID_METRICS_FORMAT", + ) + + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, PivotTableChartConfig): + return [] + refs: list[ColumnRef] = list(config.rows) + refs.extend(config.metrics) + if config.columns: + refs.extend(config.columns) + if config.filters: + for f in config.filters: + refs.append(ColumnRef(name=f.column)) + return refs + + def to_form_data( + self, config: Any, dataset_id: int | str | None = None + ) -> dict[str, Any]: + return map_pivot_table_config(config) + + def generate_name(self, config: Any, dataset_name: str | None = None) -> str: + what = _pivot_table_what(config) + context = _summarize_filters(config.filters) + return self._with_context(what, context) + + def resolve_viz_type(self, config: Any) -> str: + return "pivot_table_v2" + + def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any: + config_dict = config.model_dump() Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Avoid flagging missing docstrings for self-evident protocol methods or trivial wrappers; only require docstrings when the behavior or return structure is non-obvious. **Applied to:** - `**/*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* -- 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]
