Make with-expression-name set the display-name of the clause (#36203)

Co-authored-by: Kamil Mielnik <kamil@kamilmielnik.com>

frontend/src/metabase-lib/expression.ts

@@ -21,10 +21,6 @@ export function expression(
   return ML.expression(query, stageIndex, expressionName, clause);
 }
 
-export function expressionName(clause: ExpressionClause): string {
-  return ML.expression_name(clause);
-}
-
 export function withExpressionName(
   clause: ExpressionClause,
   newName: string,

frontend/src/metabase-lib/types.ts

@@ -174,7 +174,9 @@ export type MetricDisplayInfo = {
 export type ClauseDisplayInfo = Pick<
   ColumnDisplayInfo,
   "name" | "displayName" | "longDisplayName" | "table"
->;
+> & {
+  isNamed?: boolean;
+};
 
 export type AggregationClauseDisplayInfo = ClauseDisplayInfo;
 

src/metabase/lib/core.cljc

@@ -115,7 +115,6 @@
   find-matching-column]
  [lib.expression
   expression
-  expression-name
   expressions
   expressions-metadata
   expressionable-columns

src/metabase/lib/expression.cljc

@@ -347,16 +347,20 @@
         (expression-metadata query stage-number)
         lib.ref/ref)))
 
-(mu/defn expression-name :- :string
-  "Return the name of `an-expression-clause`."
-  [an-expression-clause :- ::lib.schema.expression/expression]
-  (-> an-expression-clause lib.options/options :lib/expression-name))
-
 (mu/defn with-expression-name :- ::lib.schema.expression/expression
-  "Return a new expression clause like `an-expression-clause` but with name `new-name`."
+  "Return a new expression clause like `an-expression-clause` but with name `new-name`.
+  For expressions from the :expressions clause of a pMBQL query this sets the :lib/expression-name option,
+  for other expressions (for example named aggregation expressions) the :display-name option is set.
+
+  Note that always setting :lib/expression-name would lead to confusion, because that option is used
+  to decide what kind of reference is to be created. For example, expression are referenced by name,
+  aggregations are referenced by position."
   [an-expression-clause :- ::lib.schema.expression/expression
    new-name :- :string]
   (lib.options/update-options
-   an-expression-clause assoc
-   :lib/expression-name new-name
-   :lib/uuid (str (random-uuid))))
+   an-expression-clause
+   (fn [opts]
+     (let [opts (assoc opts :lib/uuid (str (random-uuid)))]
+       (if (:lib/expression-name opts)
+         (assoc opts :lib/expression-name new-name)
+         (assoc opts :display-name new-name))))))

src/metabase/lib/js.cljs

@@ -696,13 +696,10 @@
   [a-query stage-number expression-name an-expression-clause]
   (lib.core/expression a-query stage-number expression-name an-expression-clause))
 
-(defn ^:export expression-name
-  "Return the name of `an-expression-clause`."
-  [an-expression-clause]
-  (lib.core/expression-name an-expression-clause))
-
 (defn ^:export with-expression-name
-  "Return an new expressions clause like `an-expression-clause` but with name `new-name`."
+  "Return a new expression clause like `an-expression-clause` but with name `new-name`.
+  For expressions from the :expressions clause of a pMBQL query this sets the :lib/expression-name option,
+  for other expressions (for example named aggregation expressions) the :display-name option is set."
   [an-expression-clause new-name]
   (lib.core/with-expression-name an-expression-clause new-name))
 

src/metabase/lib/metadata/calculation.cljc

