bito-code-review[bot] commented on code in PR #44413:
URL: https://github.com/apache/superset/pull/44413#discussion_r4048157506
##########
superset/dashboards/api.py:
##########
@@ -756,6 +757,10 @@ def _serialize_dashboard_dataset(
) -> dict[str, Any]:
"""Dump a member dataset, narrowed when the caller cannot access it."""
serialized = self.dashboard_dataset_schema.dump(payload)
+ if isinstance(datasource, SemanticView):
+ # Redux keys dashboard datasets by Slice.form_data["datasource"],
+ # whereas SemanticView.uid is the provider's independent identity.
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to use a shared helper for the datasource UID
format is technically sound for maintaining consistency and avoiding
duplication. However, your concern regarding the potential for lookup failures
due to the specific identity requirements of the dashboard store is valid.
Given that `SemanticView.uid` serves a distinct purpose as a provider identity,
you should prioritize maintaining that functional separation while still
centralizing the `{id}__{type}` format logic into a shared utility method or
property to satisfy the goal of avoiding hardcoded duplication.
##########
superset/models/dashboard.py:
##########
@@ -346,21 +347,37 @@ def data(self) -> dict[str, Any]:
def datasets_trimmed_for_slices(
self,
- ) -> list[tuple[BaseDatasource, dict[str, Any]]]:
- slices_by_datasource: dict[int, set[Slice]] = defaultdict(set)
+ ) -> list[tuple[BaseDatasource | SemanticView, dict[str, Any]]]:
+ """Return trimmed chart metadata, keeping datasource types distinct."""
+ slices_by_datasource: dict[tuple[str, int], set[Slice]] =
defaultdict(set)
for slc in self.slices:
- slices_by_datasource[slc.datasource_id].add(slc)
+ slices_by_datasource[(slc.datasource_type,
slc.datasource_id)].add(slc)
- result: list[tuple[BaseDatasource, dict[str, Any]]] = []
+ result: list[tuple[BaseDatasource | SemanticView, dict[str, Any]]] = []
for _, slices in slices_by_datasource.items():
- # Use the eagerly-loaded datasource from any slice in the group
- datasource = next(iter(slices)).datasource
+ # Resolve once per typed datasource, retaining eager-loaded tables.
+ datasource: Datasource | None =
next(iter(slices)).resolved_datasource
- if datasource:
+ if isinstance(datasource, (BaseDatasource, SemanticView)):
# Filter out unneeded fields from the datasource payload
- result.append((datasource,
datasource.data_for_slices(list(slices))))
+ try:
+ payload: dict[str, Any] = dict(
+ datasource.data_for_slices(list(slices))
+ )
+ except Exception: # noqa: BLE001
+ if not isinstance(datasource, SemanticView):
+ raise
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to narrow the `except Exception` block is
appropriate for improving error handling, as the current broad catch masks
programming errors like `AttributeError` or `TypeError`. However, as you noted,
simply narrowing the exception type without a shared failure contract between
`get_dimensions`/`get_metrics` and the provider boundary risks losing necessary
isolation for provider-specific failures. Establishing a shared exception
family or a specific provider-failure contract is a sound architectural step
before narrowing the catch block to ensure that genuine code bugs are still
propagated while expected provider discovery failures are handled gracefully.
**superset/models/dashboard.py**
```
try:
payload: dict[str, Any] = dict(
datasource.data_for_slices(list(slices))
)
except Exception: # noqa: BLE001
if not isinstance(datasource, SemanticView):
raise
```
--
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]
