bito-code-review[bot] commented on code in PR #44370:
URL: https://github.com/apache/superset/pull/44370#discussion_r4050458249
##########
superset/common/query_object.py:
##########
@@ -405,6 +406,38 @@ def validate(
) -> QueryObjectValidationError | None:
"""Validate query object"""
try:
+ if self.datasource and self.datasource.type == "semantic_view":
+ try:
+ cast(
+ "SemanticView", self.datasource
+ ).implementation.validate_selection_version(
+ self.extras.get("semantic_selection_version")
+ )
+ except ValueError as ex:
+ if self.extras.get("semantic_selection_version") == (
+ "unverified-external-selections"
+ ):
Review Comment:
<!-- Bito Reply -->
The suggestion to use a named constant for the sentinel string and hoist the
lookup is appropriate. It improves code maintainability by centralizing the
cross-layer contract and avoids redundant evaluations of the same dictionary
key, which is a standard best practice.
**superset/common/query_object.py**
```
if self.datasource and self.datasource.type == "semantic_view":
try:
cast(
"SemanticView", self.datasource
).implementation.validate_selection_version(
self.extras.get("semantic_selection_version")
)
except ValueError as ex:
if self.extras.get("semantic_selection_version") == (
"unverified-external-selections"
):
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -940,6 +960,48 @@ const FiltersConfigForm = (
forceRender: true,
children: (
<>
+ <FormItem
+ hidden
+ name={['filters', filterId, 'semantic_selection_version']}
+ initialValue={
+ (filterToEdit ?? customizationToEdit)?.targets?.[0]
+ ?.semantic_selection_version
+ }
+ />
+ {datasetDetails?.semantic_selection_version &&
+ formFilter?.semantic_selection_version !==
+ datasetDetails.semantic_selection_version && (
+ <Alert
+ type="warning"
+ message={t('Choose current semantic filter fields')}
+ description={t(
+ 'Start field selection using current member IDs. This
clears any existing field selections, pre-filters, sorting, defaults and
dependencies. Saved display titles cannot be recovered automatically.',
+ )}
+ action={
+ <Button
+ onClick={() => {
+ setNativeFilterFieldValues(form, filterId, {
+ semantic_selection_version:
+ datasetDetails.semantic_selection_version,
+ column: undefined,
+ adhoc_filters: [],
+ granularity_sqla: undefined,
+ sortMetric: null,
+ defaultDataMask: {},
+ dependencies: [],
+ defaultValue: undefined,
+ controlValues: {},
+ });
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is valid and addresses a potential
bug where `time_range` and `time_grains` were not being cleared during the
reset process. Applying this suggestion ensures that the UI state correctly
reflects the removal of pre-filters, as promised by the Alert message.
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -220,6 +220,7 @@ def _build_query_dict(
dimensions=request.dimensions,
filters=[{"col": f.col, "op": f.op, "val": f.val} for f in
request.filters],
time_range=request.time_range,
+ semantic_selection_version=request.semantic_selection_version,
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate for the local context. It correctly identifies
that a version mismatch should be handled as a client-side validation error
rather than an internal server error, allowing agents to recover by reselecting
member IDs.
**superset/mcp_service/semantic_layer/tool/get_table.py**
```
dimensions=request.dimensions,
filters=[{"col": f.col, "op": f.op, "val": f.val} for f in
request.filters],
time_range=request.time_range,
+ semantic_selection_version=request.semantic_selection_version,
```
##########
superset-frontend/src/dataMask/reducer.ts:
##########
@@ -162,6 +162,12 @@ function fillNativeFilters(
...getInitialDataMask(filter.id), // take initial data
...filter.defaultDataMask, // if something new came from BE - take it
...loaded,
+ // A restored value must not inherit the default's identity evidence.
+ ...(filter.targets?.[0]?.semantic_selection_version &&
+ loaded?.filterState &&
+ !loaded.extraFormData
+ ? { extraFormData: {} }
+ : {}),
Review Comment:
<!-- Bito Reply -->
The suggestion to extract a shared helper for the hydration guard logic is
appropriate. It improves code maintainability by centralizing the logic for the
restored-value predicate, ensuring consistency across the two hydration paths
and preventing potential drift.
**superset-frontend/src/dataMask/reducer.ts**
```
// A restored value must not inherit the default's identity evidence.
...(filter.targets?.[0]?.semantic_selection_version &&
loaded?.filterState &&
!loaded.extraFormData
? { extraFormData: {} }
: {}),
```
##########
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:
##########
@@ -568,6 +630,13 @@ export default function getFormDataWithExtraFilters({
? getExtraFormData(dataMask, customizationIds)
: {};
+ const appliedGroupByIds = new Set(
+ Object.keys(groupByFormData).length
+ ? getMatchingGroupByCustomizations(groupByCustomizations, chart)
+ .filter(item => groupByState[item.id]?.selectedValues.length)
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate and improves the code by ensuring that
`appliedGroupByIds` is derived from the same contribution logic used for
processing group-by customizations. This prevents spurious provenance sources
and incorrect semantic selection versions that occur when selections are
filtered out but still marked as applied.
**superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts**
```
+ const appliedGroupByIds = new Set(
+ Object.keys(groupByFormData).length
+ ? getMatchingGroupByCustomizations(groupByCustomizations, chart)
+ .filter(item => groupByState[item.id]?.selectedValues.length)
```
--
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]
