chore: adress comments

superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx

@@ -30,6 +30,7 @@ import { connect, ConnectedProps } from 'react-redux';
 import type { AnyAction } from 'redux';
 import type { ThunkDispatch } from 'redux-thunk';
 import { Radio } from '@superset-ui/core/components/Radio';
+import { formatSpecifier } from 'd3-format';
 import {
   isFeatureEnabled,
   FeatureFlag,
@@ -827,6 +828,64 @@ 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.
+export const isPercentD3Format = (d3format?: string): boolean => {
+  const trimmed = d3format?.trim();
+  if (!trimmed) {
+    return false;
+  }
+  try {
+    const { type } = formatSpecifier(trimmed);
+    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.
+export const isCountExpression = (expression?: string): boolean => {
+  const trimmed = expression?.trim();
+  if (!trimmed || !/^count\(/i.test(trimmed) || !trimmed.endsWith(')')) {
+    return false;
+  }
+  let depth = 0;
+  for (let i = trimmed.indexOf('('); i < trimmed.length; i += 1) {
+    if (trimmed[i] === '(') {
+      depth += 1;
+    } else if (trimmed[i] === ')') {
+      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 +2229,7 @@ function DatasourceEditor({
           }}
           expandFieldset={
             <FormContainer>
-              <Fieldset compact>
+              <Fieldset compact renderWarning={renderMetricFormatWarning}>
                 <Field
                   fieldKey="expression"
                   label={t('SQL expression')}

superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorMetricFormatWarning.test.tsx

@@ -0,0 +1,167 @@
+/**
+ * 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 } from 'spec/helpers/testing-library';
+import { Constants } from '@superset-ui/core/components';
+import { isCountExpression, isPercentD3Format } from '../DatasourceEditor';
+import {
+  createProps,
+  DATASOURCE_ENDPOINT,
+  setupDatasourceEditorMocks,
+  cleanupAsyncOperations,
+  fastRender,
+  dismissDatasourceWarning,
+} 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;
+
+// Negative assertions must wait past TextControl's debounce, or they pass
+// before the value even commits.
+const waitPastDebounce = () =>
+  new Promise(resolve => {
+    setTimeout(resolve, Constants.FAST_DEBOUNCE + 50);
+  });
+
+test('isCountExpression matches a COUNT(...) call, including nested calls', () => {
+  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('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);
+});
+
+// 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'));
+  const expandToggles = await screen.findAllByLabelText(/expand row/i);
+  // Rows sort by metric id descending, so `COUNT(*)` (id 7) is first.
+  await userEvent.click(expandToggles[0]);
+
+  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'));
+  const expandToggles = await screen.findAllByLabelText(/expand row/i);
+  await userEvent.click(expandToggles[0]);
+
+  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), ',.0f');
+
+  await waitPastDebounce();
+  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'));
+  const expandToggles = await screen.findAllByLabelText(/expand row/i);
+  // Rows sort by metric id descending, so id 1 (`SUM(...)`) sorts last.
+  await userEvent.click(expandToggles[6]);
+
+  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
+
+  await waitPastDebounce();
+  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'));
+  const expandToggles = await screen.findAllByLabelText(/expand row/i);
+  // The appended metric (id 99) sorts first.
+  await userEvent.click(expandToggles[0]);
+
+  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
+
+  await waitPastDebounce();
+  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'));
+  const expandToggles = await screen.findAllByLabelText(/expand row/i);
+  await userEvent.click(expandToggles[0]);
+
+  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), 'foo%');
+
+  await waitPastDebounce();
+  expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
+});

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>
   );