You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by en...@apache.org on 2026/09/01 16:16:49 UTC
(superset) branch master updated: fix(datasets): warn on percent D3 format for count metrics (#43453)
This is an automated email from the ASF dual-hosted git repository.
EnxDev pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 842efd5f120 fix(datasets): warn on percent D3 format for count metrics (#43453)
842efd5f120 is described below
commit 842efd5f120f15c9ecfd13bf7444bbc818bc4469
Author: Enzo Martellucci <52...@users.noreply.github.com>
AuthorDate: Tue Sep 1 18:16:40 2026 +0200
fix(datasets): warn on percent D3 format for count metrics (#43453)
---
superset-frontend/package-lock.json | 3 +-
superset-frontend/package.json | 1 -
.../packages/superset-ui-core/package.json | 2 +-
.../superset-ui-core/src/number-format/index.ts | 2 +
.../DatasourceEditor/DatasourceEditor.tsx | 75 +++++++-
.../DatasourceEditorMetricFormatWarning.test.tsx | 210 +++++++++++++++++++++
.../Datasource/components/Fieldset/index.tsx | 3 +
superset-frontend/src/setup/setupFormatters.ts | 2 +-
superset-frontend/src/types/bootstrapTypes.ts | 2 +-
9 files changed, 293 insertions(+), 7 deletions(-)
diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json
index 875ad5de4b8..670e9571bda 100644
--- a/superset-frontend/package-lock.json
+++ b/superset-frontend/package-lock.json
@@ -69,7 +69,6 @@
"@superset-ui/plugin-chart-world-map": "file:./plugins/plugin-chart-world-map",
"@superset-ui/preset-chart-deckgl": "file:./plugins/preset-chart-deckgl",
"@superset-ui/switchboard": "file:./packages/superset-ui-switchboard",
- "@types/d3-format": "^3.0.1",
"@types/d3-selection": "^3.0.11",
"@types/d3-time-format": "^4.0.3",
"@types/react-google-recaptcha": "^2.1.9",
@@ -42974,6 +42973,7 @@
"@apache-superset/core": "*",
"@babel/runtime": "^7.29.7",
"@braintree/sanitize-url": "^7.1.2",
+ "@types/d3-format": "^3.0.4",
"@types/json-bigint": "^1.0.4",
"@visx/responsive": "^4.0.0",
"ace-builds": "^1.44.0",
@@ -43017,7 +43017,6 @@
},
"devDependencies": {
"@emotion/styled": "^11.14.1",
- "@types/d3-format": "^3.0.4",
"@types/d3-interpolate": "^3.0.4",
"@types/d3-scale": "^4.0.9",
"@types/d3-time": "^3.0.4",
diff --git a/superset-frontend/package.json b/superset-frontend/package.json
index 980e97691d0..7ec39fe2212 100644
--- a/superset-frontend/package.json
+++ b/superset-frontend/package.json
@@ -146,7 +146,6 @@
"@superset-ui/plugin-chart-world-map": "file:./plugins/plugin-chart-world-map",
"@superset-ui/preset-chart-deckgl": "file:./plugins/preset-chart-deckgl",
"@superset-ui/switchboard": "file:./packages/superset-ui-switchboard",
- "@types/d3-format": "^3.0.1",
"@types/d3-selection": "^3.0.11",
"@types/d3-time-format": "^4.0.3",
"@types/react-google-recaptcha": "^2.1.9",
diff --git a/superset-frontend/packages/superset-ui-core/package.json b/superset-frontend/packages/superset-ui-core/package.json
index 37f5d6c3af4..7c2330f572c 100644
--- a/superset-frontend/packages/superset-ui-core/package.json
+++ b/superset-frontend/packages/superset-ui-core/package.json
@@ -53,6 +53,7 @@
"@apache-superset/core": "*",
"@babel/runtime": "^7.29.7",
"@braintree/sanitize-url": "^7.1.2",
+ "@types/d3-format": "^3.0.4",
"@types/json-bigint": "^1.0.4",
"@visx/responsive": "^4.0.0",
"ace-builds": "^1.44.0",
@@ -96,7 +97,6 @@
},
"devDependencies": {
"@emotion/styled": "^11.14.1",
- "@types/d3-format": "^3.0.4",
"@types/d3-interpolate": "^3.0.4",
"@types/d3-scale": "^4.0.9",
"@types/d3-time": "^3.0.4",
diff --git a/superset-frontend/packages/superset-ui-core/src/number-format/index.ts b/superset-frontend/packages/superset-ui-core/src/number-format/index.ts
index d6d6096d8fc..87b6633feb0 100644
--- a/superset-frontend/packages/superset-ui-core/src/number-format/index.ts
+++ b/superset-frontend/packages/superset-ui-core/src/number-format/index.ts
@@ -19,6 +19,8 @@
export { default as NumberFormats } from './NumberFormats';
export { default as NumberFormatter, PREVIEW_VALUE } from './NumberFormatter';
+export { formatSpecifier } from 'd3-format';
+export type { FormatLocaleDefinition } from 'd3-format';
export { DEFAULT_D3_FORMAT } from './D3FormatConfig';
export {
diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
index a20e5e4b8d3..394806b1716 100644
--- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
+++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx
@@ -36,6 +36,7 @@ import {
SupersetClient,
getClientErrorObject,
getExtensionsRegistry,
+ formatSpecifier,
} from '@superset-ui/core';
import { GenericDataType } from '@apache-superset/core/common';
import { t } from '@apache-superset/core/translation';
@@ -827,6 +828,78 @@ function EditorsSelector({
const ResultTable =
extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable;
+// D3's '%' and 'p' types both multiply by 100; parsed via d3-format's own
+// grammar so garbage like "foo%" is rejected rather than matched by suffix.
+// The stored value is trimmed before parsing because
+// NumberFormatterRegistry.get() trims it the same way before rendering, so
+// this check agrees with what the renderer actually sees.
+export const isPercentD3Format = (d3format?: string): boolean => {
+ if (!d3format) {
+ return false;
+ }
+ try {
+ const { type } = formatSpecifier(d3format.trim());
+ return type === '%' || type === 'p';
+ } catch {
+ return false;
+ }
+};
+
+// Matches the outermost COUNT(...) call's parens by depth, so a ratio like
+// `COUNT(*) / COUNT(*)` isn't misclassified but a nested call like
+// `COUNT(DISTINCT COALESCE(a, b))` is still recognized. Parens inside a
+// quoted string literal (single- or double-quoted, with a doubled quote as
+// an escaped quote) are ignored so they don't desync the depth count.
+export const isCountExpression = (expression?: string): boolean => {
+ const trimmed = expression?.trim();
+ if (!trimmed || !/^count\s*\(/i.test(trimmed) || !trimmed.endsWith(')')) {
+ return false;
+ }
+ let depth = 0;
+ let stringDelimiter: string | null = null;
+ for (let i = trimmed.indexOf('('); i < trimmed.length; i += 1) {
+ const char = trimmed[i];
+ if (stringDelimiter) {
+ if (char === stringDelimiter && trimmed[i + 1] === stringDelimiter) {
+ i += 1;
+ } else if (char === stringDelimiter) {
+ stringDelimiter = null;
+ }
+ } else if (char === "'" || char === '"') {
+ stringDelimiter = char;
+ } else if (char === '(') {
+ depth += 1;
+ } else if (char === ')') {
+ depth -= 1;
+ if (depth === 0) {
+ return i === trimmed.length - 1;
+ }
+ }
+ }
+ return false;
+};
+
+function renderMetricFormatWarning(item: Record<string, any>): ReactNode {
+ if (
+ !isCountExpression(item.expression) ||
+ !isPercentD3Format(item.d3format)
+ ) {
+ return null;
+ }
+ return (
+ <Alert
+ css={themeParam => ({ marginBottom: themeParam.sizeUnit * 4 })}
+ type="warning"
+ showIcon
+ message={t(
+ 'This metric is a count, but its D3 format is a percentage. ' +
+ 'Percent formats multiply the value by 100, which will make a ' +
+ 'raw count render as a misleadingly large number.',
+ )}
+ />
+ );
+}
+
// Redux connector types
interface QueryPayload {
client_id?: string;
@@ -2170,7 +2243,7 @@ function DatasourceEditor({
}}
expandFieldset={
<FormContainer>
- <Fieldset compact>
+ <Fieldset compact renderWarning={renderMetricFormatWarning}>
<Field
fieldKey="expression"
label={t('SQL expression')}
diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorMetricFormatWarning.test.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorMetricFormatWarning.test.tsx
new file mode 100644
index 00000000000..255b63e57eb
--- /dev/null
+++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorMetricFormatWarning.test.tsx
@@ -0,0 +1,210 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import fetchMock from 'fetch-mock';
+import {
+ screen,
+ userEvent,
+ waitFor,
+ within,
+} from 'spec/helpers/testing-library';
+import { isCountExpression, isPercentD3Format } from '../DatasourceEditor';
+import {
+ createProps,
+ DATASOURCE_ENDPOINT,
+ setupDatasourceEditorMocks,
+ cleanupAsyncOperations,
+ fastRender,
+ dismissDatasourceWarning,
+ DatasourceEditorProps,
+} from './DatasourceEditor.test.utils';
+
+beforeEach(() => {
+ fetchMock.get(DATASOURCE_ENDPOINT, [], { name: DATASOURCE_ENDPOINT });
+ setupDatasourceEditorMocks();
+});
+
+afterEach(async () => {
+ await cleanupAsyncOperations();
+ fetchMock.clearHistory().removeRoutes();
+});
+
+const WARNING_TEXT = /D3 format is a percentage/i;
+
+// Selecting the expand toggle by its position in the list is brittle: any
+// change to the fixture or the table's sort order would expand a different
+// row and negative assertions would keep passing against the wrong metric.
+// Look up the toggle via the row that actually contains the metric name.
+const expandMetricRow = async (metricName: string) => {
+ const nameCell = await screen.findByText(metricName);
+ const row = nameCell.closest('tr');
+ if (!row) {
+ throw new Error(`Could not find a table row for metric "${metricName}"`);
+ }
+ await userEvent.click(within(row).getByLabelText(/expand row/i));
+};
+
+// A fixed-time sleep can't prove the debounced value actually committed, so
+// negative assertions would pass vacuously if the commit landed late on a
+// loaded runner. Instead, wait for the real signal of a commit: the metric's
+// d3format reaching the top-level onChange the editor calls after every
+// datasource state update.
+const waitForD3FormatCommit = (
+ onChange: DatasourceEditorProps['onChange'],
+ metricName: string,
+ d3format: string,
+) =>
+ waitFor(() => {
+ const [datasource] = onChange.mock.calls.at(-1) ?? [];
+ const metric = datasource?.metrics?.find(
+ (m: { metric_name?: string }) => m.metric_name === metricName,
+ );
+ expect(metric?.d3format).toBe(d3format);
+ });
+
+test('isCountExpression matches a COUNT(...) call, including nested calls', () => {
+ expect(isCountExpression('COUNT(*)')).toBe(true);
+ expect(isCountExpression('count( * )')).toBe(true);
+ expect(isCountExpression('COUNT (*)')).toBe(true);
+ expect(isCountExpression('COUNT(DISTINCT name)')).toBe(true);
+ expect(isCountExpression('COUNT(DISTINCT COALESCE(a, b))')).toBe(true);
+ expect(isCountExpression('COUNT(*) / COUNT(*)')).toBe(false);
+ expect(isCountExpression('COUNT(*) * 100')).toBe(false);
+ expect(isCountExpression('SUM(num)')).toBe(false);
+ expect(isCountExpression(undefined)).toBe(false);
+});
+
+test('isCountExpression ignores parens inside string literals', () => {
+ expect(isCountExpression("COUNT(CASE WHEN x = '(' THEN 1 END)")).toBe(true);
+ expect(isCountExpression("COUNT(CASE WHEN x = ')' THEN 1 END)")).toBe(true);
+ expect(isCountExpression("COUNT(CASE WHEN x = '''(' THEN 1 END)")).toBe(true);
+});
+
+test('isCountExpression ignores parens inside double-quoted identifiers', () => {
+ expect(isCountExpression('COUNT("x\'")')).toBe(true);
+ expect(isCountExpression('COUNT("y\'")')).toBe(true);
+ expect(isCountExpression('COUNT(CASE WHEN "a""b" = 1 THEN 1 END)')).toBe(
+ true,
+ );
+});
+
+test('isPercentD3Format accepts only a valid D3 percent/p spec', () => {
+ expect(isPercentD3Format('.0%')).toBe(true);
+ expect(isPercentD3Format(',.2%')).toBe(true);
+ expect(isPercentD3Format('.1p')).toBe(true);
+ expect(isPercentD3Format('foo%')).toBe(false);
+ expect(isPercentD3Format('.0%garbage%')).toBe(false);
+ expect(isPercentD3Format(',.0f')).toBe(false);
+ expect(isPercentD3Format(undefined)).toBe(false);
+});
+
+// NumberFormatterRegistry.get() trims the stored value before parsing it at
+// render time, so this must trim too rather than reject a format the
+// renderer accepts.
+test('isPercentD3Format trims, matching render-time parsing', () => {
+ expect(isPercentD3Format('.0% ')).toBe(true);
+});
+
+// A '%' format is valid syntax, so it never hits the "Invalid format" fallback.
+test('warns when a percent D3 format is set on a COUNT metric', async () => {
+ const testProps = createProps();
+ fastRender(testProps);
+ await dismissDatasourceWarning();
+
+ await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
+ await expandMetricRow('count');
+
+ expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
+
+ await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
+
+ expect(await screen.findByText(WARNING_TEXT)).toBeInTheDocument();
+});
+
+test('does not warn for a non-percent format on a COUNT metric', async () => {
+ const testProps = createProps();
+ fastRender(testProps);
+ await dismissDatasourceWarning();
+
+ await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
+ await expandMetricRow('count');
+
+ await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), ',.0f');
+
+ await waitForD3FormatCommit(testProps.onChange, 'count', ',.0f');
+ expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
+});
+
+test('does not warn for a percent format on a non-COUNT metric', async () => {
+ const testProps = createProps();
+ fastRender(testProps);
+ await dismissDatasourceWarning();
+
+ await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
+ await expandMetricRow('sum__num');
+
+ await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
+
+ await waitForD3FormatCommit(testProps.onChange, 'sum__num', '.0%');
+ expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
+});
+
+test('does not warn for a ratio built from COUNT, e.g. COUNT(*) / COUNT(*)', async () => {
+ const baseProps = createProps();
+ const testProps = {
+ ...baseProps,
+ datasource: {
+ ...baseProps.datasource,
+ metrics: [
+ ...baseProps.datasource.metrics,
+ {
+ id: 99,
+ uuid: 'metric-99-uuid',
+ expression: 'COUNT(*) / COUNT(*)',
+ verbose_name: 'ratio',
+ metric_name: 'ratio',
+ metric_type: 'count',
+ },
+ ],
+ },
+ };
+ fastRender(testProps);
+ await dismissDatasourceWarning();
+
+ await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
+ await expandMetricRow('ratio');
+
+ await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
+
+ await waitForD3FormatCommit(testProps.onChange, 'ratio', '.0%');
+ expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
+});
+
+test('does not warn for a garbage format string that merely ends in %', async () => {
+ const testProps = createProps();
+ fastRender(testProps);
+ await dismissDatasourceWarning();
+
+ await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
+ await expandMetricRow('count');
+
+ await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), 'foo%');
+
+ await waitForD3FormatCommit(testProps.onChange, 'count', 'foo%');
+ expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
+});
diff --git a/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx b/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx
index e74d25f6965..e2f77be2c47 100644
--- a/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx
+++ b/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx
@@ -28,6 +28,7 @@ export interface FieldsetProps {
item?: Record<string, any>;
title?: ReactNode;
compact?: boolean;
+ renderWarning?: (item: Record<string, any>) => ReactNode;
}
type fieldKeyType = string | number;
@@ -38,6 +39,7 @@ export default function Fieldset({
item = {},
title = null,
compact = false,
+ renderWarning,
}: FieldsetProps) {
// Controls report their edits asynchronously - TextControl debounces by
// FAST_DEBOUNCE - so the callback that eventually fires was built during an
@@ -78,6 +80,7 @@ export default function Fieldset({
</Typography.Title>
)}
+ {renderWarning?.(item)}
{recurseReactClone(children, Field, propExtender)}
</Form>
);
diff --git a/superset-frontend/src/setup/setupFormatters.ts b/superset-frontend/src/setup/setupFormatters.ts
index a96074f71a8..d041913a3bb 100644
--- a/superset-frontend/src/setup/setupFormatters.ts
+++ b/superset-frontend/src/setup/setupFormatters.ts
@@ -32,8 +32,8 @@ import {
setCurrencyLocale,
createLengthFormatter,
createThroughputFormatter,
+ FormatLocaleDefinition,
} from '@superset-ui/core';
-import { FormatLocaleDefinition } from 'd3-format';
import { TimeLocaleDefinition } from 'd3-time-format';
export default function setupFormatters(
diff --git a/superset-frontend/src/types/bootstrapTypes.ts b/superset-frontend/src/types/bootstrapTypes.ts
index 9a10e88d684..9c341034f38 100644
--- a/superset-frontend/src/types/bootstrapTypes.ts
+++ b/superset-frontend/src/types/bootstrapTypes.ts
@@ -16,7 +16,6 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { FormatLocaleDefinition } from 'd3-format';
import { TimeLocaleDefinition } from 'd3-time-format';
import { isPlainObject } from 'lodash-es';
import { Languages } from 'src/features/home/LanguagePicker';
@@ -28,6 +27,7 @@ import {
import type {
ColorSchemeConfig,
FeatureFlagMap,
+ FormatLocaleDefinition,
JsonObject,
SequentialSchemeConfig,
} from '@superset-ui/core';