From 460ac41f6f7a31c4f705c4bdfe53af8e8c65e164 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Tue, 13 Aug 2024 21:21:37 +0300
Subject: [PATCH] Use the metric's source instead of the metric itself as base
(#46688)
---
.../metrics/metrics-editing.cy.spec.js | 15 +-
.../metrics/metrics-question.cy.spec.js | 2 +-
.../metrics/metrics-search.cy.spec.js | 14 --
frontend/src/metabase-lib/metrics.ts | 4 -
frontend/src/metabase-lib/v1/Question.ts | 4 +
.../metabase-lib/v1/metadata/utils/models.ts | 24 +--
.../v1/metadata/utils/models.unit.spec.js | 29 ++--
.../AggregationPicker/AggregationPicker.tsx | 12 +-
.../query_builder/actions/core/core.js | 7 +-
.../query_builder/actions/navigation.js | 10 +-
.../query_builder/actions/querying.js | 9 +-
.../components/notebook/lib/steps.ts | 6 +-
.../steps/DataStep/DataStep.unit.spec.tsx | 1 -
.../SummarizeContent/use-summarize-query.ts | 2 +-
.../src/metabase/query_builder/selectors.js | 4 +-
.../TableInteractive/TableInteractive.jsx | 6 +-
src/metabase/lib/query.cljc | 10 +-
.../query_processor/middleware/metrics.clj | 31 ++--
test/metabase/lib/metric_test.cljc | 2 +-
test/metabase/lib/query_test.cljc | 51 +++++++
test/metabase/lib/remove_replace_test.cljc | 23 +--
.../middleware/metrics_test.clj | 138 +++++++++---------
22 files changed, 217 insertions(+), 187 deletions(-)
diff --git a/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js b/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js
index 487c9b7713a..166b79e467e 100644
--- a/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js
+++ b/e2e/test/scenarios/metrics/metrics-editing.cy.spec.js
@@ -297,7 +297,7 @@ describe("scenarios > metrics > editing", () => {
});
});
- it("should not be possible to join data on the first stage of a metric-based query", () => {
+ it("should be possible to join data on the first stage of a metric-based query", () => {
createQuestion(ORDERS_SCALAR_METRIC);
startNewQuestion();
entityPickerModal().within(() => {
@@ -306,7 +306,7 @@ describe("scenarios > metrics > editing", () => {
});
getNotebookStep("data").within(() => {
getActionButton("Custom column").should("be.visible");
- getActionButton("Join data").should("not.exist");
+ getActionButton("Join data").should("be.visible");
});
});
});
@@ -345,17 +345,6 @@ describe("scenarios > metrics > editing", () => {
saveMetric();
verifyScalarValue("111.38");
});
-
- it("should open the expression editor automatically when the source metric is already used in an aggregation expression", () => {
- createQuestion(ORDERS_SCALAR_METRIC);
- startNewMetric();
- entityPickerModal().within(() => {
- entityPickerModalTab("Metrics").click();
- cy.findByText(ORDERS_SCALAR_METRIC.name).click();
- });
- startNewAggregation();
- cy.findByTestId("expression-editor").should("be.visible");
- });
});
describe("breakouts", () => {
diff --git a/e2e/test/scenarios/metrics/metrics-question.cy.spec.js b/e2e/test/scenarios/metrics/metrics-question.cy.spec.js
index b1c37b8c9cc..a86e8deea81 100644
--- a/e2e/test/scenarios/metrics/metrics-question.cy.spec.js
+++ b/e2e/test/scenarios/metrics/metrics-question.cy.spec.js
@@ -162,7 +162,7 @@ describe("scenarios > metrics > question", () => {
cartesianChartCircle()
.eq(23) // random dot
.click({ force: true });
- popover().findByText("See these records").click();
+ popover().findByText("See these Orders").click();
cy.wait("@dataset");
cy.findByTestId("qb-filters-panel")
.findByText("Created At is Mar 1–31, 2024")
diff --git a/e2e/test/scenarios/metrics/metrics-search.cy.spec.js b/e2e/test/scenarios/metrics/metrics-search.cy.spec.js
index e1ade129124..3935b7a9337 100644
--- a/e2e/test/scenarios/metrics/metrics-search.cy.spec.js
+++ b/e2e/test/scenarios/metrics/metrics-search.cy.spec.js
@@ -76,18 +76,4 @@ describe("scenarios > metrics > search", () => {
cy.wait("@dataset");
cy.findByTestId("scalar-container").should("be.visible");
});
-
- it("should see metrics in recent items on the home page", () => {
- createQuestion(ORDERS_SCALAR_METRIC).then(({ body: card }) => {
- visitMetric(card.id);
- cy.wait("@dataset");
- });
- navigationSidebar().findByText("Home").click();
- cy.findByTestId("home-page").within(() => {
- cy.findByText("Pick up where you left off").should("be.visible");
- cy.findByText(ORDERS_SCALAR_METRIC.name).click();
- cy.wait("@dataset");
- });
- cy.findByTestId("scalar-container").should("be.visible");
- });
});
diff --git a/frontend/src/metabase-lib/metrics.ts b/frontend/src/metabase-lib/metrics.ts
index 9ba892f5dfe..ca232fbe551 100644
--- a/frontend/src/metabase-lib/metrics.ts
+++ b/frontend/src/metabase-lib/metrics.ts
@@ -8,7 +8,3 @@ export function availableMetrics(
): MetricMetadata[] {
return ML.available_metrics(query, stageIndex);
}
-
-export function isMetricBased(query: Query, stageIndex: number): boolean {
- return ML.metric_based_QMARK_(query, stageIndex);
-}
diff --git a/frontend/src/metabase-lib/v1/Question.ts b/frontend/src/metabase-lib/v1/Question.ts
index 76dddd5a338..874fbe12ad7 100644
--- a/frontend/src/metabase-lib/v1/Question.ts
+++ b/frontend/src/metabase-lib/v1/Question.ts
@@ -459,6 +459,10 @@ class Question {
const metadata = this.metadataProvider();
const tableId = getQuestionVirtualTableId(this.id());
const table = Lib.tableOrCardMetadata(metadata, tableId);
+ if (!table) {
+ return this;
+ }
+
const query = Lib.queryFromTableOrCardMetadata(metadata, table);
return this.setQuery(query);
}
diff --git a/frontend/src/metabase-lib/v1/metadata/utils/models.ts b/frontend/src/metabase-lib/v1/metadata/utils/models.ts
index 7d72b9ac012..2ff77730a32 100644
--- a/frontend/src/metabase-lib/v1/metadata/utils/models.ts
+++ b/frontend/src/metabase-lib/v1/metadata/utils/models.ts
@@ -1,7 +1,6 @@
import * as Lib from "metabase-lib";
import type Question from "metabase-lib/v1/Question";
import type Database from "metabase-lib/v1/metadata/Database";
-import { getQuestionVirtualTableId } from "metabase-lib/v1/metadata/utils/saved-questions";
import type NativeQuery from "metabase-lib/v1/queries/NativeQuery";
import { findColumnIndexesForColumnSettings } from "metabase-lib/v1/queries/utils/dataset";
import type {
@@ -101,22 +100,23 @@ export function checkCanBeModel(question: Question) {
.every(isSupportedTemplateTagForModel);
}
-export function isAdHocModelQuestion(
+export function isAdHocModelOrMetricQuestion(
question?: Question,
originalQuestion?: Question,
) {
- if (!question || !originalQuestion) {
- return false;
+ if (
+ question &&
+ originalQuestion &&
+ question.id() === originalQuestion.id() &&
+ (question.type() !== "question" || originalQuestion.type() !== "question")
+ ) {
+ return Lib.areLegacyQueriesEqual(
+ question.datasetQuery(),
+ originalQuestion.composeQuestion().datasetQuery(),
+ );
}
- const isModel =
- question.type() !== "question" || originalQuestion.type() !== "question";
- const isSameQuestion = question.id() === originalQuestion.id();
- const isSelfReferencing =
- Lib.sourceTableOrCardId(question.query()) ===
- getQuestionVirtualTableId(originalQuestion.id());
-
- return isModel && isSameQuestion && isSelfReferencing;
+ return false;
}
export function checkCanRefreshModelCache(
diff --git a/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js b/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js
index fd0ceafcaa1..b7c60d23df5 100644
--- a/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js
+++ b/frontend/src/metabase-lib/v1/metadata/utils/models.unit.spec.js
@@ -6,7 +6,7 @@ import {
checkCanBeModel,
checkCanRefreshModelCache,
getModelCacheSchemaName,
- isAdHocModelQuestion,
+ isAdHocModelOrMetricQuestion,
getDatasetMetadataCompletenessPercentage,
} from "metabase-lib/v1/metadata/utils/models";
import {
@@ -200,7 +200,7 @@ describe("data model utils", () => {
const { metadata } = setup({ cards: [modelCard] });
const question = new Question(composedModelCard, metadata);
- expect(isAdHocModelQuestion(question)).toBe(false);
+ expect(isAdHocModelOrMetricQuestion(question)).toBe(false);
});
it("returns false for native questions", () => {
@@ -209,21 +209,18 @@ describe("data model utils", () => {
const question = metadata.question(card.id);
- expect(isAdHocModelQuestion(question, question)).toBe(false);
+ expect(isAdHocModelOrMetricQuestion(question, question)).toBe(false);
});
it("identifies when model goes into ad-hoc exploration mode", () => {
const modelCard = createStructuredModelCard({ id: 1 });
- const composedModelCard = createSavedStructuredCard({
- id: 1,
- sourceTable: "card__1",
- });
const { metadata } = setup({ cards: [modelCard] });
-
const originalQuestion = metadata.question(modelCard.id);
- const question = new Question(composedModelCard, metadata);
+ const question = originalQuestion.composeQuestion();
- expect(isAdHocModelQuestion(question, originalQuestion)).toBe(true);
+ expect(isAdHocModelOrMetricQuestion(question, originalQuestion)).toBe(
+ true,
+ );
});
it("returns false when IDs don't match", () => {
@@ -237,7 +234,9 @@ describe("data model utils", () => {
const originalQuestion = metadata.question(modelCard.id);
const question = new Question(composedModelCard, metadata);
- expect(isAdHocModelQuestion(question, originalQuestion)).toBe(false);
+ expect(isAdHocModelOrMetricQuestion(question, originalQuestion)).toBe(
+ false,
+ );
});
it("returns false when questions are not models", () => {
@@ -251,7 +250,9 @@ describe("data model utils", () => {
const originalQuestion = metadata.question(modelCard.id);
const question = new Question(composedModelCard, metadata);
- expect(isAdHocModelQuestion(question, originalQuestion)).toBe(false);
+ expect(isAdHocModelOrMetricQuestion(question, originalQuestion)).toBe(
+ false,
+ );
});
it("returns false when potential ad-hoc model question is not self-referencing", () => {
@@ -265,7 +266,9 @@ describe("data model utils", () => {
const originalQuestion = metadata.question(modelCard.id);
const question = new Question(composedModelCard, metadata);
- expect(isAdHocModelQuestion(question, originalQuestion)).toBe(false);
+ expect(isAdHocModelOrMetricQuestion(question, originalQuestion)).toBe(
+ false,
+ );
});
});
diff --git a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx
index 3392269f32f..4f9f7452e7e 100644
--- a/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx
+++ b/frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx
@@ -108,9 +108,8 @@ export function AggregationPicker({
const databaseId = Lib.databaseID(query);
const database = metadata.database(databaseId);
const canUseExpressions = database?.hasFeature("expression-aggregations");
- const isMetricBased = Lib.isMetricBased(query, stageIndex);
- if (operators.length > 0 && !isMetricBased) {
+ if (operators.length > 0) {
const operatorItems = operators.map(operator =>
getOperatorListItem(query, stageIndex, operator),
);
@@ -126,7 +125,7 @@ export function AggregationPicker({
if (metrics.length > 0) {
sections.push({
key: "metrics",
- name: isMetricBased ? t`Metrics` : t`Common Metrics`,
+ name: t`Common Metrics`,
items: metrics.map(metric =>
getMetricListItem(query, stageIndex, metric, clauseIndex),
),
@@ -371,12 +370,7 @@ function isExpressionEditorInitiallyOpen(
operators: Lib.AggregationOperator[],
): boolean {
if (!clause) {
- return (
- Lib.isMetricBased(query, stageIndex) &&
- Lib.availableMetrics(query, stageIndex)
- .map(metric => Lib.displayInfo(query, stageIndex, metric))
- .every(metricInfo => metricInfo.aggregationPosition != null)
- );
+ return false;
}
const initialOperator = getInitialOperator(query, stageIndex, operators);
diff --git a/frontend/src/metabase/query_builder/actions/core/core.js b/frontend/src/metabase/query_builder/actions/core/core.js
index e0d7e9920cf..8294ca46eca 100644
--- a/frontend/src/metabase/query_builder/actions/core/core.js
+++ b/frontend/src/metabase/query_builder/actions/core/core.js
@@ -18,7 +18,7 @@ import { getMetadata } from "metabase/selectors/metadata";
import { getCardAfterVisualizationClick } from "metabase/visualizations/lib/utils";
import * as Lib from "metabase-lib";
import Question from "metabase-lib/v1/Question";
-import { isAdHocModelQuestion } from "metabase-lib/v1/metadata/utils/models";
+import { isAdHocModelOrMetricQuestion } from "metabase-lib/v1/metadata/utils/models";
import Query from "metabase-lib/v1/queries/Query";
import {
cardIsEquivalent,
@@ -257,7 +257,10 @@ export const apiUpdateQuestion = (question, { rerunQuery } = {}) => {
submittableQuestion,
dispatch,
{
- excludeDatasetQuery: isAdHocModelQuestion(question, originalQuestion),
+ excludeDatasetQuery: isAdHocModelOrMetricQuestion(
+ question,
+ originalQuestion,
+ ),
},
);
diff --git a/frontend/src/metabase/query_builder/actions/navigation.js b/frontend/src/metabase/query_builder/actions/navigation.js
index 340d2b09728..6b42db3186d 100644
--- a/frontend/src/metabase/query_builder/actions/navigation.js
+++ b/frontend/src/metabase/query_builder/actions/navigation.js
@@ -7,7 +7,7 @@ import { createThunkAction } from "metabase/lib/redux";
import { equals } from "metabase/lib/utils";
import { getLocation } from "metabase/selectors/routing";
import * as Lib from "metabase-lib";
-import { isAdHocModelQuestion } from "metabase-lib/v1/metadata/utils/models";
+import { isAdHocModelOrMetricQuestion } from "metabase-lib/v1/metadata/utils/models";
import {
getCard,
@@ -142,10 +142,14 @@ export const updateUrl = createThunkAction(
if (dirty == null) {
const originalQuestion = getOriginalQuestion(getState());
- const isAdHocModel = isAdHocModelQuestion(question, originalQuestion);
+ const isAdHocModelOrMetric = isAdHocModelOrMetricQuestion(
+ question,
+ originalQuestion,
+ );
dirty =
!originalQuestion ||
- (!isAdHocModel && question.isDirtyComparedTo(originalQuestion));
+ (!isAdHocModelOrMetric &&
+ question.isDirtyComparedTo(originalQuestion));
}
const { isNative } = Lib.queryDisplayInfo(question.query());
diff --git a/frontend/src/metabase/query_builder/actions/querying.js b/frontend/src/metabase/query_builder/actions/querying.js
index 900467352d7..07b84396774 100644
--- a/frontend/src/metabase/query_builder/actions/querying.js
+++ b/frontend/src/metabase/query_builder/actions/querying.js
@@ -10,7 +10,7 @@ import { getWhiteLabeledLoadingMessageFactory } from "metabase/selectors/whitela
import { runQuestionQuery as apiRunQuestionQuery } from "metabase/services";
import { getSensibleDisplays } from "metabase/visualizations";
import * as Lib from "metabase-lib";
-import { isAdHocModelQuestion } from "metabase-lib/v1/metadata/utils/models";
+import { isAdHocModelOrMetricQuestion } from "metabase-lib/v1/metadata/utils/models";
import {
getCard,
@@ -114,9 +114,10 @@ export const runQuestionQuery = ({
: true;
if (shouldUpdateUrl) {
- const isAdHocModelOrMetric =
- (question.type() === "model" || question.type() === "metric") &&
- isAdHocModelQuestion(question, originalQuestion);
+ const isAdHocModelOrMetric = isAdHocModelOrMetricQuestion(
+ question,
+ originalQuestion,
+ );
dispatch(
updateUrl(question, { dirty: !isAdHocModelOrMetric && isCardDirty }),
diff --git a/frontend/src/metabase/query_builder/components/notebook/lib/steps.ts b/frontend/src/metabase/query_builder/components/notebook/lib/steps.ts
index ccbc2dba3d2..967984bf92b 100644
--- a/frontend/src/metabase/query_builder/components/notebook/lib/steps.ts
+++ b/frontend/src/metabase/query_builder/components/notebook/lib/steps.ts
@@ -33,11 +33,7 @@ const STEPS: NotebookStepDef[] = [
clauseType: "joins",
valid: (query, stageIndex, metadata) => {
const database = metadata.database(Lib.databaseID(query));
- return (
- hasData(query) &&
- Boolean(database?.hasFeature("join")) &&
- !Lib.isMetricBased(query, stageIndex)
- );
+ return hasData(query) && Boolean(database?.hasFeature("join"));
},
subSteps: (query, stageIndex) => {
return Lib.joins(query, stageIndex).length;
diff --git a/frontend/src/metabase/query_builder/components/notebook/steps/DataStep/DataStep.unit.spec.tsx b/frontend/src/metabase/query_builder/components/notebook/steps/DataStep/DataStep.unit.spec.tsx
index c602714fbf7..29a58d5be38 100644
--- a/frontend/src/metabase/query_builder/components/notebook/steps/DataStep/DataStep.unit.spec.tsx
+++ b/frontend/src/metabase/query_builder/components/notebook/steps/DataStep/DataStep.unit.spec.tsx
@@ -164,7 +164,6 @@ describe("DataStep", () => {
it.each<{ type: CardType; icon: IconName }>([
{ type: "question", icon: "table2" },
{ type: "model", icon: "model" },
- { type: "metric", icon: "metric" },
])("should render with a selected card", ({ type, icon }) => {
const card = createSavedStructuredCard({
id: 1,
diff --git a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeContent/use-summarize-query.ts b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeContent/use-summarize-query.ts
index b68c5d75ca7..6b04d5d9c6c 100644
--- a/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeContent/use-summarize-query.ts
+++ b/frontend/src/metabase/query_builder/components/view/sidebars/SummarizeSidebar/SummarizeContent/use-summarize-query.ts
@@ -119,5 +119,5 @@ export const useSummarizeQuery = ({
function shouldAddDefaultAggregation(query: Lib.Query): boolean {
const aggregations = Lib.aggregations(query, STAGE_INDEX);
- return aggregations.length === 0 && !Lib.isMetricBased(query, STAGE_INDEX);
+ return aggregations.length === 0;
}
diff --git a/frontend/src/metabase/query_builder/selectors.js b/frontend/src/metabase/query_builder/selectors.js
index 90d61f2cb05..8a6f5e06233 100644
--- a/frontend/src/metabase/query_builder/selectors.js
+++ b/frontend/src/metabase/query_builder/selectors.js
@@ -37,7 +37,7 @@ import {
isTimeseries,
} from "metabase/visualizations/lib/renderer_utils";
import { isAbsoluteDateTimeUnit } from "metabase-types/guards/date-time";
-import { isAdHocModelQuestion } from "metabase-lib/v1/metadata/utils/models";
+import { isAdHocModelOrMetricQuestion } from "metabase-lib/v1/metadata/utils/models";
import { getCardUiParameters } from "metabase-lib/v1/parameters/utils/cards";
import {
normalizeParameters,
@@ -602,7 +602,7 @@ export const getIsDirty = createSelector(
// We need to escape the isDirty check as it will always be true in this case,
// and the page will always be covered with a 'rerun' overlay.
// Once the dataset_query changes, the question will loose the "dataset" flag and it'll work normally
- if (!question || isAdHocModelQuestion(question, originalQuestion)) {
+ if (!question || isAdHocModelOrMetricQuestion(question, originalQuestion)) {
return false;
}
return question.isDirtyComparedToWithoutParameters(originalQuestion);
diff --git a/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx b/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx
index cefbd602d53..34364718fbf 100644
--- a/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx
+++ b/frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.jsx
@@ -44,7 +44,7 @@ import {
} from "metabase/visualizations/lib/table";
import { getColumnExtent } from "metabase/visualizations/lib/utils";
import * as Lib from "metabase-lib";
-import { isAdHocModelQuestion } from "metabase-lib/v1/metadata/utils/models";
+import { isAdHocModelOrMetricQuestion } from "metabase-lib/v1/metadata/utils/models";
import { isID, isPK, isFK } from "metabase-lib/v1/types/utils/isa";
import { memoizeClass } from "metabase-lib/v1/utils";
@@ -214,8 +214,8 @@ class TableInteractive extends Component {
const isDataChange =
data && nextData && !_.isEqual(data.cols, nextData.cols);
const isDatasetStatusChange =
- isAdHocModelQuestion(nextQuestion, question) ||
- isAdHocModelQuestion(question, nextQuestion);
+ isAdHocModelOrMetricQuestion(nextQuestion, question) ||
+ isAdHocModelOrMetricQuestion(question, nextQuestion);
if (isDataChange && !isDatasetStatusChange) {
this.resetColumnWidths();
diff --git a/src/metabase/lib/query.cljc b/src/metabase/lib/query.cljc
index 364e9e19c82..295a9e029b0 100644
--- a/src/metabase/lib/query.cljc
+++ b/src/metabase/lib/query.cljc
@@ -235,16 +235,14 @@
(defn- metric-query
[metadata-providerable card-metadata]
(let [card-id (u/the-id card-metadata)
+ metric-first-stage (-> (query metadata-providerable (:dataset-query card-metadata))
+ (lib.util/query-stage 0))
base-query (query-with-stages metadata-providerable
- [{:lib/type :mbql.stage/mbql
- :source-card card-id}])
- metric-breakouts (-> (query metadata-providerable (:dataset-query card-metadata))
- (lib.util/query-stage -1)
- :breakout)
+ [(select-keys metric-first-stage [:lib/type :source-card :source-table])])
base-query (reduce
#(lib.util/add-summary-clause %1 0 :breakout %2)
base-query
- metric-breakouts)]
+ (:breakout metric-first-stage))]
(-> base-query
(lib.util/add-summary-clause
0 :aggregation
diff --git a/src/metabase/query_processor/middleware/metrics.clj b/src/metabase/query_processor/middleware/metrics.clj
index 831e56a9fa8..d84a9cde8ee 100644
--- a/src/metabase/query_processor/middleware/metrics.clj
+++ b/src/metabase/query_processor/middleware/metrics.clj
@@ -93,24 +93,33 @@
(defn- aggregation-stage-index
[stages]
- (count (take-while :qp/stage-is-from-source-card stages)))
+ (count (take-while (complement (comp find-metric-ids :aggregation)) stages)))
+
+(defn- add-join-aliases
+ [x source-field->join-alias]
+ (lib.util.match/replace x
+ [:field (opts :guard (every-pred (comp source-field->join-alias :source-field) (complement :join-alias))) _]
+ (assoc-in &match [1 :join-alias] (-> opts :source-field source-field->join-alias))))
(defn- include-implicit-joins
[query agg-stage-index metric-query]
(let [metric-joins (lib/joins metric-query -1)
existing-joins (into #{}
(map (juxt :fk-field-id :alias))
- (lib/joins query agg-stage-index))]
- (reduce #(lib/join %1 agg-stage-index %2)
- query
- (remove (comp existing-joins (juxt :fk-field-id :alias)) metric-joins))))
+ (lib/joins query agg-stage-index))
+ new-joins (remove (comp existing-joins (juxt :fk-field-id :alias)) metric-joins)
+ source-field->join-alias (into {} (map (juxt :fk-field-id :alias)) new-joins)
+ query-with-joins (reduce #(lib/join %1 agg-stage-index %2)
+ query
+ new-joins)]
+ (lib.util/update-query-stage query-with-joins agg-stage-index add-join-aliases source-field->join-alias)))
(defn splice-compatible-metrics
"Splices in metric definitions that are compatible with the query."
[query path expanded-stages]
- (let [agg-stage-index (aggregation-stage-index (:stages query))]
+ (let [agg-stage-index (aggregation-stage-index expanded-stages)]
(if-let [lookup (->> expanded-stages
- (drop-while :qp/stage-is-from-source-card)
+ (drop agg-stage-index)
first
:aggregation
(fetch-referenced-metrics query))]
@@ -237,10 +246,10 @@
A query built from a `:source-card` of `:type :metric` can reference itself."
[query]
(let [query (lib.walk/walk
- query
- (fn [_query path-type path stage-or-join]
- (when (= path-type :lib.walk/join)
- (update stage-or-join :stages #(adjust-metric-stages query path %)))))]
+ query
+ (fn [_query path-type path stage-or-join]
+ (when (= path-type :lib.walk/join)
+ (update stage-or-join :stages #(adjust-metric-stages query path %)))))]
(u/prog1
(update query :stages #(adjust-metric-stages query nil %))
(when-let [metric (lib.util.match/match-one <>
diff --git a/test/metabase/lib/metric_test.cljc b/test/metabase/lib/metric_test.cljc
index 5081d8b4ac7..4d141a164cd 100644
--- a/test/metabase/lib/metric_test.cljc
+++ b/test/metabase/lib/metric_test.cljc
@@ -171,7 +171,7 @@
:database (meta/id)
:stages
[{:lib/type :mbql.stage/mbql
- :source-card 1
+ :source-table (meta/id :orders)
:breakout
[[:field
{:base-type :type/DateTimeWithLocalTZ, :temporal-unit :month}
diff --git a/test/metabase/lib/query_test.cljc b/test/metabase/lib/query_test.cljc
index 5d6e17292f5..82b5322f7b3 100644
--- a/test/metabase/lib/query_test.cljc
+++ b/test/metabase/lib/query_test.cljc
@@ -568,3 +568,54 @@
(is (= exp-fields
(into #{} (map :id) (lib/visible-columns query2))))
(is (= 1 @calls))))))))))
+
+(deftest ^:parallel metric-based-question-test
+ (let [question-id 100
+ model-id 101
+ table-based-metric-id 102
+ model-based-metric-id 103
+ base-card {:name "Sum of Cans"
+ :database-id (meta/id)
+ :table-id (meta/id :venues)
+ :dataset-query
+ (-> lib.tu/venues-query
+ (lib/filter (lib/= (meta/field-metadata :venues :price) 4))
+ (lib/aggregate (lib/sum (meta/field-metadata :venues :price)))
+ (lib/breakout (meta/field-metadata :venues :category-id))
+ (lib/breakout (meta/field-metadata :venues :latitude))
+ (lib/breakout (meta/field-metadata :venues :longitude))
+ lib.convert/->legacy-MBQL)}
+ base-mp (lib.tu/mock-metadata-provider
+ meta/metadata-provider
+ {:cards [(assoc base-card :id question-id :type :question)
+ (assoc base-card :id model-id :type :model)
+ (assoc base-card :id table-based-metric-id :type :metric)]})
+ mp (lib.tu/mock-metadata-provider
+ base-mp
+ {:cards [{:id model-based-metric-id
+ :name "Model based metric"
+ :database-id (meta/id)
+ :table-id (meta/id :venues)
+ :source-card-id model-id
+ :dataset-query
+ (-> (lib/query base-mp (lib.metadata/card base-mp model-id))
+ (lib/aggregate (lib/count))
+ lib.convert/->legacy-MBQL)
+ :type :metric}]})]
+ (is (=? {:lib/type :mbql/query
+ :database (meta/id)
+ :stages
+ [{:lib/type :mbql.stage/mbql
+ :source-table (meta/id :venues)
+ :aggregation [[:metric {} table-based-metric-id]]
+ :breakout [[:field {} (meta/id :venues :category-id)]
+ [:field {} (meta/id :venues :latitude)]
+ [:field {} (meta/id :venues :longitude)]]}]}
+ (lib/query base-mp (lib.metadata/card base-mp table-based-metric-id))))
+ (is (=? {:lib/type :mbql/query
+ :database (meta/id)
+ :stages
+ [{:lib/type :mbql.stage/mbql
+ :source-card model-id
+ :aggregation [[:metric {} model-based-metric-id]]}]}
+ (lib/query base-mp (lib.metadata/card mp model-based-metric-id))))))
diff --git a/test/metabase/lib/remove_replace_test.cljc b/test/metabase/lib/remove_replace_test.cljc
index 9dd7ea2668a..eb279fce8f6 100644
--- a/test/metabase/lib/remove_replace_test.cljc
+++ b/test/metabase/lib/remove_replace_test.cljc
@@ -483,6 +483,7 @@
{:cards [{:id 100
:name "Sum of Cans"
:database-id (meta/id)
+ :table-id (meta/id :venues)
:dataset-query
(-> lib.tu/venues-query
(lib/filter (lib/= (meta/field-metadata :venues :price) 4))
@@ -496,18 +497,18 @@
[:count {:lib/uuid string?}]]}]}
query))
(is (=? {:stages [{:aggregation [[:metric {:lib/uuid string?} 100]
- [:metric {:lib/uuid string?} 100]]}]}
- (lib/replace-clause
- query
- (second (lib/aggregations query))
- (first (lib/available-metrics query)))))
+ [:metric {:lib/uuid string?} 100]]}]}
+ (lib/replace-clause
+ query
+ (second (lib/aggregations query))
+ (first (lib/available-metrics query)))))
(is (=? {:stages [{:aggregation [[:count {:lib/uuid string?}]
- [:metric {:lib/uuid string?} 100]]}]}
- (-> query
- (lib/replace-clause
- (second (lib/aggregations query))
- (first (lib/available-metrics query)))
- (as-> $q (lib/replace-clause $q (first (lib/aggregations $q)) (lib/count)))))))))
+ [:metric {:lib/uuid string?} 100]]}]}
+ (-> query
+ (lib/replace-clause
+ (second (lib/aggregations query))
+ (first (lib/available-metrics query)))
+ (as-> $q (lib/replace-clause $q (first (lib/aggregations $q)) (lib/count)))))))))
(deftest ^:parallel replace-segment-test
(testing "replacing with segment should work"
diff --git a/test/metabase/query_processor/middleware/metrics_test.clj b/test/metabase/query_processor/middleware/metrics_test.clj
index 5e3d3b7e275..e1b256606a6 100644
--- a/test/metabase/query_processor/middleware/metrics_test.clj
+++ b/test/metabase/query_processor/middleware/metrics_test.clj
@@ -75,10 +75,9 @@
(deftest ^:parallel adjust-basic-source-metric-test
(let [[source-metric mp] (mock-metric)
query (lib/query mp source-metric)]
- (is (=?
- {:stages [{:source-table (meta/id :products)}
- {:aggregation [[:avg {} [:field {} (comp #{"rating"} u/lower-case-en)]]]}]}
- (adjust query)))))
+ (is (=? {:stages [{:source-table (meta/id :products)
+ :aggregation [[:avg {} [:field {} (meta/id :products :rating)]]]}]}
+ (adjust query)))))
(deftest ^:parallel adjust-aggregation-metric-ref-test
(let [[source-metric mp] (mock-metric)
@@ -169,30 +168,29 @@
query (-> (lib/query mp source-metric)
(lib/join (-> (lib/join-clause (meta/table-metadata :orders)
[(lib/=
- (meta/field-metadata :products :id)
- (meta/field-metadata :orders :product-id))])
+ (meta/field-metadata :products :id)
+ (meta/field-metadata :orders :product-id))])
(lib/with-join-fields :all))))]
- (is (=?
- {:stages [{:source-table (meta/id :products)}
- {:aggregation [[:avg {} [:field {} (comp #{"rating"} u/lower-case-en)]]]
- :joins [{:stages
- [{:source-table (meta/id :orders)}],
- :conditions
- [[:= {}
- [:field {} (meta/id :products :id)]
- [:field {:join-alias "Orders"} (meta/id :orders :product-id)]]],
- :alias "Orders"}]}]}
- (adjust query)))))
+ (is (=? {:stages [{:source-table (meta/id :products)
+ :aggregation [[:avg {} [:field {} (meta/id :products :rating)]]]
+ :joins [{:stages
+ [{:source-table (meta/id :orders)}],
+ :conditions
+ [[:= {}
+ [:field {} (meta/id :products :id)]
+ [:field {:join-alias "Orders"} (meta/id :orders :product-id)]]],
+ :alias "Orders"}]}]}
+ (adjust query)))))
(deftest ^:parallel adjust-expression-test
(let [[source-metric mp] (mock-metric (lib/expression (basic-metric-query) "source" (lib/+ 1 1)))
query (-> (lib/query mp source-metric)
(lib/expression "target" (lib/- 2 2)))]
(is (=?
- {:stages [{:expressions [[:+ {:lib/expression-name "source"} 1 1]]}
- {:expressions [[:- {:lib/expression-name "target"} 2 2]]
- :aggregation [[:avg {} [:field {} (comp #{"rating"} u/lower-case-en)]]]}]}
- (adjust query)))))
+ {:stages [{:expressions [[:- {:lib/expression-name "target"} 2 2]
+ [:+ {:lib/expression-name "source"} 1 1]]
+ :aggregation [[:avg {} [:field {} (meta/id :products :rating)]]]}]}
+ (adjust query)))))
(deftest ^:parallel adjust-expression-name-collision-test
(let [[source-metric mp] (mock-metric (-> (basic-metric-query)
@@ -221,10 +219,10 @@
query (-> (lib/query mp source-metric)
(lib/filter (lib/= (meta/field-metadata :products :category) "Widget")))]
(is (=?
- {:stages [{:source-table (meta/id :products)
- :filters [[:> {} [:field {} (meta/id :products :price)] 1]]}
- {:filters [[:= {} [:field {} (meta/id :products :category)] "Widget"]]}]}
- (adjust query)))))
+ {:stages [{:source-table (meta/id :products)
+ :filters [[:= {} [:field {} (meta/id :products :category)] "Widget"]
+ [:> {} [:field {} (meta/id :products :price)] [:value {} 1]]]}]}
+ (adjust query)))))
(deftest ^:parallel adjust-mixed-multi-source-test
(let [[first-metric mp] (mock-metric lib.tu/metadata-provider-with-mock-cards
@@ -236,11 +234,9 @@
query (-> (lib/query mp second-metric)
(lib/filter (lib/= (meta/field-metadata :products :category) "Widget")))]
(is (=? {:stages [{:source-table (meta/id :products)}
- {:filters [[:> {} [:field {} (meta/id :products :price)] 1]]
- :aggregation complement}
- {:filters [[:< {} [:field {} (meta/id :products :price)] 100]]
- :aggregation complement}
- {:filters [[:= {} [:field {} (meta/id :products :category)] "Widget"]]
+ {:filters [[:= {} [:field {} (meta/id :products :category)] "Widget"]
+ [:< {} [:field {} (meta/id :products :price)] [:value {} 100]]
+ [:> {} [:field {} (meta/id :products :price)] [:value {} 1]]]
:aggregation some?}]}
(adjust query)))))
@@ -249,10 +245,7 @@
[second-metric mp] (mock-metric mp (lib/query mp first-metric))
[third-metric mp] (mock-metric mp (lib/query mp second-metric))
query (lib/query mp third-metric)]
- (is (=? {:stages [{:aggregation complement}
- {:aggregation complement}
- {:aggregation complement}
- {:aggregation [[:avg {} [:field {} (comp #{"rating"} u/lower-case-en)]]]}]}
+ (is (=? {:stages [{:aggregation [[:avg {} [:field {} (meta/id :products :rating)]]]}]}
(adjust query)))))
(deftest ^:parallel joined-question-based-on-metric-based-on-metric-based-on-metric-test
@@ -261,9 +254,7 @@
[question mp] (mock-metric mp (lib/query mp second-metric) {:type :question})
query (-> (lib/query mp (meta/table-metadata :products))
(lib/join (lib/join-clause question [(lib/= 1 1)])))]
- (is (=? {:stages [{:joins [{:stages [{:aggregation complement}
- {:aggregation complement}
- {:aggregation [[:avg {} [:field {} (comp #{"rating"} u/lower-case-en)]]]}
+ (is (=? {:stages [{:joins [{:stages [{:aggregation [[:avg {} [:field {} (meta/id :products :rating)]]]}
;; Empty stage added by resolved-source-cards to nest join
#(= #{:lib/type :qp/stage-had-source-card :source-query/model?} (set (keys %)))]}]}]}
(adjust query)))))
@@ -385,8 +376,7 @@
(let [[source-metric mp] (mock-metric)
query (-> (lib/query mp source-metric)
(as-> $q (lib/order-by $q (lib/aggregation-ref $q 0))))]
- (is (=? {:stages [{}
- {:aggregation [[:avg {:lib/uuid (=?/same :uuid)} some?]]
+ (is (=? {:stages [{:aggregation [[:avg {:lib/uuid (=?/same :uuid)} some?]]
:order-by [[:asc {} [:aggregation {} (=?/same :uuid)]]]}]}
(adjust query))))))
@@ -439,26 +429,6 @@
(mt/rows
(qp/process-query query))))))))
-(deftest ^:parallel execute-multi-stage-metric
- (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
- stage-one (-> (lib/query mp (lib.metadata/table mp (mt/id :orders)))
- (lib/breakout (lib/with-temporal-bucket
- (lib.metadata/field mp (mt/id :orders :created_at))
- :month))
- (lib/aggregate (lib/count))
- (lib/append-stage))
- stage-one-cols (lib/visible-columns stage-one)
- source-query (-> stage-one
- (lib/breakout (m/find-first (comp #{"Created At: Month"} :display-name) stage-one-cols))
- (lib/aggregate (lib/avg (m/find-first (comp #{"Count"} :display-name) stage-one-cols))))]
- (mt/with-temp [:model/Card source-metric {:dataset_query (lib.convert/->legacy-MBQL source-query)
- :database_id (mt/id)
- :name "new_metric"
- :type :metric}]
- (let [query (lib/query mp (lib.metadata/card mp (:id source-metric)))]
- (is (=? (mt/rows (qp/process-query source-query))
- (mt/rows (qp/process-query query))))))))
-
(deftest ^:parallel execute-single-stage-metric
(let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
source-query (-> (lib/query mp (lib.metadata/table mp (mt/id :products)))
@@ -539,24 +509,26 @@
(deftest ^:parallel metric-with-nested-segments-test
(let [mp (lib.tu/mock-metadata-provider
- meta/metadata-provider
- {:segments [{:id 1
- :name "Segment 1"
- :table-id (meta/id :venues)
- :definition {:filter [:= [:field (meta/id :venues :name) nil] "abc"]}}]})
+ meta/metadata-provider
+ {:segments [{:id 1
+ :name "Segment 1"
+ :table-id (meta/id :venues)
+ :definition {:filter [:= [:field (meta/id :venues :name) nil] "abc"]}}]})
[source-metric mp] (mock-metric mp (-> (basic-metric-query)
(lib/filter (lib.metadata/segment mp 1))))]
;; Segments are handled further in the pipeline when the source is a metric
(is (=?
- {:stages [{:filters [[:segment {} 1]]}
- {}]}
- (adjust (lib/query mp source-metric))))
+ {:stages
+ [{:source-table (meta/id :products)
+ :aggregation [[:avg {:name "Mock Metric"} [:field {} (meta/id :products :rating)]]]
+ :filters [[:= {} [:field {} (meta/id :venues :name)] some?]]}]}
+ (adjust (lib/query mp source-metric))))
;; Segments will be expanded in this case as the metric query that is spliced in needs to be processed
(is (=?
- {:stages [{:filters [[:= {} [:field {} (meta/id :venues :name)] some?]]}]}
- (adjust
- (-> (lib/query mp (meta/table-metadata :products))
- (lib/aggregate (lib.metadata/metric mp (:id source-metric)))))))))
+ {:stages [{:filters [[:= {} [:field {} (meta/id :venues :name)] some?]]}]}
+ (adjust
+ (-> (lib/query mp (meta/table-metadata :products))
+ (lib/aggregate (lib.metadata/metric mp (:id source-metric)))))))))
(deftest ^:parallel expand-macros-in-nested-queries-test
(testing "expand-macros should expand things in the correct nested level (#12507)"
@@ -656,3 +628,27 @@
:filters [[:< {} [:field {} "RATING"] [:value {} 5]]
[:> {} [:field {} "RATING"] [:value {} 3]]]}]}
(adjust query))))))
+
+(deftest ^:parallel model-based-metric-with-implicit-join-test
+ (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+ model-query (lib/query mp (lib.metadata/table mp (mt/id :orders)))]
+ (mt/with-temp [:model/Card model {:dataset_query (lib.convert/->legacy-MBQL model-query)
+ :database_id (mt/id)
+ :name "Orders model"
+ :type :model}
+ :model/Card metric {:dataset_query
+ (as-> (lib/query mp (lib.metadata/card mp (:id model))) $q
+ (lib/breakout $q (m/find-first (comp #{"Category"} :display-name)
+ (lib/breakoutable-columns $q)))
+ (lib/aggregate $q (lib/count))
+ (lib.convert/->legacy-MBQL $q))
+ :database_id (mt/id)
+ :name "Orders model metric"
+ :type :metric}]
+ (let [metric-query (lib/query mp (lib.metadata/card mp (:id metric)))
+ etalon-query (as-> (lib/query mp (lib.metadata/card mp (:id model))) $q
+ (lib/breakout $q (m/find-first (comp #{"Category"} :display-name)
+ (lib/breakoutable-columns $q)))
+ (lib/aggregate $q (lib/count)))]
+ (is (=? (mt/rows (qp/process-query etalon-query))
+ (mt/rows (qp/process-query metric-query))))))))
--
GitLab