Support converting aggregation references to legacy references (#37705)

* Support converting aggregation references to legacy references
* Pass query and stageIndex to legacyQuery

---------

Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>

frontend/src/metabase-lib/expressions/diagnostics.ts

@@ -137,14 +137,14 @@ function prattCompiler({
         throw new ResolverError(t`Unknown Metric: ${name}`, node);
       }
 
-      return Lib.legacyRef(metric);
+      return Lib.legacyRef(query, stageIndex, metric);
     } else if (kind === "segment") {
       const segment = parseSegment(name, options);
       if (!segment) {
         throw new ResolverError(t`Unknown Segment: ${name}`, node);
       }
 
-      return Lib.legacyRef(segment);
+      return Lib.legacyRef(query, stageIndex, segment);
     } else {
       const reference = options.name ?? ""; // avoid circular reference
 
@@ -154,7 +154,7 @@ function prattCompiler({
         throw new ResolverError(t`Unknown Field: ${name}`, node);
       }
 
-      return Lib.legacyRef(dimension);
+      return Lib.legacyRef(query, stageIndex, dimension);
     }
   }
 

frontend/src/metabase-lib/expressions/format.ts

@@ -135,7 +135,7 @@ function formatMetric([, metricId]: FieldReference, options: Options) {
   }
 
   const metric = Lib.availableMetrics(query).find(metric => {
-    const [_, availableMetricId] = Lib.legacyRef(metric);
+    const [_, availableMetricId] = Lib.legacyRef(query, stageIndex, metric);
 
     return availableMetricId === metricId;
   });
@@ -177,7 +177,7 @@ function formatSegment([, segmentId]: FieldReference, options: Options) {
   }
 
   const segment = Lib.availableSegments(query, stageIndex).find(segment => {
-    const [_, availableSegmentId] = Lib.legacyRef(segment);
+    const [_, availableSegmentId] = Lib.legacyRef(query, stageIndex, segment);
 
     return availableSegmentId === segmentId;
   });

frontend/src/metabase-lib/expressions/process.ts

@@ -21,14 +21,14 @@ export function processSource(options: {
         throw new Error(t`Unknown Metric: ${name}`);
       }
 
-      return Lib.legacyRef(metric);
+      return Lib.legacyRef(query, stageIndex, metric);
     } else if (kind === "segment") {
       const segment = parseSegment(name, options);
       if (!segment) {
         throw new Error(t`Unknown Segment: ${name}`);
       }
 
-      return Lib.legacyRef(segment);
+      return Lib.legacyRef(query, stageIndex, segment);
     } else {
       const reference = options.name ?? ""; // avoid circular reference
 
@@ -38,7 +38,7 @@ export function processSource(options: {
         throw new Error(t`Unknown Field: ${name}`);
       }
 
-      return Lib.legacyRef(dimension);
+      return Lib.legacyRef(query, stageIndex, dimension);
     }
   };
 

frontend/src/metabase-lib/fields.ts

@@ -52,7 +52,9 @@ export function fieldValuesSearchInfo(
 }
 
 export function legacyRef(
+  query: Query,
+  stageIndex: number,
   column: ColumnMetadata | MetricMetadata | SegmentMetadata,
 ): FieldReference {
-  return ML.legacy_ref(column);
+  return ML.legacy_ref(query, stageIndex, column);
 }

frontend/src/metabase-lib/parameters/utils/click-behavior.ts

@@ -149,7 +149,7 @@ function getTargetsForStructuredQuestion(question: Question): Target[] {
   return visibleColumns.map(targetColumn => {
     const dimension: ClickBehaviorDimensionTarget["dimension"] = [
       "dimension",
-      Lib.legacyRef(targetColumn),
+      Lib.legacyRef(query, stageIndex, targetColumn),
     ];
     const id = JSON.stringify(dimension);
     const target: ClickBehaviorTarget = { type: "dimension", id, dimension };

frontend/src/metabase-lib/parameters/utils/mbql.js

@@ -195,7 +195,7 @@ function fieldFilterParameterToMBQL(query, stageIndex, parameter) {
   }
 
   const column = columns[columnIndex];
-  const fieldRef = Lib.legacyRef(column);
+  const fieldRef = Lib.legacyRef(query, stageIndex, column);
 
   if (isDateParameter(parameter)) {
     return dateParameterValueToMBQL(parameter.value, fieldRef);

frontend/src/metabase-lib/parameters/utils/targets.ts

@@ -42,8 +42,12 @@ export function buildDimensionTarget(dimension: Dimension) {
   return ["dimension", dimension.mbql()];
 }
 
-export function buildColumnTarget(column: Lib.ColumnMetadata) {
-  return ["dimension", Lib.legacyRef(column)];
+export function buildColumnTarget(
+  query: Lib.Query,
+  stageIndex: number,
+  column: Lib.ColumnMetadata,
+) {
+  return ["dimension", Lib.legacyRef(query, stageIndex, column)];
 }
 
 export function buildTemplateTagVariableTarget(variable: TemplateTagVariable) {

frontend/src/metabase/parameters/utils/mapping-options.js

@@ -29,7 +29,7 @@ function buildStructuredQuerySectionOptions(query, stageIndex, group) {
       sectionName: getColumnGroupName(groupInfo),
       name: columnInfo.displayName,
       icon: getColumnIcon(column),
-      target: buildColumnTarget(column),
+      target: buildColumnTarget(query, stageIndex, column),
       isForeign: columnInfo.isFromJoin || columnInfo.isImplicitlyJoinable,
     };
   });

frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts

@@ -224,7 +224,7 @@ export const addColumnInSettings = (
   if (settingIndex >= 0) {
     newSettings[settingIndex] = { ...newSettings[settingIndex], enabled: true };
   } else {
-    const fieldRef = Lib.legacyRef(column);
+    const fieldRef = Lib.legacyRef(query, STAGE_INDEX, column);
     newSettings.push({ name, fieldRef, enabled: true });
   }
 

src/metabase/lib/convert.cljc

@@ -407,9 +407,11 @@
 (defmethod ->legacy-MBQL :aggregation [[_ opts agg-uuid :as ag]]
   (if (map? opts)
     (try
-      (let [opts (options->legacy-MBQL opts)]
-        (cond-> [:aggregation (get-or-throw! *pMBQL-uuid->legacy-index* agg-uuid)]
-          opts (conj opts)))
+      (let [opts (options->legacy-MBQL opts)
+            base-agg [:aggregation (get-or-throw! *pMBQL-uuid->legacy-index* agg-uuid)]]
+        (if (seq opts)
+          [:aggregation-options base-agg opts]
+          base-agg))
       (catch #?(:clj Throwable :cljs :default) e
         (throw (ex-info (lib.util/format "Error converting aggregation reference to pMBQL: %s" (ex-message e))
                         {:ref ag}

src/metabase/lib/js.cljs

@@ -663,7 +663,7 @@
 
 (defn- normalize-legacy-ref
   [a-ref]
-  (if (#{:metric :segment} (first a-ref))
+  (if (#{:aggregation :metric :segment} (first a-ref))
     (subvec a-ref 0 2)
     (update a-ref 2 update-vals #(if (qualified-keyword? %)
                                    (u/qualified-name %)
@@ -673,12 +673,13 @@
   "Given a column, metric or segment metadata from eg. [[fieldable-columns]] or [[available-segments]],
   return it as a legacy JSON field ref.
   For compatibility reasons, segment and metric references are always returned without options."
-  [column]
-  (-> column
-      lib.core/ref
-      lib.convert/->legacy-MBQL
-      normalize-legacy-ref
-      clj->js))
+  [a-query stage-number column]
+  (lib.convert/with-aggregation-list (:aggregation (lib.util/query-stage a-query stage-number))
+    (-> column
+        lib.core/ref
+        lib.convert/->legacy-MBQL
+        normalize-legacy-ref
+        clj->js)))
 
 (defn- legacy-ref->pMBQL [a-legacy-ref]
   (-> a-legacy-ref

test/metabase/lib/convert_test.cljc

@@ -205,19 +205,19 @@
     (is (=? {:type  :query
              :query {:source-table 1
                      :aggregation  [[:sum [:field 1 nil]]]
-                     :breakout     [[:aggregation 0 {:display-name "Revenue"}]]}}
+                     :breakout     [[:aggregation-options [:aggregation 0] {:display-name "Revenue"}]]}}
             (let [ag-uuid (str (random-uuid))]
               (lib.convert/->legacy-MBQL
-                {:lib/type :mbql/query
-                 :stages   [{:lib/type     :mbql.stage/mbql
-                             :source-table 1
-                             :aggregation  [[:sum {:lib/uuid ag-uuid}
-                                             [:field {:lib/uuid string?
-                                                      :effective-type :type/Integer} 1]]]
-                             :breakout     [[:aggregation
-                                             {:display-name   "Revenue"
-                                              :effective-type :type/Integer}
-                                             ag-uuid]]}]}))))))
+               {:lib/type :mbql/query
+                :stages   [{:lib/type     :mbql.stage/mbql
+                            :source-table 1
+                            :aggregation  [[:sum {:lib/uuid ag-uuid}
+                                            [:field {:lib/uuid string?
+                                                     :effective-type :type/Integer} 1]]]
+                            :breakout     [[:aggregation
+                                            {:display-name   "Revenue"
+                                             :effective-type :type/Integer}
+                                            ag-uuid]]}]}))))))
 
 (deftest ^:parallel source-card-test
   (let [original {:database 1

test/metabase/lib/js_test.cljs

@@ -292,7 +292,7 @@
         query (lib/query metadata-provider (meta/table-metadata :venues))
 
         array-checker #(when (array? %) (js->clj %))
-        to-legacy-refs (comp array-checker lib.js/legacy-ref)]
+        to-legacy-refs (comp array-checker #(lib.js/legacy-ref query -1 %))]
     (testing "field refs come with options"
       (is (= [["field" (meta/id :venues :id) {"base-type" "type/BigInteger"}]
               ["field" (meta/id :venues :name) {"base-type" "type/Text"}]
@@ -301,14 +301,34 @@
               ["field" (meta/id :venues :longitude) {"base-type" "type/Float"}]
               ["field" (meta/id :venues :price) {"base-type" "type/Integer"}]]
              (->> query lib/returned-columns (map to-legacy-refs)))))
-    (let [legacy-refs (->> query lib/available-segments (map lib.js/legacy-ref))]
+    (testing "segment refs come without options"
+      (is (= [["segment" segment-id]]
+             (->> query lib/available-segments (map to-legacy-refs)))))
+    (testing "metric refs come without options"
+      (is (= [["metric" metric-id]]
+             (->> query lib/available-metrics (map to-legacy-refs)))))
+    (testing "aggregation references (#37698)"
+      (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
+                      (lib/aggregate (lib/sum (meta/field-metadata :venues :price)))
+                      ;; The display-name set here gets lost when the corresponding column
+                      ;; is converted to a ref. Currently there is no option that would
+                      ;; survive the aggregation -> column -> ref -> legacy ref conversion
+                      ;; (cf. column-metadata->aggregation-ref and options->legacy-MBQL).
+                      #_(lib/aggregate (lib.options/update-options (lib/avg (meta/field-metadata :venues :price))
+                                                                   assoc :display-name "avg price")))]
+        (is (= [["aggregation" 0]
+                ;; add this back when the second aggregation is added back
+                #_["aggregation-options" ["aggregation 1"] {"display-name" "avg price"}]]
+               (map (comp array-checker #(lib.js/legacy-ref query -1 %))
+                    (lib.js/returned-columns query -1))))))
+    (let [legacy-refs (->> query lib/available-segments (map #(lib.js/legacy-ref query -1 %)))]
       (testing "legacy segment refs come without options"
         (is (= [["segment" segment-id]] (map array-checker legacy-refs))))
       (testing "segment legacy ref can be converted to an expression and back (#37173)"
         (let [segment-expr (lib.js/expression-clause-for-legacy-expression query -1 (first legacy-refs))]
           (is (=? [:segment {} segment-id] segment-expr))
           (is (= ["segment" segment-id] (js->clj (lib.js/legacy-expression-for-expression-clause query -1 segment-expr)))))))
-    (let [legacy-refs (->> query lib/available-metrics (map lib.js/legacy-ref))]
+    (let [legacy-refs (->> query lib/available-metrics (map #(lib.js/legacy-ref query -1 %)))]
       (testing "metric refs come without options"
         (is (= [["metric" metric-id]] (map array-checker legacy-refs))))
       (testing "metric legacy ref can be converted to an expression and back (#37173)"