From 1692da23e9e3a46d0239e4ab44c343f82312faa0 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Tue, 30 Jan 2024 17:51:09 +0300
Subject: [PATCH] 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>
---
.../metabase-lib/expressions/diagnostics.ts | 6 ++---
.../src/metabase-lib/expressions/format.ts | 4 +--
.../src/metabase-lib/expressions/process.ts | 6 ++---
frontend/src/metabase-lib/fields.ts | 4 ++-
.../parameters/utils/click-behavior.ts | 2 +-
.../src/metabase-lib/parameters/utils/mbql.js | 2 +-
.../metabase-lib/parameters/utils/targets.ts | 8 ++++--
.../parameters/utils/mapping-options.js | 2 +-
.../ChartSettingTableColumns/utils.ts | 2 +-
src/metabase/lib/convert.cljc | 8 +++---
src/metabase/lib/js.cljs | 15 ++++++-----
test/metabase/lib/convert_test.cljc | 22 ++++++++--------
test/metabase/lib/js_test.cljs | 26 ++++++++++++++++---
13 files changed, 68 insertions(+), 39 deletions(-)
diff --git a/frontend/src/metabase-lib/expressions/diagnostics.ts b/frontend/src/metabase-lib/expressions/diagnostics.ts
index e4f71b77543..05887f1bf71 100644
--- a/frontend/src/metabase-lib/expressions/diagnostics.ts
+++ b/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);
}
}
diff --git a/frontend/src/metabase-lib/expressions/format.ts b/frontend/src/metabase-lib/expressions/format.ts
index b9f0f5bc818..7e6432b3e4a 100644
--- a/frontend/src/metabase-lib/expressions/format.ts
+++ b/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;
});
diff --git a/frontend/src/metabase-lib/expressions/process.ts b/frontend/src/metabase-lib/expressions/process.ts
index 5f9798aa490..67b3f0961ba 100644
--- a/frontend/src/metabase-lib/expressions/process.ts
+++ b/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);
}
};
diff --git a/frontend/src/metabase-lib/fields.ts b/frontend/src/metabase-lib/fields.ts
index 131195076f9..4df1f4637ba 100644
--- a/frontend/src/metabase-lib/fields.ts
+++ b/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);
}
diff --git a/frontend/src/metabase-lib/parameters/utils/click-behavior.ts b/frontend/src/metabase-lib/parameters/utils/click-behavior.ts
index 87099d8b072..31827b4307d 100644
--- a/frontend/src/metabase-lib/parameters/utils/click-behavior.ts
+++ b/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 };
diff --git a/frontend/src/metabase-lib/parameters/utils/mbql.js b/frontend/src/metabase-lib/parameters/utils/mbql.js
index 2f5bc9200e8..25b219e4228 100644
--- a/frontend/src/metabase-lib/parameters/utils/mbql.js
+++ b/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);
diff --git a/frontend/src/metabase-lib/parameters/utils/targets.ts b/frontend/src/metabase-lib/parameters/utils/targets.ts
index 1026b780e07..1b8be802e91 100644
--- a/frontend/src/metabase-lib/parameters/utils/targets.ts
+++ b/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) {
diff --git a/frontend/src/metabase/parameters/utils/mapping-options.js b/frontend/src/metabase/parameters/utils/mapping-options.js
index 48d6535ab27..8a58f72637b 100644
--- a/frontend/src/metabase/parameters/utils/mapping-options.js
+++ b/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,
};
});
diff --git a/frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts b/frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts
index 99a5a392e7e..53b43b51055 100644
--- a/frontend/src/metabase/visualizations/components/settings/ChartSettingTableColumns/utils.ts
+++ b/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 });
}
diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc
index ba442764be7..e7b22e2c5fc 100644
--- a/src/metabase/lib/convert.cljc
+++ b/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}
diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs
index acfba4e1d9d..ca0a2a1bb53 100644
--- a/src/metabase/lib/js.cljs
+++ b/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
diff --git a/test/metabase/lib/convert_test.cljc b/test/metabase/lib/convert_test.cljc
index 65dc32a2ddd..b37f0a7f2f8 100644
--- a/test/metabase/lib/convert_test.cljc
+++ b/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
diff --git a/test/metabase/lib/js_test.cljs b/test/metabase/lib/js_test.cljs
index 3b367eecdb4..e0f2c34abe9 100644
--- a/test/metabase/lib/js_test.cljs
+++ b/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)"
--
GitLab