mikebridge commented on code in PR #44370:
URL: https://github.com/apache/superset/pull/44370#discussion_r4054404099
##########
superset/mcp_service/semantic_layer/schemas.py:
##########
@@ -81,6 +81,7 @@ class MetricInfo(BaseModel):
dataset_name: str | None = None
view_id: int | None = None
view_name: str | None = None
+ semantic_selection_version: str | None = None
Review Comment:
Confirmed at e618d3481e. A red-first MCP regression reproduced the missing
version (1 failed, 12 passed). The staged fix populates
MetricInfo.semantic_selection_version from the external view implementation,
matching list_metrics; both unversioned and versioned providers are tested.
This round is staged for independent review, not pushed yet.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx:
##########
@@ -203,6 +207,7 @@ const FilterValue: FC<FilterValueProps> = ({
...filter,
datasetId,
datasourceType,
+ semantic_selection_version,
Review Comment:
Checked at e618d3481e: semantic_selection_version comes from target, which
comes from filter.targets. The effect already depends on filter, so a
Redux-immutable target/version update replaces filter and recomputes formData.
There is no separately changing version input omitted from the dependency list.
In-place mutation of filter.targets would violate the Redux update contract; no
code change needed here.
##########
superset-frontend/packages/superset-ui-core/src/query/extractExtras.ts:
##########
@@ -72,6 +73,27 @@ export default function extractExtras(formData:
QueryFormData): ExtractedExtra {
}
});
+ const selectionSources = [
+ ...getSemanticSelectionSources(formData.extra_form_data),
+ ...(formData.semantic_selection_sources ?? []),
+ ];
+ const hasUnversionedExtras = (formData.extra_filters ?? []).some(
+ filter =>
+ !['__time_range', '__time_grain', '__time_compare'].includes(filter.col),
+ );
Review Comment:
This rejection is intentional: __time_col and __granularity carry a member
identity, whereas __time_range, __time_grain and __time_compare carry scalar
temporal settings. An old title can exactly equal a stable member ID. An
unversioned external member override cannot borrow the chart identity
certificate. The existing parameterized test “legacy external member %s cannot
borrow the chart generation” covers __time_col and __granularity. Keeping this
guard preserves the explicit-reselection contract.
##########
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:
##########
@@ -584,6 +658,21 @@ export default function getFormDataWithExtraFilters({
...groupByFormData,
...customizationExtraFormData,
...(chartCustomization && { chart_customization: chartCustomization }),
+ // Customizations can rewrite member fields outside extra_form_data.
+ ...(chart.form_data?.semantic_selection_version && {
+ semantic_selection_sources: [
+ ...(chart.form_data.semantic_selection_sources ?? []),
+ ...customizationIds.flatMap(id =>
+ getCustomizationSelectionSources({
+ customization: chartCustomizationItems?.find(
+ item => item.id === id,
+ ),
+ mask: dataMask[id],
+ groupByApplied: appliedGroupByIds.has(id),
+ }),
+ ),
+ ],
+ }),
Review Comment:
Confirmed at e618d3481e. Three red-first tests reproduce stale cache hits
when the chart version, source list, or datasource changes. The staged fix
compares those chart inputs separately from the merged output provenance; it
also verifies unchanged inputs still hit the cache. Awaiting independent review
before publication; not pushed yet.
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -381,9 +381,26 @@ function ChartRendererComponent({
const setDataMaskCallback = useCallback(
(dataMask: DataMask) => {
- actions?.updateDataMask?.(chartId, dataMask);
+ const sourceForm = latestQueryFormData ?? formData;
+ actions?.updateDataMask?.(
+ chartId,
+ sourceForm.datasource?.endsWith('__semantic_view')
+ ? {
+ ...dataMask,
+ extraFormData: {
+ ...dataMask.extraFormData,
+ semantic_selection_sources: [
+ {
+ datasource: sourceForm.datasource,
+ version: sourceForm.semantic_selection_version ?? null,
+ },
+ ],
Review Comment:
Confirmed at e618d3481e. A red-first ChartRenderer test shows explicit
incoming provenance was overwritten. The staged fix preserves incoming mask
sources and the source query sources, then appends the chart identity, so
foreign or unversioned evidence cannot be erased by the chart certificate.
Awaiting independent review before publication; not pushed yet.
##########
superset-frontend/src/dataMask/reducer.ts:
##########
@@ -77,6 +77,16 @@ export interface HydrateDataMaskAction {
};
}
+function getRestoredSelectionEvidence(
+ mask: DataMask | undefined,
+ version: string | undefined,
+): Partial<DataMask> {
+ // A restored value must not inherit the default's identity evidence.
+ return version && mask?.filterState && !mask.extraFormData
+ ? { extraFormData: {} }
Review Comment:
This replacement is intentional for a restored versioned filterState without
its own extraFormData. defaultDataMask.extraFormData describes the default
selection, so retaining it alongside a different restored value would apply a
different predicate and incorrectly borrow the default identity certificate.
Independent prefilters remain on the filter configuration. The required-filter
fallback separately restores the default value and extraFormData together.
Existing native-filter and customization hydration tests cover this
provenance/default precedence; no code change here.
--
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]
