From 25e572a56e8cca1c9dd466fcd64ad610e86a385c Mon Sep 17 00:00:00 2001
From: Yongjie Zhao <yongjie.zhao@gmail.com>
Date: Tue, 26 Apr 2022 20:03:55 +0800
Subject: [PATCH] fix: count(distinct column_name) in metrics (#19842)

---
 .../controls/MetricControl/AdhocMetric.js     | 21 ++++++++---
 .../MetricControl/AdhocMetric.test.js         | 36 +++++++++++++++++++
 .../AdhocMetricEditPopover/index.jsx          |  5 ++-
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js
index 01fea2dab69f..5b29d7418c2f 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js
@@ -16,7 +16,10 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { sqlaAutoGeneratedMetricRegex } from 'src/explore/constants';
+import {
+  sqlaAutoGeneratedMetricRegex,
+  AGGREGATES,
+} from 'src/explore/constants';
 
 export const EXPRESSION_TYPES = {
   SIMPLE: 'SIMPLE',
@@ -86,20 +89,30 @@ export default class AdhocMetric {
   }
 
   getDefaultLabel() {
-    const label = this.translateToSql(true);
+    const label = this.translateToSql({ useVerboseName: true });
     return label.length < 43 ? label : `${label.substring(0, 40)}...`;
   }
 
-  translateToSql(useVerboseName = false) {
+  translateToSql(
+    params = { useVerboseName: false, transformCountDistinct: false },
+  ) {
     if (this.expressionType === EXPRESSION_TYPES.SIMPLE) {
       const aggregate = this.aggregate || '';
       // eslint-disable-next-line camelcase
       const column =
-        useVerboseName && this.column?.verbose_name
+        params.useVerboseName && this.column?.verbose_name
           ? `(${this.column.verbose_name})`
           : this.column?.column_name
           ? `(${this.column.column_name})`
           : '';
+      // transform from `count_distinct(column)` to `count(distinct column)`
+      if (
+        params.transformCountDistinct &&
+        aggregate === AGGREGATES.COUNT_DISTINCT &&
+        /^\(.*\)$/.test(column)
+      ) {
+        return `COUNT(DISTINCT ${column.slice(1, -1)})`;
+      }
       return aggregate + column;
     }
     if (this.expressionType === EXPRESSION_TYPES.SQL) {
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js
index f3b7d08dca4a..336b194e0024 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.js
@@ -216,4 +216,40 @@ describe('AdhocMetric', () => {
     expect(adhocMetric3.aggregate).toBe(AGGREGATES.AVG);
     expect(adhocMetric3.column.column_name).toBe('value');
   });
+
+  it('should transform count_distinct SQL and do not change label if does not set metric label', () => {
+    const withBrackets = new AdhocMetric({
+      column: { type: 'TEXT', column_name: '(column-with-barckets)' },
+      aggregate: AGGREGATES.COUNT_DISTINCT,
+      hasCustomLabel: false,
+    });
+    expect(withBrackets.translateToSql({ transformCountDistinct: true })).toBe(
+      'COUNT(DISTINCT (column-with-barckets))',
+    );
+    expect(withBrackets.getDefaultLabel()).toBe(
+      'COUNT_DISTINCT((column-with-barckets))',
+    );
+
+    const withoutBrackets = new AdhocMetric({
+      column: { type: 'TEXT', column_name: 'column-without-barckets' },
+      aggregate: AGGREGATES.COUNT_DISTINCT,
+      hasCustomLabel: false,
+    });
+    expect(
+      withoutBrackets.translateToSql({ transformCountDistinct: true }),
+    ).toBe('COUNT(DISTINCT column-without-barckets)');
+    expect(withoutBrackets.getDefaultLabel()).toBe(
+      'COUNT_DISTINCT(column-without-barckets)',
+    );
+
+    const emptyColumnName = new AdhocMetric({
+      column: { type: 'TEXT', column_name: '' },
+      aggregate: AGGREGATES.COUNT_DISTINCT,
+      hasCustomLabel: false,
+    });
+    expect(
+      emptyColumnName.translateToSql({ transformCountDistinct: true }),
+    ).toBe('COUNT_DISTINCT');
+    expect(emptyColumnName.getDefaultLabel()).toBe('COUNT_DISTINCT');
+  });
 });
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
index be5d15ffb5d8..decad4c12d5d 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
@@ -465,7 +465,10 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
               onChange={this.onSqlExpressionChange}
               width="100%"
               showGutter={false}
-              value={adhocMetric.sqlExpression || adhocMetric.translateToSql()}
+              value={
+                adhocMetric.sqlExpression ||
+                adhocMetric.translateToSql({ transformCountDistinct: true })
+              }
               editorProps={{ $blockScrolling: true }}
               enableLiveAutocompletion
               className="filter-sql-editor"