gabotorresruiz commented on code in PR #45147:
URL: https://github.com/apache/superset/pull/45147#discussion_r4250987686
##########
superset/datasource/api.py:
##########
@@ -290,6 +295,8 @@ def _column_values_response(
array_elements=array_elements,
search=search,
)
+ except (SemanticLayerQueryRejectedError, SemanticLayerExecutionError)
as ex:
+ return json_error_response([ex.error], status=ex.status)
Review Comment:
Small one, not a blocker: this branch answers with `{"errors": [...]}` while
the three branches below it answer with `{"message": ...}`, and the route's
docstring points 400 and 500 at `#/components/responses/400` and `/500`, both
of which are `{"message": string}`. A provider that returns a wrong shaped
result also still escapes as the bare `ValueError` in `values_for_column` and
comes back as FAB's `{"message": "Fatal error"}`, so this route now has two 500
shapes. Was envelope parity with the chart data API the goal here, or would
`self.response(ex.status, message=ex.message)` do? Happy with either, I would
just like the docstring to say which.
##########
superset/semantic_layers/models.py:
##########
@@ -552,42 +552,27 @@ def values_for_column(
a metric name included — which the endpoint reports as the caller's
error naming the column, exactly as a dataset does.
"""
- dimensions = {
+ dimensions: dict[str, Dimension] = {
dimension.name: dimension for dimension in self._unique_dimensions
}
if column_name not in dimensions:
raise KeyError(column_name)
- dimension = dimensions[column_name]
-
- if search:
- narrowing = Filter(
+ dimension: Dimension = dimensions[column_name]
+ narrowing: Filter | None = (
+ Filter(
type=PredicateType.WHERE,
column=dimension,
operator=Operator.LIKE,
value=f"%{search}%",
)
- try:
- with provider_completeness():
- result = self.implementation.get_values(dimension,
{narrowing})
- except SemanticResultCompletenessError:
- raise
- except Exception: # pylint: disable=broad-exception-caught
- # The narrowing filter is best-effort: a provider that cannot
- # apply it must degrade to the bounded first page (the picker
- # still narrows within it), never to an error — but say so,
- # or the degradation is the next silent failure.
- logger.warning(
- "Semantic view %s rejected the value-search filter on "
- "dimension %s; returning the unfiltered page",
- self.uuid,
- dimension.name,
- exc_info=True,
- )
- with provider_completeness():
- result = self.implementation.get_values(dimension, None)
- else:
- with provider_completeness():
- result = self.implementation.get_values(dimension, None)
+ if search
+ else None
+ )
+ result: SemanticResult = execute_semantic_query(
Review Comment:
This block worries me a bit. Dropping the unfiltered retry is right for the
chart path, but on this route I think it trades a working picker for a silent
one.
`narrowing` is built with `Operator.LIKE` for every dimension regardless of
`dimension.type`, and `SemanticView.data` hard codes `"filter_select": True`,
so a numeric or date dimension still gets `%12%` sent at the provider. For a
provider that refuses that filter but serves the unfiltered call, this route
goes from `200` with the bounded page plus a `WARNING` naming the view and the
dimension, to `400` with nothing logged at `INFO` or above.
That 400 is then swallowed in the UI. `isSuggestionsOutage` in
`AdhocFilterEditPopoverSimpleTabContent/index.tsx` is `status >= 500`, so
`suggestionsUnavailable` stays `false`, `helperText` is `undefined`, and the
user gets an empty dropdown reading `Type a value here`. The jest case `stays
quiet when the server refuses the request` pins exactly that, on the reasoning
that a 4xx is the caller's problem, which held when the only 400 here was the
unknown column one. And `Check its operator` is advice the user cannot act on,
since the host picked the operator.
Cheapest fix I can see is to narrow only where the dimension can take it:
```python
if search and pa.types.is_string(dimension.type)
else None
```
(with `is_large_string` too if you want the wide variants). Otherwise the
picker needs to learn about this 400, with a jest case next to that one. Either
way, could the rejection path keep a `WARNING` naming the view and the
dimension? Nothing server side records it now. Is the frontend half coming
separately, or am I misreading the flow?
--
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]
