[GitHub] [superset] villebro commented on a diff in pull request #19842: fix: count(distinct column_name) in metrics

[GitBox <gi...@apache.org>]

villebro commented on code in PR #19842:
URL: https://github.com/apache/superset/pull/19842#discussion_r858325003


##########
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.js:
##########
@@ -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)})`;
+      }

Review Comment:
   I had to reread these lines a few times to understand what was going on (mostly there from before this PR so not your fault!). IMO it would be more readable if we could first do something like
   ```js
   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
       : '';
   ```
   and then something like
   ```js
   if (params.transformCountDistinct && aggregate === AGGREGATES.COUNT_DISTINCT) {
     return `COUNT(DISTINCT ${column})`;
   }
   return `${aggregate}($column)`;
   ```



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org