Compare commits

...

2 Commits

Author SHA1 Message Date
Beto Dealmeida
30032676a7 Address review + reconcile with time-grain PR
Reconcile with #41456: that PR sets ``expression=None`` on SV
dimensions so a truthy ``expression`` (the "adhoc SQL column" signal)
does not hide the time-grain selector. As a consequence, SV
dimensions with empty ``expression`` land in the popover's Simple
list, not the Saved list — the split in ``ColumnSelectPopover.tsx``
is done purely on ``expression`` truthiness. Disabling Simple would
leave dimensions unreachable in the popover, so only Custom SQL
stays disabled.

- Drop ``'simple'`` from ``effectiveDisabledTabs`` in
  ``DndColumnSelect.tsx`` and ``DndColumnMetricSelect.tsx``; keep
  ``'sqlExpression'``. Update comments to explain the tie to #41456.
- Rewrite the popover test to assert the new SV behavior (Simple
  enabled and selected by default; Custom SQL disabled; Saved
  enabled but not initial). Drop the ``as any`` cast per review.
- Remove PR-added "Saved tab" comment blocks in ``models.py`` — they
  will be misleading once #41456 lands with its own ``expression``
  rationale.
- Remove the four ``expression == "..."`` assertions in
  ``models_test.py`` for the same reason.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-07-01 19:13:48 -04:00
Beto Dealmeida
6db1fe893a fix(semantic layers): disable Simple tab in columns 2026-06-29 17:24:27 -04:00
4 changed files with 71 additions and 8 deletions

View File

@@ -128,6 +128,44 @@ test('open with Saved tab selected when there is a saved column selected', () =>
expect(getByText('Custom SQL')).toHaveAttribute('aria-selected', 'false');
});
test('disables Custom SQL for semantic views but keeps Simple enabled', () => {
// Semantic-view contexts disable the Custom SQL tab (ad-hoc SQL doesn't
// apply to pre-defined semantic-model items). Simple stays enabled — SV
// dimensions carry ``expression=`` empty, which routes them into the
// Simple tab's list, and disabling that tab would leave the dimensions
// unreachable in the popover.
const store = mockStore({
explore: { datasource: { type: 'semantic_view' } },
});
const { getByText } = render(
<ColumnSelectPopover
columns={[{ column_name: 'year' }]}
editedColumn={undefined}
getCurrentTab={jest.fn()}
onChange={jest.fn()}
hasCustomLabel
isTemporal={false}
label="Custom Label"
onClose={jest.fn()}
setDatasetModal={jest.fn()}
setLabel={jest.fn()}
disabledTabs={new Set(['sqlExpression'])}
/>,
{ store },
);
// Simple is enabled and selected by default (no adhoc/saved column
// to steer initial selection elsewhere).
expect(getByText('Simple')).not.toHaveAttribute('aria-disabled', 'true');
expect(getByText('Simple')).toHaveAttribute('aria-selected', 'true');
// Saved remains enabled but is not the initial tab.
expect(getByText('Saved')).not.toHaveAttribute('aria-disabled', 'true');
expect(getByText('Saved')).toHaveAttribute('aria-selected', 'false');
// Custom SQL is greyed out and unselected.
expect(getByText('Custom SQL')).toHaveAttribute('aria-disabled', 'true');
expect(getByText('Custom SQL')).toHaveAttribute('aria-selected', 'false');
});
test('open with Custom SQL tab selected when there is a custom SQL selected', () => {
const { getByText } = renderPopover({
columns: [{ column_name: 'year' }],

View File

@@ -281,11 +281,20 @@ const ColumnSelectPopover = ({
[columnMap, metricMap, onSimpleColumnChange, onSimpleMetricChange],
);
const defaultActiveTabKey = initialAdhocColumn
? 'sqlExpression'
: selectedCalculatedColumn
? 'saved'
: 'simple';
// Pick the most natural starting tab, but never one that's disabled —
// semantic-view contexts disable Simple and Custom SQL, leaving Saved.
let defaultActiveTabKey: string;
if (initialAdhocColumn && !disabledTabs.has('sqlExpression')) {
defaultActiveTabKey = 'sqlExpression';
} else if (selectedCalculatedColumn && !disabledTabs.has('saved')) {
defaultActiveTabKey = 'saved';
} else if (!disabledTabs.has('simple')) {
defaultActiveTabKey = 'simple';
} else if (!disabledTabs.has('saved')) {
defaultActiveTabKey = 'saved';
} else {
defaultActiveTabKey = 'sqlExpression';
}
useEffect(() => {
getCurrentTab(defaultActiveTabKey);
@@ -501,8 +510,15 @@ const ColumnSelectPopover = ({
},
]),
{
// The Simple tab is rendered even when disabled (mirroring how
// Custom SQL behaves below) so users see all the options that
// could exist in another context and understand why they're
// unavailable here. Semantic-view contexts disable it because
// dimensions there are pre-defined items in the semantic model,
// not physical columns to wrap.
key: TABS_KEYS.SIMPLE,
label: t('Simple'),
disabled: disabledTabs.has('simple'),
children: (
<>
{isTemporal && simpleColumns.length === 0 ? (

View File

@@ -130,8 +130,12 @@ function DndColumnMetricSelect(props: DndColumnMetricSelectProps) {
formData,
} = props;
// Semantic views do not support arbitrary SQL expressions as dimensions.
// Merge 'sqlExpression' into disabledTabs so the Custom SQL tab is hidden.
// Semantic-view dimensions and metrics are pre-defined items in the
// semantic model, so the Custom SQL tab (ad-hoc SQL expressions) doesn't
// apply and is disabled. The Simple tab stays enabled: dimensions land
// there because ``expression`` is left unset (a truthy ``expression``
// triggers fx-icon treatment in the popover and hides the time-grain
// selector, which we need to keep reachable).
const effectiveDisabledTabs = useMemo(
() =>
String(datasource?.type) === 'semantic_view'

View File

@@ -53,7 +53,12 @@ function DndColumnSelect(props: DndColumnSelectProps) {
disabledTabs,
} = props;
// Semantic views do not support arbitrary SQL expressions as dimensions.
// Semantic-view dimensions are pre-defined items in the semantic model,
// so the Custom SQL tab (ad-hoc SQL expressions) doesn't apply and is
// disabled. The Simple tab stays enabled: dimensions land there because
// ``expression`` is left unset (a truthy ``expression`` triggers fx-icon
// treatment in the popover and hides the time-grain selector, which we
// need to keep reachable).
const datasourceType = useSelector<ExplorePageState, string | undefined>(
state => state.explore.datasource?.type,
);