mikebridge commented on PR #42540:
URL: https://github.com/apache/superset/pull/42540#issuecomment-5535092069
Thanks @aminghadersohi — thorough review. Both actionable items are fixed at
head `a17477389d`, and the two intent questions are confirmed below.
**Defect — Saved-only clear × is inert.** Fixed. `onSavedItemChange` and
`onSimpleItemChange` now route a cleared value (antd's `onChange(undefined)`)
through a shared `resetSelection` that clears column/metric/adhoc state and
resets the label to `''` — mirroring what `onCalculatedColumnChange` does on a
lookup miss, so the two branches of that select no longer disagree. I fixed
`onSimpleItemChange` too, since as you noted it shares the gap and becomes
load-bearing here. Two regression tests added — `clearing the Saved-only
dimension resets the selection` (renders a Saved-only view with a selected
dimension, clicks the clear ×, asserts `setLabel('')`) and a sibling `clearing
the Simple-mode item resets the selection` that pins the `onSimpleItemChange`
branch specifically. Each was verified to fail with its clear branch removed
(the zero-calls behaviour you saw) and pass with it.
**Hardening — raw-string features 500 Explore.** Fixed. `models.py` now uses
`sorted(getattr(feature, "value", feature) for feature in
self.implementation.features)`, so a provider that declares `features =
frozenset({"GROUP_LIMIT"})` degrades to the string value instead of
`AttributeError`-ing the `data` property. Test added
(`test_semantic_view_data_features_tolerates_raw_string`) mixing a raw string
with an enum member and asserting the sorted string payload — restoring the
"new provider is safe by default" property.
**Q1 — are `[]` and absent meant to produce opposite pickers?** Yes,
deliberately. Absent fails open (Simple enabled); empty array fails closed
(Saved-only). You're right that the safety property rests entirely on the key
never being omitted, which is exactly why `models.py` emits
`semantic_view_features` unconditionally (always a `list[str]`, empty when
nothing is declared) — a live semantic view can't land on the fail-open branch.
Any future payload path that drops the key would flip a Saved-only view to
permissive, so that unconditional emit is load-bearing and worth guarding in
review on any change to the `data` payload.
**Q2 — metrics deliberately out of scope?** Yes, intentional. This PR scopes
the capability lockdown to columns/dimensions; `AdhocMetricEditPopover` reads
compatibility but not capabilities, so metrics keep Simple/Custom SQL on a
Saved-only view. That matches the feature name (`ADHOC_COLUMN_EXPRESSIONS`) and
the docs ("Saved-only **column** picker"). Metric-side capability gating is a
separate follow-up if we want it.
**Minor — dimension-before-metric name collision.** Left as-is: testing
`calculatedColumns` before `metricMap` gives a deterministic tie-break (a
dimension wins over a same-named metric). Names collide rarely in practice, and
a stable precedence is preferable to ambiguity; happy to revisit if you'd
rather it error or prefer the metric.
Re-requesting your review — thanks again.
_Reply generated by Claude (AI) on behalf of @mikebridge._
--
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]
