aminghadersohi commented on code in PR #45484:
URL: https://github.com/apache/superset/pull/45484#discussion_r4343936981
##########
superset/mcp_service/chart/compile.py:
##########
@@ -404,43 +409,164 @@ def _active_adhoc_filters(filters: list[Any]) ->
list[dict[str, Any]]:
]
-def _classify_as_database_error(exc: BaseException, dataset_id: int) -> bool:
- """Use the dataset's DB engine spec to classify the error.
+@dataclass(frozen=True)
+class _DatabaseFailure:
+ """A database failure found in an exception's ``__cause__`` chain."""
- Walks the ``__cause__`` chain for direct ``SQLAlchemyError`` instances,
- then falls back to the engine spec's ``extract_errors`` regex patterns —
- the same classification the Superset UI uses.
+ error: BaseException
+ # SQLAlchemy errors are database failures without engine pattern matching.
+ typed: bool
+ # Found beneath a validation wrapper, as annotation query failures are.
+ wrapped: bool
+
+
+def _find_database_failure(exc: BaseException) -> _DatabaseFailure | None:
+ """Find the deepest database failure in *exc*'s ``__cause__`` chain.
+
+ Validation wrappers can carry an annotation chart's database failure, so
+ they are walked through but never classified themselves.
"""
- # Direct SQLAlchemy errors (unwrapped or in cause chain)
+ from superset.commands.chart.exceptions import (
+ ChartDataQueryFailedError,
+ ChartDataQueryValidationError,
+ )
+ from superset.db_engine_specs.exceptions import SupersetDBAPIError
+
+ # A non-validation outer error (e.g. a cache load failure) keeps the engine
+ # pattern fallback even when no typed database error is in its chain.
+ failure = (
+ None
+ if isinstance(exc, (ChartDataQueryValidationError,
QueryObjectValidationError))
+ else _DatabaseFailure(exc, typed=False, wrapped=False)
+ )
+ wrapped = False
+ seen: set[int] = set()
current: BaseException | None = exc
- while current is not None:
+ while current is not None and id(current) not in seen:
+ seen.add(id(current))
if isinstance(current, SQLAlchemyError):
- return True
+ return _DatabaseFailure(current, typed=True, wrapped=wrapped)
+ if isinstance(
+ current, (ChartDataQueryValidationError,
QueryObjectValidationError)
+ ):
+ wrapped = True
+ elif isinstance(current, (ChartDataQueryFailedError,
SupersetDBAPIError)):
+ failure = _DatabaseFailure(current, typed=False, wrapped=wrapped)
current = current.__cause__
+ return failure
- # Use the dataset's engine spec to classify (same as the UI)
- try:
- from superset.daos.dataset import DatasetDAO
- dataset = DatasetDAO.find_by_id(dataset_id)
- if dataset and dataset.database and isinstance(exc, Exception):
- errors = dataset.database.db_engine_spec.extract_errors(exc)
- return any(e.error_type in CONNECTION_ERROR_TYPES for e in errors)
+def _annotation_engine_specs(form_data: dict[str, Any]) -> tuple[list[Any],
bool]:
+ """Return the engine specs of the charts the annotation layers query.
+
+ The flag is True when the failing layer's engine is certain: every chart
+ layer resolved to the same engine. The exception does not name the layer
+ that raised it, so with several engines (or an unresolved chart) any of
+ them could be the failing one.
+ """
+ from superset.daos.chart import ChartDAO
+ from superset.utils.core import
ANNOTATION_SOURCE_TYPES_WITH_CHART_REFERENCE
+
+ layers = form_data.get("annotation_layers")
Review Comment:
Fixed in 1c8fa4b4d587a5a15d92adb4385f0d80b28efbb4. I added
`tests/unit_tests/mcp_service/chart/test_compile.py::test_compile_chart_mixed_timeseries_query_b_annotation_failure`,
covering query-B-only annotations and Postgres query-A/MySQL query-B
annotations, with both connection and syntax errors. On the previous head, 3
cases failed and 1 passed; with the fix, all 4 pass (the complete compile test
file has 80 passing tests). Both helpers use the same collector for
`annotation_layers` and `annotation_layers_b`.
Siblings checked:
- superset/mcp_service/chart/compile.py:473 — fixed in
1c8fa4b4d587a5a15d92adb4385f0d80b28efbb4
- superset/mcp_service/chart/compile.py:498 — fixed in
1c8fa4b4d587a5a15d92adb4385f0d80b28efbb4
- superset/common/form_data_query_context.py:475 — not affected: secondary
form data strips `_b` before building each query's annotation layers.
- superset/mcp_service/chart/chart_utils.py:3336 — not affected: the Mixed
Timeseries mapper preserves `annotation_layers_b`.
- superset/common/query_context_processor.py:964 — not affected: annotation
execution reads each normalized query's layers, not the original two-key form
data.
- superset/mcp_service/chart/tool/generate_chart.py:438 — not affected:
generation delegates classification to the shared compile path, including its
saved-chart path at line 676.
- superset/mcp_service/chart/tool/update_chart.py:687 — not affected: table
updates use validate-and-compile, which delegates to the shared compile path.
- superset/mcp_service/chart/tool/update_chart.py:765 — not affected:
semantic-view updates bypass table database-error classification.
- superset/mcp_service/chart/compile.py:971 — not affected:
validate-and-compile delegates to the same compile path.
Searches: `rg -n
'annotation_layers_b|_annotation_engine_specs|_has_chart_annotation_layers'
superset tests/unit_tests/mcp_service` and `rg -n '_compile_chart\('
superset/mcp_service/chart`. Query-A-only charts remain covered by
`test_compile_chart_annotation_failure_uses_annotation_engine_spec`.
--
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]
