bito-code-review[bot] commented on code in PR #44370:
URL: https://github.com/apache/superset/pull/44370#discussion_r4050329677
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete reset keeps pre-filter</b></div>
<div id="fix">
The reset handler clears `column`, `adhoc_filters`, `sortMetric`,
`defaultDataMask`, `dependencies` and `controlValues`, but omits `time_range`
and `time_grains`, which `setNativeFilterFieldValues` merges into existing form
state. Since `hasTimeRange` (line 709) and `hasPreFilter` (line 598) read
`formFilter.time_range`, an existing time-range pre-filter silently survives,
contradicting the Alert text promising pre-filters are cleared.
</div>
</div>
<small><i>Code Review Run #2b73e1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Version mismatch mislabeled InternalError</b></div>
<div id="fix">
When a client passes a stale/mismatched `semantic_selection_version`,
`validate_selection_version` (view.py:55) raises `ValueError`, which the
generic `except Exception` (line 510) turns into `InternalError`. A version
mismatch is a client selection error, not an internal failure; surface it as
`ValidationError` so agents can recover by reselecting member IDs.
</div>
</div>
<small><i>Code Review Run #2b73e1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated hydration guard logic</b></div>
<div id="fix">
This conditional-spread block is duplicated verbatim in `fillNativeFilters`
(166-170) and the HYDRATE_DASHBOARD chart-customization path (334-338),
differing only in the inspected mask (`loaded` vs
`dataMask[customizationFilterId]`). Extract a shared helper, e.g.
`withSemanticSelectionEvidence(mask, targets)`, so the restored-value predicate
cannot drift apart between the two hydration paths.
</div>
</div>
<small><i>Code Review Run #2b73e1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Magic sentinel string</b></div>
<div id="fix">
The sentinel `"unverified-external-selections"` is hardcoded here while the
frontend producer lives in `extractExtras.ts`
(superset-frontend/packages/superset-ui-core/src/query/extractExtras.ts:94);
`self.extras.get("semantic_selection_version")` is also evaluated twice (lines
414, 417). A named constant plus one hoisted lookup keeps this cross-layer
contract from drifting silently.
</div>
</div>
<small><i>Code Review Run #2b73e1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Applied-ids divergence</b></div>
<div id="fix">
`appliedGroupByIds` marks a customization applied when raw
`groupByState[item.id].selectedValues` is non-empty, but
`processGroupByCustomizations` drops non-string values (lines 423-425) and
columns conflicting with `existingColumns` (lines 432-438). A selection that
was entirely filtered out still gets `groupByApplied: true`, so
`getCustomizationSelectionSources` emits a spurious provenance source and
`extractExtras` can downgrade `semantic_selection_version` to
'unverified-external-selections'. Derive applied ids from the same contribution
logic.
</div>
</div>
<small><i>Code Review Run #2b73e1</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
