mikebridge commented on code in PR #45292:
URL: https://github.com/apache/superset/pull/45292#discussion_r4287660494
##########
superset/tasks/async_queries.py:
##########
@@ -432,13 +441,21 @@ def _schedule(
index: int,
depends_on: "list[CoreTask | UUID | str] | None" = None,
) -> "Task":
+ is_totals_prerequisite: bool = index == totals_idx and
bool(needs_totals)
+ # Task dedup ignores arguments; keep the two failure policies distinct.
+ task_key: str | None = (
+ f"totals:{query_cache_keys[index]}"
+ if is_totals_prerequisite and query_cache_keys[index] is not None
+ else query_cache_keys[index]
+ )
return execute_chart_query.schedule(
serialized_queries[index],
user_id,
guest_token,
_needs_prerequisite_totals(index),
+ is_totals_prerequisite=is_totals_prerequisite,
Review Comment:
Good catch — adopted the true-only keyword unpack and your absent-key
assertion in
https://github.com/apache/superset/commit/e64310cd5ea02ca1e009a853aaad25d4c33e3b7e.
The assertion failed before the fix; all 295 focused tests pass afterward.
Ordinary tasks no longer send the new keyword, and the PR rollout note is
narrowed to contribution charts.
##########
tests/unit_tests/semantic_layers/query_errors_test.py:
##########
@@ -83,6 +90,150 @@ def date_view(mocker: MockerFixture) -> MagicMock:
return provider
[email protected](
+ "selection,message",
+ [
+ (
+ {"series_columns": ["ORDER_DATE"], "series_limit": 5},
+ "Group limit is not supported in this Semantic View.",
+ ),
+ (
+ {"filters": [{"col": "ORDER_DATE", "op": "==", "val":
"not-a-date"}]},
+ "Invalid date value",
+ ),
+ ],
+)
+def test_mapper_refusal_returns_400(
+ client: FlaskClient,
+ full_api_access: None,
+ date_view: MagicMock,
+ selection: dict[str, Any],
+ message: str,
+) -> None:
+ """Real validation and mapping refusals are client errors before
dispatch."""
+ response: TestResponse = client.post(
+ "/api/v1/chart/data",
+ json={
+ "datasource": {"id": 7, "type": "semantic_view"},
+ "queries": [
+ {"columns": ["ORDER_DATE"], "metrics": ["ORDER_COUNT"],
**selection}
+ ],
+ "result_format": "json",
+ "result_type": "full",
+ "force": True,
+ },
+ )
+ assert response.status_code == 400
+ assert message in response.get_data(as_text=True)
+ date_view.get_table.assert_not_called()
+
+
[email protected]("method", ["get_metrics", "get_dimensions"])
+def test_mapper_metadata_fault_is_not_converted(
+ app_context: None,
+ date_view: MagicMock,
+ method: str,
+) -> None:
+ """The mapper leaves provider metadata exceptions unchanged."""
+ view: SemanticView = SemanticView(id=7, name="Orders", configuration="{}")
+ view.__dict__["implementation"] = date_view
+ query: QueryObject = QueryObject(columns=["ORDER_DATE"],
metrics=["ORDER_COUNT"])
+ query.datasource = view
+ error: ValueError = ValueError("metadata fault")
+ getattr(date_view, method).side_effect = error
+ captured: pytest.ExceptionInfo[ValueError]
+ with pytest.raises(ValueError, match="metadata fault") as captured:
+ get_results(query)
+ assert captured.value is error
+ date_view.get_table.assert_not_called()
+
+
+def test_malformed_provider_timezone_remains_500(
+ client: FlaskClient,
+ full_api_access: None,
+ date_view: MagicMock,
+) -> None:
+ """A provider's malformed timezone is not a correctable query refusal."""
+ date_view.get_dimensions.return_value = {
+ Dimension("order_date", "ORDER_DATE", pa.timestamp("us", tz="/abs"))
+ }
+ response: TestResponse = client.post(
+ "/api/v1/chart/data",
+ json={
+ "datasource": {"id": 7, "type": "semantic_view"},
+ "queries": [
+ {
+ "columns": ["ORDER_DATE"],
+ "metrics": ["ORDER_COUNT"],
+ "filters": [{"col": "ORDER_DATE", "op": ">=", "val":
"2026-01-01"}],
+ }
+ ],
+ "result_format": "json",
+ "result_type": "full",
+ "force": True,
+ },
+ )
+ assert response.status_code == 500
+ assert "/abs" not in response.get_data(as_text=True)
+ date_view.get_table.assert_not_called()
+
+
+def test_metadata_discovery_fault_http(
+ client: FlaskClient,
+ full_api_access: None,
+ date_view: MagicMock,
+) -> None:
+ """A metadata discovery fault stays a server error over chart-data HTTP."""
+ date_view.get_metrics.side_effect = ValueError("Metadata unavailable")
Review Comment:
Agreed —
https://github.com/apache/superset/commit/e64310cd5ea02ca1e009a853aaad25d4c33e3b7e
parametrizes the HTTP test over both methods and pins the observed 500 for
get_metrics versus 400 for get_dimensions. The docs explicitly distinguish the
mapper boundary from earlier schema loading. I have proposed a follow-up for
classifying discovery faults there without changing genuine malformed-query
400s; that inherited asymmetry is not fixed by this PR.
##########
superset/semantic_layers/mapper.py:
##########
@@ -211,10 +211,15 @@ def get_results(query_object: QueryObject) -> QueryResult:
raise ValueError("QueryObject must have a datasource defined.")
query_object = cast(ValidatedQueryObject, query_object)
metadata: _ViewMetadata = _validation_metadata(query_object)
- _validate_query_object(query_object, metadata)
- # Track execution time
- start_time = current_time()
+ # Track validation, translation and execution time, excluding discovery.
+ start_time: float = current_time()
+ # Step 1: Translate the main query followed by any time-offset queries.
+ try:
+ _validate_query_object(query_object, metadata)
+ queries: list[SemanticQuery] = _map_query_object(query_object,
metadata)
+ except ValueError as ex:
Review Comment:
Yes, the catch was the deliberate smaller diff, and your future-bug concern
is valid. I rechecked the alternatives alongside
https://github.com/apache/superset/commit/e64310cd5ea02ca1e009a853aaad25d4c33e3b7e
and left this boundary unchanged: replacing explicit mapper raises alone
misses ValueErrors from time-range/date parsing (for example reversed bounds),
and removing the catch also changes direct-helper error contracts. Could we
take source-typed refusals and narrow parser adapters as a follow-up, with
tests that invalid input remains 400 while an injected unexpected mapper
ValueError stays 500? I have recorded that proposal; this PR still accepts the
masking trade-off rather than claiming it is solved.
--
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]
