msyavuz commented on code in PR #43453:
URL: https://github.com/apache/superset/pull/43453#discussion_r3845167138
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -779,6 +780,43 @@ function EditorsSelector({
const ResultTable =
extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable;
+// D3's '%' type is a valid spec that multiplies by 100, so it never trips
+// the "Invalid format" fallback even when applied to a raw count. Parsing
+// via the same registry the chart uses excludes garbage like "foo%" that
+// merely ends in '%' without being a valid D3 spec.
+export const isPercentD3Format = (d3format?: string): boolean => {
+ const trimmed = d3format?.trim();
+ return (
+ !!trimmed && trimmed.endsWith('%') &&
!getNumberFormatter(trimmed).isInvalid
Review Comment:
d3's `p` type also multiplies by 100 and appends `%`, so `.1p` on a
`COUNT(*)` still renders `8000000000%` unwarned — `endsWith('%')` misses it.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorMetricFormatWarning.test.tsx:
##########
@@ -0,0 +1,161 @@
+/**
+ * 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 } from 'spec/helpers/testing-library';
+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;
+
+test('isCountExpression matches only a bare COUNT(...) call', () => {
+ expect(isCountExpression('COUNT(*)')).toBe(true);
+ expect(isCountExpression('count( * )')).toBe(true);
+ expect(isCountExpression('COUNT(DISTINCT name)')).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 spec', () => {
+ expect(isPercentD3Format('.0%')).toBe(true);
+ expect(isPercentD3Format(',.2%')).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 waitFor(() =>
Review Comment:
This `waitFor` resolves on its first synchronous pass, ~250ms before
`TextControl`'s `FAST_DEBOUNCE` commits the typed value, so all four negative
tests pass whether or not the warning fires — making `isPercentD3Format` return
true for `,.0f` keeps them green.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -779,6 +780,43 @@ function EditorsSelector({
const ResultTable =
extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable;
+// D3's '%' type is a valid spec that multiplies by 100, so it never trips
+// the "Invalid format" fallback even when applied to a raw count. Parsing
+// via the same registry the chart uses excludes garbage like "foo%" that
+// merely ends in '%' without being a valid D3 spec.
+export const isPercentD3Format = (d3format?: string): boolean => {
+ const trimmed = d3format?.trim();
+ return (
+ !!trimmed && trimmed.endsWith('%') &&
!getNumberFormatter(trimmed).isInvalid
+ );
+};
+
+// Anchored to the whole expression so a ratio like `COUNT(*) / COUNT(*)`
+// isn't misclassified as a raw count just because it starts with COUNT(.
+export const isCountExpression = (expression?: string): boolean =>
+ !!expression && /^count\([^()]*\)$/i.test(expression.trim());
Review Comment:
`[^()]*` rejects nested calls, so `COUNT(DISTINCT COALESCE(a, b))` and
`COUNT(CASE WHEN x THEN 1 END)` get no warning — intentional?
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2140,7 +2178,7 @@ function DatasourceEditor({
}}
expandFieldset={
<FormContainer>
- <Fieldset compact>
+ <Fieldset compact renderWarning={renderMetricFormatWarning}>
Review Comment:
The same `.0%` typed into a chart's Number format control still renders
silently, since `y_axis_format`/`number_format` take precedence over the
metric's `d3format` at render time — is dataset-metric-only the intended scope
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]