@@ -67,7 +67,7 @@
     style        :- DisplayNameStyle]
    (or
     ;; if this is an MBQL clause with `:display-name` in the options map, then use that rather than calculating a name.
-    (:display-name (lib.options/options x))
+    ((some-fn :display-name :lib/expression-name) (lib.options/options x))
     (try
       (display-name-method query stage-number x style)
       (catch #?(:clj Throwable :cljs js/Error) e
@@ -97,7 +97,6 @@
 
 (defmethod display-name-method :default
   [_query _stage-number x _stage]
-  ;; hopefully this is dev-facing only, so not i18n'ed.
   (log/warnf "Don't know how to calculate display name for %s. Add an impl for %s for %s"
              (pr-str x)
              `display-name-method
@@ -279,6 +278,8 @@
               [:map
                [:display-name {:optional true} :string]
                [:long-display-name {:optional true} :string]
+               ;; for things with user specified names
+               [:named? {:optional true} :boolean]
    ;; for things that have a Table, e.g. a Field
                [:table {:optional true} [:maybe [:ref ::display-info]]]
    ;; these are derived from the `:lib/source`/`:metabase.lib.schema.metadata/column-source`, but instead of using
@@ -338,6 +339,24 @@
                            {:query query, :stage-number stage-number, :x x}
                            e))))))))
 
+(defmulti custom-name-method
+  "Implementation for [[custom-name]]."
+  {:arglists '([x])}
+  lib.dispatch/dispatch-value
+  :hierarchy lib.hierarchy/hierarchy)
+
+(defn custom-name
+  "Return the user supplied name of `x`, if any."
+  [x]
+  (custom-name-method x))
+
+(defmethod custom-name-method :default
+  [x]
+  ;; We assume that clauses only get a :display-name option if the user explicitly specifies it.
+  ;; Expressions from the :expressions clause of pMBQL queries have custom names by default.
+  (when (lib.util/clause? x)
+    ((some-fn :display-name :lib/expression-name) (lib.options/options x))))
+
 (defn default-display-info
   "Default implementation of [[display-info-method]], available in case you want to use this in a different
   implementation and add additional information to it."
@@ -347,6 +366,9 @@
      ;; TODO -- not 100% convinced the FE should actually have access to `:name`, can't it use `:display-name`
      ;; everywhere? Determine whether or not this is the case.
      (select-keys x-metadata [:name :display-name :semantic-type])
+     (when-let [custom (custom-name x)]
+       {:display-name custom
+        :named? true})
      (when-let [long-display-name (display-name query stage-number x :long)]
        {:long-display-name long-display-name})
      ;; don't return `:base-type`, FE should just use `:effective-type` everywhere and not even need to know

src/metabase/lib/util.cljc

@@ -41,10 +41,10 @@
   "Returns true if this is a clause."
   [clause]
   (and (vector? clause)
-       (> (count clause) 1)
        (keyword? (first clause))
-       (map? (second clause))
-       (contains? (second clause) :lib/uuid)))
+       (let [opts (get clause 1)]
+         (and (map? opts)
+              (contains? opts :lib/uuid)))))
 
 (defn clause-of-type?
   "Returns true if this is a clause."

test/metabase/lib/expression_test.cljc

@@ -244,11 +244,12 @@
             (-> lib.tu/venues-query
                 (lib/expression "expr" (lib/absolute-datetime "2020" :month))
                 lib/expressions-metadata)))
-    (is (= ["expr"]
-           (-> lib.tu/venues-query
-               (lib/expression "expr" (lib/absolute-datetime "2020" :month))
-               lib/expressions
-               (->> (map lib/expression-name))))))
+    (is (=? [{:display-name "expr"
+              :named? true}]
+            (-> lib.tu/venues-query
+                (lib/expression "expr" (lib/absolute-datetime "2020" :month))
+                lib/expressions
+                (->> (map (fn [expr] (lib/display-info lib.tu/venues-query expr))))))))
   (testing "collisions with other column names are detected and rejected"
     (let [query (lib/query meta/metadata-provider (meta/table-metadata :categories))
           ex    (try
@@ -358,17 +359,46 @@
         (is (empty? (lib/expressions dropped)))))))
 
 (deftest ^:parallel with-expression-name-test
-  (let [query (-> lib.tu/venues-query
-                  (lib/expression "expr" (lib/absolute-datetime "2020" :month)))
-        [orig-expr :as orig-exprs] (lib/expressions query)
-        expr (lib/with-expression-name orig-expr "newly-named-expression")]
+  (let [query       (-> lib.tu/venues-query
+                        (lib/expression "expr" (lib/absolute-datetime "2020" :month))
+                        (lib/aggregate (lib/count)))
+        [orig-expr] (lib/expressions query)
+        expr        (lib/with-expression-name orig-expr "newly-named-expression")
+        [orig-agg]  (lib/aggregations query)
+        agg         (lib/with-expression-name orig-agg "my count")]
     (testing "expressions should include the original expression name"
       (is (=? [{:name         "expr"
                 :display-name "expr"}]
-              (lib/expressions-metadata query)))
-      (is (= ["expr"]
-             (map lib/expression-name orig-exprs)))
+              (lib/expressions-metadata query))))
+    (testing "expressions from the expressions query clause can be renamed"
+      (is (= "newly-named-expression"
+             (lib/display-name query expr)))
+      (is (nil? (:display-name (lib.options/options expr))))
+      (is (=? {:display-name "expr"
+               :named? true}
+              (lib/display-info query orig-expr)))
+      (is (= "expr"
+             (lib/display-name query orig-expr)))
+      (is (=? {:display-name "newly-named-expression"
+               :named? true}
+              (lib/display-info query expr)))
       (is (= "newly-named-expression"
-             (lib/expression-name expr)))
+             (lib/display-name query expr)))
       (is (not= (lib.options/uuid orig-expr)
-                (lib.options/uuid expr))))))
+                (lib.options/uuid expr))))
+    (testing "aggregation expressions can be renamed"
+      (is (= "my count"
+             (lib/display-name query agg)))
+      (is (nil? (:lib/expression-name (lib.options/options agg))))
+      (is (=? {:display-name "Count"
+               :named? (symbol "nil #_\"key is not present.\"")}
+              (lib/display-info query orig-agg)))
+      (is (= "Count"
+             (lib/display-name query orig-agg)))
+      (is (=? {:display-name "my count"
+               :named? true}
+              (lib/display-info query agg)))
+      (is (= "my count"
+             (lib/display-name query agg)))
+      (is (not= (lib.options/uuid orig-agg)
+                (lib.options/uuid agg))))))