bito-code-review[bot] commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3305705147
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -196,6 +196,29 @@ def _validate_update_against_dataset(
}
)
+ # Column existence + fuzzy-match validation
+ # (mirrors generate_chart pipeline layer 2)
+ from superset.mcp_service.chart.validation.dataset_validator import
DatasetValidator
+
+ is_col_valid, col_error = DatasetValidator.validate_against_dataset(
+ parsed_config, dataset.id
Review Comment:
<!-- Bito Reply -->
The suggestion to avoid redundant DB queries by reusing the pre-fetched
dataset is valid and appropriate. It improves the code by eliminating an
unnecessary database call, which aligns with best practices for performance
optimization. The change correctly passes the pre-fetched dataset context to
the validation function, ensuring the same logic is applied without additional
overhead.
**superset/mcp_service/chart/tool/update_chart.py**
```
- is_col_valid, col_error = DatasetValidator.validate_against_dataset(
- parsed_config, dataset.id
+ is_col_valid, col_error = DatasetValidator.validate_against_dataset(
+ parsed_config, dataset.id, dataset_context=dataset_context
)
```
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -390,6 +413,24 @@ async def update_chart( # noqa: C901
# config is already a typed ChartConfig | None (validated by Pydantic)
parsed_config = request.config
+ # Normalize column case to match dataset canonical names
+ # (mirrors generate_chart pipeline layer 4)
+ chart_datasource_id = getattr(chart, "datasource_id", None)
+ if parsed_config is not None and chart_datasource_id is not None:
+ from superset.mcp_service.chart.validation.dataset_validator
import (
+ DatasetValidator,
+ NORMALIZATION_EXCEPTIONS,
+ )
+
+ try:
+ parsed_config = DatasetValidator.normalize_column_names(
+ parsed_config, chart.datasource_id
+ )
+ except NORMALIZATION_EXCEPTIONS as e:
+ logger.warning(
+ "Column normalization failed for chart %s: %s", chart.id, e
+ )
Review Comment:
<!-- Bito Reply -->
The suggestion in the PR comment is valid and appropriate to apply. It
correctly identifies a gap in test coverage for the new column normalization
logic in the `update_chart` function. The suggestion recommends adding unit
tests to verify both the normalization of column names and the handling of
exceptions during normalization, which are critical for ensuring the
reliability and correctness of the feature. Implementing these tests in the
`TestUpdateChartValidationGate` suite, as suggested, is a logical and
maintainable approach.
**superset/mcp_service/chart/tool/update_chart.py**
```
# Normalize column case to match dataset canonical names
# (mirrors generate_chart pipeline layer 4)
chart_datasource_id = getattr(chart, "datasource_id", None)
if parsed_config is not None and chart_datasource_id is not None:
from superset.mcp_service.chart.validation.dataset_validator
import (
DatasetValidator,
NORMALIZATION_EXCEPTIONS,
)
try:
parsed_config = DatasetValidator.normalize_column_names(
parsed_config, chart.datasource_id
)
except NORMALIZATION_EXCEPTIONS as e:
logger.warning(
"Column normalization failed for chart %s: %s",
chart.id, e
)
```
--
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]
