gabotorresruiz commented on code in PR #45292:
URL: https://github.com/apache/superset/pull/45292#discussion_r4286833116
##########
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:
Not a blocker, and I agree the extent is narrow: no provider I/O lands in
here, `features` is a plain attribute on the ABC, and `.implementation` is
already warmed by `_validation_metadata` outside the catch.
The part I keep coming back to is that anything in that extent which is not
a deliberate refusal becomes a 400 too. A `ValueError` out of `members_by_key`
or out of `superset/utils/date_parser.py` comes back as `{"message": "Error:
..."}` with a 400 where `master` gave a 500 `GENERIC_BACKEND_ERROR`, so a
future bug in the mapper would stop registering as a server error anywhere.
Since the host already has `QueryObjectValidationError`, and
`_validate_granularity` and `_normalize_membership_filter` already raise it
directly, did you consider converting the remaining `raise ValueError` sites
instead of catching here? Happy to hear the catch was the deliberate smaller
diff.
##########
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:
Not a blocker, but I think this makes the rolling-deploy note in the
description stricter than it needs to be. `is_totals_prerequisite` is passed on
every `_schedule` call, not just the totals one, so in a web-before-worker
window it is not only contribution charts that break: every async chart-data
task hits `TypeError: execute_chart_query() got an unexpected keyword argument
'is_totals_prerequisite'` at the `executor_fn(*args, **kwargs)` call in
`superset/tasks/scheduler.py` and lands as `FAILURE`. Loud and terminal, so
nothing is silently dropped, but it is every chart rather than the contribution
ones.
Sending it only when it is true would keep the ordering requirement scoped
to the charts that actually need it:
```python
**({"is_totals_prerequisite": True} if is_totals_prerequisite else {}),
```
That needs one line in `test_contribution_query_depends_on_totals` (`assert
"is_totals_prerequisite" not in dep_call["kwargs"]`); everything else in that
module still passes. Worth it?
##########
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:
Question about the boundary this pins rather than about the test itself.
`test_mapper_metadata_fault_is_not_converted` covers both `get_metrics` and
`get_dimensions`, but the HTTP case only covers `get_metrics`. I tried
`get_dimensions` through the same route and got a 400, not a 500:
`QueryContextFactory._apply_granularity` reads `datasource.columns` during the
schema load, that calls `get_dimensions()` via
`SemanticView._unique_dimensions`, and marshmallow turns the `ValueError` into
`Request is incorrect: {'_schema': [...]}`.
Same on `master`, so nothing this PR did, but it does mean the doc's "does
not surround provider metadata discovery" only holds end to end for one of the
two discovery calls. Would you rather parametrize this one too and track the
`get_dimensions` side as its own fix, or leave the HTTP case deliberately
narrow? I might be missing a reason the asymmetry is fine.
--
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]
