mikebridge commented on PR #45110:
URL: https://github.com/apache/superset/pull/45110#issuecomment-6075418568
Thanks @aminghadersohi, this cross-PR read was really useful.
1. **Two translation sites.** Done in 0186c094cc, which merges master.
`execute_semantic_query()` now calls master's `provider_completeness()` around
the dispatcher. The inline core-to-host translation is gone, and `errors.py`
and the host completeness class are master's. There is one translation site.
2. **`get_values`.** For now I've documented the exclusion rather than
changed it (5a8a4f5086). The SDK `get_values` docstring and the developer doc
state that `SemanticQueryRejectedError` is not part of that method's contract,
and why: value search treats its filter as best-effort and retries without it.
Routing the values path through the same classification would change that
endpoint's behavior, so I'll take it as a follow-up.
3. **Async loses the rejection code.** I've narrowed the claim in the
description and the developer doc: completeness reasons survive async
execution, but a typed rejection in a worker shows the generic failure message.
Agreed that the fix is a closed code mapped to host text. I'll do that as a
follow-up, ideally with the client messages generated from one source so they
can't drift.
4. **Interaction with #42760.** Agreed on the boundary. Classification
belongs on the dispatcher passed into the cache, and provider exceptions
already propagate outside the cache's broad `except` blocks. The miss-path test
and adding `result_cache_discriminator` to the containment identity belong in
#42760, and I'll raise both there before it merges.
5. **Passthrough and normalization.**
- *Allowlist:* intended. It covers only the control signals the host
owns: security, OAuth2, cancellation, the worker soft time limit and
host-classified validation errors. Adapters raise standard exceptions or the
SDK types. A `SupersetException` from inside a provider isn't part of the
contract, so it is treated as a fault: a generic 500, plus an ERROR log with
the cause chained.
- *Unknown codes:* done. They are now logged at warning level (truncated)
before normalizing to `INVALID_QUERY`, with a test.
- *Bare `QueryObjectValidationError` in `charts/data/api.py`:* I checked
this. Before, one escaping `command.execute` reached the global
`SupersetException` handler, which also returned 400 (with an `error` key, or
an HTML error page for a browser `Accept` header). The status is unchanged; the
body is now a sanitized `message`, and the client error parser reads both keys.
Validation errors for ordinary datasets are mostly raised earlier, in
`command.validate()`, which already returns a 400 `message`.
On the plan: agreed. This PR now includes master. Next, #42760 should rebase
onto it and adopt the dispatcher boundary and the discriminator.
--
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]
