fix(Table chart): fix percentage metric column (apache#34175)

LevisNgigi · web-flow · commit 53503e32ae92 · 2025-08-06T17:51:02.000+03:00
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts
@@ -29,3 +29,4 @@ export * from './getStandardizedControls';
 export * from './getTemporalColumns';
 export * from './displayTimeRelatedControls';
 export * from './colorControls';
+export * from './metricColumnFilter';
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/metricColumnFilter.test.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/metricColumnFilter.test.ts
@@ -0,0 +1,135 @@
+/**
+ * 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 { QueryFormMetric, SqlaFormData } from '@superset-ui/core';
+import {
+  shouldSkipMetricColumn,
+  isRegularMetric,
+  isPercentMetric,
+} from './metricColumnFilter';
+
+const createMetric = (label: string): QueryFormMetric =>
+  ({
+    label,
+    expressionType: 'SIMPLE',
+    column: { column_name: label },
+    aggregate: 'SUM',
+  }) as QueryFormMetric;
+
+describe('metricColumnFilter', () => {
+  const createFormData = (
+    metrics: string[],
+    percentMetrics: string[],
+  ): SqlaFormData =>
+    ({
+      datasource: 'test_datasource',
+      viz_type: 'table',
+      metrics: metrics.map(createMetric),
+      percent_metrics: percentMetrics.map(createMetric),
+    }) as SqlaFormData;
+
+  describe('shouldSkipMetricColumn', () => {
+    it('should skip unprefixed percent metric columns if prefixed version exists', () => {
+      const colnames = ['metric1', '%metric1'];
+      const formData = createFormData([], ['metric1']);
+
+      const result = shouldSkipMetricColumn({
+        colname: 'metric1',
+        colnames,
+        formData,
+      });
+
+      expect(result).toBe(true);
+    });
+
+    it('should not skip if column is also a regular metric', () => {
+      const colnames = ['metric1', '%metric1'];
+      const formData = createFormData(['metric1'], ['metric1']);
+
+      const result = shouldSkipMetricColumn({
+        colname: 'metric1',
+        colnames,
+        formData,
+      });
+
+      expect(result).toBe(false);
+    });
+
+    it('should not skip if column starts with %', () => {
+      const colnames = ['%metric1'];
+      const formData = createFormData(['metric1'], []);
+
+      const result = shouldSkipMetricColumn({
+        colname: '%metric1',
+        colnames,
+        formData,
+      });
+
+      expect(result).toBe(false);
+    });
+
+    it('should not skip if no prefixed version exists', () => {
+      const colnames = ['metric1'];
+      const formData = createFormData([], ['metric1']);
+
+      const result = shouldSkipMetricColumn({
+        colname: 'metric1',
+        colnames,
+        formData,
+      });
+
+      expect(result).toBe(false);
+    });
+  });
+
+  describe('isRegularMetric', () => {
+    it('should return true for regular metrics', () => {
+      const formData = createFormData(['metric1', 'metric2'], []);
+      expect(isRegularMetric('metric1', formData)).toBe(true);
+      expect(isRegularMetric('metric2', formData)).toBe(true);
+    });
+
+    it('should return false for non-metrics', () => {
+      const formData = createFormData(['metric1'], []);
+      expect(isRegularMetric('non_metric', formData)).toBe(false);
+    });
+
+    it('should return false for percentage metrics', () => {
+      const formData = createFormData([], ['percent_metric1']);
+      expect(isRegularMetric('percent_metric1', formData)).toBe(false);
+    });
+  });
+
+  describe('isPercentMetric', () => {
+    it('should return true for percentage metrics', () => {
+      const formData = createFormData([], ['percent_metric1']);
+      expect(isPercentMetric('%percent_metric1', formData)).toBe(true);
+    });
+
+    it('should return false for non-percentage metrics', () => {
+      const formData = createFormData(['regular_metric'], []);
+      expect(isPercentMetric('regular_metric', formData)).toBe(false);
+    });
+
+    it('should return false for regular metrics', () => {
+      const formData = createFormData(['metric1'], []);
+      expect(isPercentMetric('metric1', formData)).toBe(false);
+    });
+  });
+});
diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/metricColumnFilter.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/metricColumnFilter.ts
@@ -0,0 +1,95 @@
+/**
+ * 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 {
+  QueryFormMetric,
+  getMetricLabel,
+  SqlaFormData,
+} from '@superset-ui/core';
+
+export interface MetricColumnFilterParams {
+  colname: string;
+  colnames: string[];
+  formData: SqlaFormData;
+}
+
+/**
+ * Determines if a column should be skipped based on metric filtering logic.
+ *
+ * This function implements the logic to skip unprefixed percent metric columns
+ * if a prefixed version exists, but doesn't skip if it's also a regular metric.
+ *
+ * @param params - The parameters for metric column filtering
+ * @returns true if the column should be skipped, false otherwise
+ */
+export function shouldSkipMetricColumn({
+  colname,
+  colnames,
+  formData,
+}: MetricColumnFilterParams): boolean {
+  if (!colname) {
+    return false;
+  }
+
+  // Check if this column name exists as a percent metric in form data
+  const isPercentMetric = formData.percent_metrics?.some(
+    (metric: QueryFormMetric) => getMetricLabel(metric) === colname,
+  );
+
+  // Check if this column name exists as a regular metric in form data
+  const isRegularMetric = formData.metrics?.some(
+    (metric: QueryFormMetric) => getMetricLabel(metric) === colname,
+  );
+
+  // Check if there's a prefixed version of this column in the column list
+  const hasPrefixedVersion = colnames.includes(`%${colname}`);
+
+  // Skip if: has prefixed version AND is percent metric AND is NOT regular metric
+  return hasPrefixedVersion && isPercentMetric && !isRegularMetric;
+}
+
+/**
+ * Determines if a column is a regular metric.
+ *
+ * @param colname - The column name to check
+ * @param formData - The form data containing metrics
+ * @returns true if the column is a regular metric, false otherwise
+ */
+export function isRegularMetric(
+  colname: string,
+  formData: SqlaFormData,
+): boolean {
+  return !!formData.metrics?.some(metric => getMetricLabel(metric) === colname);
+}
+
+/**
+ * Determines if a column is a percentage metric.
+ *
+ * @param colname: string,
+ * @param formData - The form data containing percent_metrics
+ * @returns true if the column is a percentage metric, false otherwise
+ */
+export function isPercentMetric(
+  colname: string,
+  formData: SqlaFormData,
+): boolean {
+  return !!formData.percent_metrics?.some(
+    (metric: QueryFormMetric) => `%${getMetricLabel(metric)}` === colname,
+  );
+}
diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx
@@ -36,18 +36,19 @@ import {
   QueryModeLabel,
   sections,
   sharedControls,
+  shouldSkipMetricColumn,
+  isRegularMetric,
+  isPercentMetric,
 } from '@superset-ui/chart-controls';
 import {
   ensureIsArray,
   FeatureFlag,
   GenericDataType,
-  getMetricLabel,
   isAdhocColumn,
   isFeatureEnabled,
   isPhysicalColumn,
   legacyValidateInteger,
   QueryFormColumn,
-  QueryFormMetric,
   QueryMode,
   SMART_DATE_ID,
   t,
@@ -533,14 +534,25 @@ const config: ControlPanelConfig = {
                     )
                     .forEach((colname, index) => {
                       if (
-                        explore.form_data.metrics?.some(
-                          metric => getMetricLabel(metric) === colname,
-                        ) ||
-                        explore.form_data.percent_metrics?.some(
-                          (metric: QueryFormMetric) =>
-                            getMetricLabel(metric) === colname,
-                        )
+                        shouldSkipMetricColumn({
+                          colname,
+                          colnames,
+                          formData: explore.form_data,
+                        })
                       ) {
+                        return;
+                      }
+
+                      const isMetric = isRegularMetric(
+                        colname,
+                        explore.form_data,
+                      );
+                      const isPercentMetricValue = isPercentMetric(
+                        colname,
+                        explore.form_data,
+                      );
+
+                      if (isMetric || isPercentMetricValue) {
                         const comparisonColumns =
                           generateComparisonColumns(colname);
                         comparisonColumns.forEach((name, idx) => {
diff --git a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx
@@ -36,16 +36,17 @@ import {
   QueryModeLabel,
   sections,
   sharedControls,
+  shouldSkipMetricColumn,
+  isRegularMetric,
+  isPercentMetric,
 } from '@superset-ui/chart-controls';
 import {
   ensureIsArray,
   GenericDataType,
-  getMetricLabel,
   isAdhocColumn,
   isPhysicalColumn,
   legacyValidateInteger,
   QueryFormColumn,
-  QueryFormMetric,
   QueryMode,
   SMART_DATE_ID,
   t,
@@ -589,15 +590,29 @@ const config: ControlPanelConfig = {
                         last(colname.split('__')) !== timeComparisonValue,
                     )
                     .forEach((colname, index) => {
+                      // Skip unprefixed percent metric columns if a prefixed version exists
+                      // But don't skip if it's also a regular metric
                       if (
-                        explore.form_data.metrics?.some(
-                          metric => getMetricLabel(metric) === colname,
-                        ) ||
-                        explore.form_data.percent_metrics?.some(
-                          (metric: QueryFormMetric) =>
-                            getMetricLabel(metric) === colname,
-                        )
+                        shouldSkipMetricColumn({
+                          colname,
+                          colnames,
+                          formData: explore.form_data,
+                        })
                       ) {
+                        return;
+                      }
+
+                      const isMetric = isRegularMetric(
+                        colname,
+                        explore.form_data,
+                      );
+                      const isPercentMetricValue = isPercentMetric(
+                        colname,
+                        explore.form_data,
+                      );
+
+                      // Generate comparison columns for metrics (time comparison feature)
+                      if (isMetric || isPercentMetricValue) {
                         const comparisonColumns =
                           generateComparisonColumns(colname);
                         comparisonColumns.forEach((name, idx) => {
diff --git a/superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx b/superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx
@@ -101,8 +101,8 @@ export default memo(function ColumnConfigItem({
       )}
       trigger="click"
       placement="right"
-      styles={{ body: { width, height } }}
-      rootClassName="column-config-popover"
+      style={{ width, height }}
+      className="column-config-popover"
     >
       <div css={outerContainerStyle}>
         <div css={nameContainerStyle}>
@@ -117,7 +117,7 @@ export default memo(function ColumnConfigItem({
               iconColor={colors.grayscale.base}
             />
           )}
-          <Icons.CaretRightOutlined css={caretIconStyle} />
+          <Icons.RightOutlined css={caretIconStyle} />
         </div>
       </div>
     </Popover>
