mikebridge commented on code in PR #42539:
URL: https://github.com/apache/superset/pull/42539#discussion_r3730055204
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.test.ts:
##########
@@ -377,3 +377,67 @@ test('doesChartMatchFilterDatasource falls back to
datasource UID parsing', () =
),
).toBe(true);
});
+
+test('fetchSemanticViewStructure returns name, dimensions, and metrics from
the structure payload', async () => {
+ const fetchMock = require('fetch-mock').default;
+ const { fetchSemanticViewStructure: fetchStructure } = require('./utils');
+ fetchMock.get('glob:*/api/v1/semantic_view/9101/structure', {
+ result: {
+ name: 'orders',
+ dimensions: [{ name: 'Orders Status', type: 'VARCHAR' }],
+ metrics: [{ name: 'order_count', definition: 'COUNT(*)' }],
+ },
+ });
+
+ const structure = await fetchStructure(9101);
+
+ expect(structure.name).toBe('orders');
+ expect(structure.dimensions).toEqual([
+ { name: 'Orders Status', type: 'VARCHAR' },
+ ]);
+ expect(structure.metrics).toEqual([
+ { name: 'order_count', definition: 'COUNT(*)' },
+ ]);
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+});
+
+test('fetchSemanticViewStructure defaults missing arrays to empty', async ()
=> {
+ const fetchMock = require('fetch-mock').default;
+ const { fetchSemanticViewStructure: fetchStructure } = require('./utils');
+ fetchMock.get('glob:*/api/v1/semantic_view/9102/structure', {
+ result: { name: 'sparse' },
+ });
+
+ const structure = await fetchStructure(9102);
+
+ expect(structure.dimensions).toEqual([]);
+ expect(structure.metrics).toEqual([]);
+ fetchMock.removeRoutes();
+ fetchMock.clearHistory();
+});
+
+test('semanticViewDimensionsToColumns maps dimension fields incl. temporal
detection', () => {
+ const { semanticViewDimensionsToColumns: toColumns } = require('./utils');
+ const columns = toColumns([
+ { name: 'ordered_at', type: 'TIMESTAMP' },
+ { name: 'status', type: 'VARCHAR' },
+ ]);
+
+ expect(columns[0]).toMatchObject({
+ column_name: 'ordered_at',
+ type: 'TIMESTAMP',
+ is_dttm: true,
+ filterable: true,
+ });
+ expect(columns[1]).toMatchObject({
+ column_name: 'status',
+ is_dttm: false,
+ filterable: true,
+ });
Review Comment:
Fixed in 23f1617b08 — the test now asserts `type_generic` explicitly for a
temporal, string, and numeric dimension. The fixture also switched to
wire-shaped pyarrow type names (`timestamp[us]`/`string`/`double`): that is
what `/structure` actually serialises, and the mapper deliberately does not
match SQL-style names like `VARCHAR`, so the old fixture could not have carried
the assertion. Control-run verified: breaking
`mapSemanticTypeToGenericDataType` now fails this test.
##########
superset-frontend/src/hooks/apiResources/datasets.ts:
##########
@@ -31,6 +32,10 @@ import {
cachedSupersetGet,
supersetGetCache,
} from 'src/utils/cachedSupersetGet';
+import {
+ fetchSemanticViewStructure,
+ semanticViewDimensionsToColumns,
+} from
'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils';
Review Comment:
Fixed in 23f1617b08 — the shared helpers
(`mapSemanticTypeToGenericDataType`, `fetchSemanticViewStructure`,
`semanticViewDimensionsToColumns`, and the `SemanticViewStructure` type) moved
to a layer-neutral `src/utils/semanticViewStructure.ts`. The filter-form utils
re-export them so existing call sites are untouched, and this hook now imports
from `src/utils` — no dashboard-modal dependency remains.
##########
superset-frontend/src/hooks/apiResources/datasets.ts:
##########
@@ -89,7 +114,29 @@ export const useDatasetDrillInfo = (
);
let result;
- if (loadDrillByOptionsExtension && formData) {
+ if (
+ getDatasourceTypeFromId(datasetId) === DatasourceType.SemanticView
+ ) {
+ // Semantic views short-circuit BEFORE the extension check: the
+ // extension receives only the numeric id, which would resolve
+ // the colliding regular dataset (sc-111089 review consensus).
+ // The structure payload carries no changed_on/owners metadata —
+ // those metadata-bar rows render their not-available state, an
+ // accepted degradation. Columns are narrowed to metadata needs;
+ // no drill flags are fabricated.
+ const structure = await fetchSemanticViewStructure(numericDatasetId);
+ result = {
+ id: numericDatasetId,
+ table_name: structure.name,
+ datasource_type: DatasourceType.SemanticView,
+ columns: semanticViewDimensionsToColumns(structure.dimensions),
+ metrics: structure.metrics.map(metric => ({
+ metric_name: metric.name,
+ expression: metric.definition,
+ verbose_name: null,
+ })),
+ } as unknown as Dataset;
Review Comment:
Fixed in 23f1617b08, slightly more aggressively than suggested: rather than
a dedicated type, the cast is deleted outright. The fabricated
`id`/`datasource_type` fields are dropped — no consumer of this resource reads
them (both call sites read only `columns`/`metrics` and derive `verbose_map`) —
and `verbose_name` is omitted rather than `null`, so the literal is a
structurally valid `Dataset` with no cast at all.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -763,34 +764,19 @@ const FiltersConfigForm = (
useEffect(() => {
if (datasetId) {
if (datasourceType === DatasourceType.SemanticView) {
- cachedSupersetGet({
- endpoint: `/api/v1/semantic_view/${datasetId}/structure`,
- })
- .then((response: JsonResponse) => {
- const {
- name: svName,
- dimensions = [],
- metrics: svMetrics = [],
- } = response.json?.result ?? {};
- const columns = dimensions.map(
- (dim: { name: string; type: string }) => {
- const mappedType = mapSemanticTypeToGenericDataType(dim.type);
- return {
- column_name: dim.name,
- type: dim.type,
- is_dttm: mappedType === GenericDataType.Temporal,
- filterable: true,
- type_generic: mappedType,
- };
- },
- );
+ fetchSemanticViewStructure(datasetId)
+ .then(({ name: svName, dimensions, metrics: svMetrics }) => {
+ const columns = semanticViewDimensionsToColumns(dimensions);
+ // verbose_name stays null at runtime (pre-refactor value —
+ // consumers only falsy-check it); Metric types it as an
+ // optional string, hence the cast.
const mappedMetrics = svMetrics.map(
(m: { name: string; definition: string }) => ({
metric_name: m.name,
expression: m.definition,
verbose_name: null,
}),
- );
+ ) as unknown as Metric[];
Review Comment:
Addressed in 23f1617b08, with one correction to the diagnosis: dropping
`verbose_name: null` alone does not lift the cast — the real gap the blanket
cast was hiding is `uuid`, which `Metric` requires and the `/structure` wire
does not carry. Since this state's consumers key on
`metric_name`/`verbose_name` and never read `uuid`, the cast is now narrowed to
exactly that one absent property (`as Omit<Metric, 'uuid'>[] as Metric[]`) with
a comment saying why — every other field stays compiler-checked instead of
being erased by `unknown`.
##########
superset-frontend/src/hooks/apiResources/datasets.ts:
##########
@@ -41,6 +46,26 @@ export const getDatasetId = (datasetId: string | number):
number =>
? Number(datasetId.split('__')[0])
: Number(datasetId);
+/**
+ * Extract the datasource type from an `<id>__<type>` datasource string.
+ * Semantic views and regular datasets have independent numeric-id
+ * sequences, so the type is load-bearing: resolving by id alone reads
+ * whatever regular dataset shares the number (sc-111089). Absent or
+ * unrecognized suffixes fall back to a regular dataset, preserving
+ * legacy behaviour.
+ */
+export const getDatasourceTypeFromId = (
+ datasetId: string | number,
+): DatasourceType => {
Review Comment:
Renamed in 23f1617b08 to `getDatasourceTypeFromDatasourceId` — it parses the
composite `<id>__<type>` datasource string. (The sibling `getDatasetId` shares
the naming quirk but predates this PR, so it is left alone 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]
