Merge branch 'master' into agg-ref-debug

e2e/test/scenarios/custom-column/custom-column.cy.spec.js

@@ -1799,3 +1799,166 @@ describe("scenarios > question > custom column > splitPart", () => {
     H.popover().should("contain", "Expected positive integer but found 0");
   });
 });
+
+describe("scenarios > question > custom column > aggregation", () => {
+  beforeEach(() => {
+    H.restore();
+    cy.signInAsNormalUser();
+    H.openOrdersTable({ mode: "notebook" });
+  });
+
+  it("should be possible to resolve aggregations from the question", () => {
+    H.createQuestion(
+      {
+        query: {
+          "source-table": ORDERS_ID,
+          aggregation: [
+            [
+              "aggregation-options",
+              ["sum", ["field", ORDERS.TOTAL, null]],
+              {
+                name: "Custom Sum",
+                "display-name": "Custom Sum",
+              },
+            ],
+          ],
+        },
+      },
+      { visitQuestion: true },
+    );
+    H.openNotebook();
+
+    H.getNotebookStep("summarize").icon("add").click();
+    H.popover().findByText("Custom Expression").scrollIntoView().click();
+
+    H.CustomExpressionEditor.type("[Custom");
+    H.CustomExpressionEditor.completion("Custom Sum")
+      .should("be.visible")
+      .click();
+    H.CustomExpressionEditor.value().should("eq", "[Custom Sum]");
+    H.CustomExpressionEditor.type("+ 1");
+    H.CustomExpressionEditor.format();
+
+    H.CustomExpressionEditor.nameInput().type("Derived");
+    H.popover().button("Done").click();
+
+    H.visualize();
+    H.assertTableData({
+      columns: ["Custom Sum", "Derived"],
+    });
+  });
+
+  it("should be possible to resolve aggregations from the question directly", () => {
+    H.createQuestion(
+      {
+        query: {
+          "source-table": ORDERS_ID,
+          aggregation: [
+            [
+              "aggregation-options",
+              ["sum", ["field", ORDERS.TOTAL, null]],
+              {
+                name: "Custom Sum",
+                "display-name": "Custom Sum",
+              },
+            ],
+          ],
+        },
+      },
+      { visitQuestion: true },
+    );
+    H.openNotebook();
+
+    H.getNotebookStep("summarize").icon("add").click();
+    H.popover().findByText("Custom Expression").scrollIntoView().click();
+
+    H.CustomExpressionEditor.type("[Custom");
+    H.CustomExpressionEditor.completion("Custom Sum")
+      .should("be.visible")
+      .click();
+    H.CustomExpressionEditor.value().should("eq", "[Custom Sum]");
+    H.CustomExpressionEditor.format();
+
+    H.CustomExpressionEditor.nameInput().type("Derived");
+    H.popover().button("Done").click();
+
+    H.visualize();
+    H.assertTableData({
+      columns: ["Custom Sum", "Derived"],
+    });
+  });
+
+  it("should be possible to resolve aggregations from the previous stage", () => {
+    H.createQuestion(
+      {
+        query: {
+          "source-table": ORDERS_ID,
+          aggregation: [
+            [
+              "aggregation-options",
+              ["sum", ["field", ORDERS.TOTAL, null]],
+              {
+                name: "Custom Sum",
+                "display-name": "Custom Sum",
+              },
+            ],
+          ],
+        },
+      },
+      { visitQuestion: true },
+    );
+    H.openNotebook();
+
+    cy.findAllByLabelText("Custom column").eq(1).click();
+    H.CustomExpressionEditor.type("[Custom S");
+    H.CustomExpressionEditor.completion("Custom Sum")
+      .should("be.visible")
+      .click();
+    H.CustomExpressionEditor.value().should("eq", "[Custom Sum]");
+    H.CustomExpressionEditor.format();
+
+    H.CustomExpressionEditor.nameInput().type("Derived");
+    H.popover().button("Done").click();
+
+    H.visualize();
+    H.assertTableData({
+      columns: ["Custom Sum", "Derived"],
+    });
+  });
+
+  it("should not be possible to create cycles in custom aggregations", () => {
+    H.createQuestion(
+      {
+        query: {
+          "source-table": ORDERS_ID,
+          aggregation: [
+            [
+              "aggregation-options",
+              ["sum", ["field", ORDERS.TOTAL, null]],
+              {
+                name: "Custom Sum",
+                "display-name": "Custom Sum",
+              },
+            ],
+          ],
+        },
+      },
+      { visitQuestion: true },
+    );
+
+    H.openNotebook();
+
+    H.getNotebookStep("summarize").icon("add").click();
+    H.popover().findByText("Custom Expression").scrollIntoView().click();
+    H.CustomExpressionEditor.type("[Custom Sum] + 1");
+    H.CustomExpressionEditor.nameInput().type("Custom Sum 2");
+    H.popover().button("Done").click();
+
+    H.getNotebookStep("summarize").findByText("Custom Sum").click();
+    H.CustomExpressionEditor.clear().type("[Custom Sum 2]");
+
+    H.popover()
+      .findByText("Cycle detected: Custom Sum → Custom Sum 2 → Custom Sum")
+      .should("be.visible");
+  });
+});

frontend/src/metabase-lib/aggregation.ts

@@ -73,3 +73,11 @@ export function aggregationColumn(
 ): ColumnMetadata | null {
   return ML.aggregation_column(query, stageIndex, aggregation);
 }
+
+export function aggregableColumns(
+  query: Query,
+  stageIndex?: number,
+  expressionIndex?: number,
+): ColumnMetadata[] {
+  return ML.aggregable_columns(query, stageIndex, expressionIndex);
+}

frontend/src/metabase-lib/test-helpers.ts

@@ -179,7 +179,7 @@ interface BreakoutClauseOpts {
   binningStrategyName?: string;
 }
 
-interface ExpressionClauseOpts {
+export interface ExpressionClauseOpts {
   name: string;
   operator: Lib.ExpressionOperator;
   args: (Lib.ExpressionArg | Lib.ExpressionClause)[];

frontend/src/metabase-lib/v1/expressions/complete/fields.unit.spec.ts

@@ -14,7 +14,8 @@ import {
   REVIEWS_ID,
 } from "metabase-types/api/mocks/presets";
 
-import { sharedMetadata } from "../test/shared";
+import { columnsForExpressionMode } from "../mode";
+import { queryWithAggregation, sharedMetadata } from "../test/shared";
 
 import { complete } from "./__support__";
 import { suggestFields } from "./fields";
@@ -67,11 +68,12 @@ describe("suggestFields", () => {
     const source = suggestFields({
       query,
       stageIndex,
-      availableColumns: Lib.expressionableColumns(
+      availableColumns: columnsForExpressionMode({
         query,
         stageIndex,
+        expressionMode: "expression",
         expressionIndex,
-      ),
+      }),
     });
 
     return function (doc: string) {
@@ -191,7 +193,11 @@ describe("suggestFields", () => {
     const source = suggestFields({
       query,
       stageIndex,
-      availableColumns: Lib.expressionableColumns(query, stageIndex),
+      availableColumns: columnsForExpressionMode({
+        query,
+        stageIndex,
+        expressionMode: "expression",
+      }),
     });
 
     const result = complete(source, "[Use|");
@@ -359,7 +365,11 @@ describe("suggestFields", () => {
     const source = suggestFields({
       query,
       stageIndex,
-      availableColumns: Lib.expressionableColumns(query, stageIndex),
+      availableColumns: columnsForExpressionMode({
+        query,
+        stageIndex,
+        expressionMode: "expression",
+      }),
     });
 
     complete(source, "Foo|");
@@ -415,10 +425,11 @@ describe("suggestFields", () => {
     const source = suggestFields({
       query,
       stageIndex: stageIndexAfterNesting,
-      availableColumns: Lib.expressionableColumns(
+      availableColumns: columnsForExpressionMode({
         query,
-        stageIndexAfterNesting,
-      ),
+        stageIndex: stageIndexAfterNesting,
+        expressionMode: "expression",
+      }),
     });
 
     const result = complete(source, "T|");
@@ -451,4 +462,71 @@ describe("suggestFields", () => {
       ],
     });
   });
+
+  it.each(["expression", "filter"] as const)(
+    "should not suggest aggregations when expressionMode = %s",
+    async (expressionMode) => {
+      const query = queryWithAggregation;
+      const stageIndex = -1;
+
+      const source = suggestFields({
+        query,
+        stageIndex,
+        availableColumns: columnsForExpressionMode({
+          query,
+          stageIndex,
+          expressionMode,
+        }),
+      });
+
+      const result = await complete(source, "[Bar aggregat|]");
+      const aggregations = result?.options.filter(
+        (option) => option.displayLabel === "Bar Aggregation",
+      );
+      expect(aggregations).toHaveLength(0);
+    },
+  );
+
+  it("should suggest aggregations when expressionMode = aggregation", async () => {
+    const query = queryWithAggregation;
+    const stageIndex = -1;
+    const source = suggestFields({
+      query,
+      stageIndex,
+      availableColumns: columnsForExpressionMode({
+        query,
+        stageIndex,
+        expressionMode: "aggregation",
+      }),
+    });
+
+    const result = await complete(source, "[Bar aggregat|]");
+    const aggregations = result?.options.filter(
+      (option) => option.displayLabel === "Bar Aggregation",
+    );
+    expect(aggregations).toHaveLength(1);
+  });
+
+  it.each(["expression", "filter", "aggregation"] as const)(
+    "should suggest aggregations when expressionMode = %s in later stages",
+    async (expressionMode) => {
+      const query = Lib.appendStage(queryWithAggregation);
+      const stageIndex = -1;
+      const source = suggestFields({
+        query,
+        stageIndex,
+        availableColumns: columnsForExpressionMode({
+          query,
+          stageIndex,
+          expressionMode,
+        }),
+      });
+
+      const result = await complete(source, "[Bar aggregat|]");
+      const aggregations = result?.options.filter(
+        (option) => option.displayLabel === "Bar Aggregation",
+      );
+      expect(aggregations).toHaveLength(1);
+    },
+  );
 });

frontend/src/metabase-lib/v1/expressions/mode.ts

@@ -1,6 +1,6 @@
 import { t } from "ttag";
 
-import type * as Lib from "metabase-lib";
+import * as Lib from "metabase-lib";
 
 import { getClauseDefinition, isDefinedClause } from "./clause";
 import { DiagnosticError } from "./errors";
@@ -40,3 +40,20 @@ export function checkExpressionModeSupportsClause(
 
   return null;
 }
+
+export function columnsForExpressionMode({
+  query,
+  stageIndex = -1,
+  expressionIndex,
+  expressionMode,
+}: {
+  query: Lib.Query;
+  stageIndex?: number;
+  expressionIndex?: number;
+  expressionMode: Lib.ExpressionMode;
+}) {
+  if (expressionMode === "aggregation") {
+    return Lib.aggregableColumns(query, stageIndex, expressionIndex);
+  }
+  return Lib.expressionableColumns(query, stageIndex, expressionIndex);
+}

frontend/src/metabase-lib/v1/expressions/resolver.ts

@@ -35,7 +35,7 @@ export function resolver(options: Options): Resolver {
       // Return metrics
       const dimension = findByName([...metrics(), ...availableColumns]);
       if (!dimension) {
-        throw new CompileError(t`Unknown Metric: ${name}`, node);
+        throw new CompileError(t`Unknown Aggregation or Metric: ${name}`, node);
       } else if (!Lib.isMetricMetadata(dimension)) {
         // If no metric was found, but there is a matching column,
         // show a more sophisticated error message
@@ -72,7 +72,10 @@ export function resolver(options: Options): Resolver {
     ]);
     if (!dimension) {
       if (expressionMode === "aggregation") {
-        throw new CompileError(t`Unknown column or Metric: ${name}`, node);
+        throw new CompileError(
+          t`Unknown column, Aggregation or Metric: ${name}`,
+          node,
+        );
       }
       throw new CompileError(t`Unknown column: ${name}`, node);
     }

frontend/src/metabase-lib/v1/expressions/resolver.unit.spec.ts

@@ -1,11 +1,15 @@
 import * as Lib from "metabase-lib";
 
+import { columnsForExpressionMode } from "./mode";
 import { resolver as makeResolver } from "./resolver";
 import {
   expressions,
   fields,
+  findDimensions,
+  findField,
   metrics,
   query,
+  queryWithAggregation,
   segments,
   stageIndex,
 } from "./test/shared";
@@ -215,7 +219,7 @@ describe("resolver", () => {
 
         it("should not resolve fields", () => {
           expect(() => aggregation("Unknown")).toThrow(
-            "Unknown Metric: Unknown",
+            "Unknown Aggregation or Metric: Unknown",
           );
           expect(() => aggregation("Created At")).toThrow(
             "No aggregation found in: Created At. Use functions like Sum() or custom Metrics",
@@ -230,13 +234,13 @@ describe("resolver", () => {
 
         it("should not resolve unknown fields", () => {
           expect(() => aggregation("Unknown")).toThrow(
-            "Unknown Metric: Unknown",
+            "Unknown Aggregation or Metric: Unknown",
           );
         });
 
         it("should not resolve segments", () => {
           expect(() => aggregation("Expensive Things")).toThrow(
-            "Unknown Metric: Expensive Things",
+            "Unknown Aggregation or Metric: Expensive Things",
           );
         });
 
@@ -250,15 +254,49 @@ describe("resolver", () => {
         expect(resolve("number", "BAR")).toEqual(expressions.BAR_UPPER);
         expect(resolve("number", "bar")).toEqual(expressions.BAR_LOWER);
       });
+
+      describe("should never resolve aggregations", () => {
+        const query = queryWithAggregation;
+
+        const resolve = makeResolver({
+          query,
+          stageIndex,
+          expressionMode,
+          availableColumns: columnsForExpressionMode({
+            query,
+            stageIndex,
+            expressionMode,
+          }),
+        });
+
+        it.each(["number", "datetime", "string", "boolean"] as const)(
+          "type = %s",
+          (type) => {
+            expect(() => resolve(type, "Bar Aggregation")).toThrow(
+              type === "boolean"
+                ? "Unknown Segment or boolean column: Bar Aggregation"
+                : "Unknown column: Bar Aggregation",
+            );
+          },
+        );
+      });
     },
   );
 
   describe("expressionMode = aggregation", () => {
+    const query = queryWithAggregation;
+    const { fields, expressions, segments, metrics, aggregations } =
+      findDimensions(query);
+
     const resolve = makeResolver({
       query,
       stageIndex,
       expressionMode: "aggregation",
-      availableColumns: Lib.expressionableColumns(query, stageIndex),
+      availableColumns: columnsForExpressionMode({
+        query,
+        stageIndex,
+        expressionMode: "aggregation",
+      }),
     });
 
     describe("type = boolean", () => {
@@ -292,6 +330,12 @@ describe("resolver", () => {
           "Unknown Segment or boolean column: Foo Metric",
         );
       });
+
+      it("should not resolve aggregations", () => {
+        expect(() => boolean("Bar Aggregation")).toThrow(
+          "Unknown Segment or boolean column: Bar Aggregation",
+        );
+      });
     });
 
     describe("type = string", () => {
@@ -311,19 +355,23 @@ describe("resolver", () => {
 
       it("should not resolve unknown fields", () => {
         expect(() => string("Unknown")).toThrow(
-          "Unknown column or Metric: Unknown",
+          "Unknown column, Aggregation or Metric: Unknown",
         );
       });
 
       it("should not resolve segments", () => {
         expect(() => string("Expensive Things")).toThrow(
-          "Unknown column or Metric: Expensive Things",
+          "Unknown column, Aggregation or Metric: Expensive Things",
         );
       });
 
       it("should resolve metrics", () => {
         expect(string("Foo Metric")).toEqual(metrics.FOO);
       });
+
+      it("should resolve aggregations", () => {
+        expect(string("Bar Aggregation")).toEqual(aggregations.BAR_AGGREGATION);
+      });
     });
 
     describe("type = number", () => {
@@ -339,19 +387,23 @@ describe("resolver", () => {
 
       it("should not resolve unknown fields", () => {
         expect(() => number("Unknown")).toThrow(
-          "Unknown column or Metric: Unknown",
+          "Unknown column, Aggregation or Metric: Unknown",
         );
       });
 
       it("should not resolve segments", () => {
         expect(() => number("Expensive Things")).toThrow(
-          "Unknown column or Metric: Expensive Things",
+          "Unknown column, Aggregation or Metric: Expensive Things",
         );
       });
 
       it("should resolve metrics", () => {
         expect(number("Foo Metric")).toEqual(metrics.FOO);
       });
+
+      it("should resolve aggregations", () => {
+        expect(number("Bar Aggregation")).toEqual(aggregations.BAR_AGGREGATION);
+      });
     });
 
     describe("type = datetime", () => {
@@ -363,19 +415,25 @@ describe("resolver", () => {
 
       it("should not resolve unknown fields", () => {
         expect(() => datetime("Unknown")).toThrow(
-          "Unknown column or Metric: Unknown",
+          "Unknown column, Aggregation or Metric: Unknown",
         );
       });
 
       it("should not resolve segments", () => {
         expect(() => datetime("Expensive Things")).toThrow(
-          "Unknown column or Metric: Expensive Things",
+          "Unknown column, Aggregation or Metric: Expensive Things",
         );
       });
 
       it("should resolve metrics", () => {
         expect(datetime("Foo Metric")).toEqual(metrics.FOO);
       });
+
+      it("should resolve aggregations", () => {
+        expect(datetime("Bar Aggregation")).toEqual(
+          aggregations.BAR_AGGREGATION,
+        );
+      });
     });
 
     describe("type = any", () => {
@@ -390,7 +448,7 @@ describe("resolver", () => {
 
       it("should not resolve unknown fields", () => {
         expect(() => any("Unknown")).toThrow(
-          "Unknown column or Metric: Unknown",
+          "Unknown column, Aggregation or Metric: Unknown",
         );
       });
 
@@ -398,13 +456,17 @@ describe("resolver", () => {
         // We do not resolve segments here since any is only used in offset
         // and offset only works in aggregations.
         expect(() => any("Expensive Things")).toThrow(
-          "Unknown column or Metric: Expensive Things",
+          "Unknown column, Aggregation or Metric: Expensive Things",
         );
       });
 
       it("should resolve metrics", () => {
         expect(any("Foo Metric")).toEqual(metrics.FOO);
       });
+
+      it("should resolve aggregations", () => {
+        expect(any("Bar Aggregation")).toEqual(aggregations.BAR_AGGREGATION);
+      });
     });
 
     describe("type = expression", () => {
@@ -419,26 +481,34 @@ describe("resolver", () => {
 
       it("should not resolve unknown fields", () => {
         expect(() => expression("Unknown")).toThrow(
-          "Unknown column or Metric: Unknown",
+          "Unknown column, Aggregation or Metric: Unknown",
         );
       });
 
       it("should not resolve segments", () => {
         expect(() => expression("Expensive Things")).toThrow(
-          "Unknown column or Metric: Expensive Things",
+          "Unknown column, Aggregation or Metric: Expensive Things",
         );
       });
 
       it("should resolve metrics", () => {
         expect(expression("Foo Metric")).toEqual(metrics.FOO);
       });
+
+      it("should resolve aggregations", () => {
+        expect(expression("Bar Aggregation")).toEqual(
+          aggregations.BAR_AGGREGATION,
+        );
+      });
     });
 
     describe("type = aggregation", () => {
       const aggregation = (name: string) => resolve("aggregation", name);
 
       it("should not resolve fields", () => {
-        expect(() => aggregation("Unknown")).toThrow("Unknown Metric: Unknown");
+        expect(() => aggregation("Unknown")).toThrow(
+          "Unknown Aggregation or Metric: Unknown",
+        );
         expect(() => aggregation("Created At")).toThrow(
           "No aggregation found in: Created At. Use functions like Sum() or custom Metrics",
         );
@@ -451,18 +521,26 @@ describe("resolver", () => {
       });
 
       it("should not resolve unknown fields", () => {
-        expect(() => aggregation("Unknown")).toThrow("Unknown Metric: Unknown");
+        expect(() => aggregation("Unknown")).toThrow(
+          "Unknown Aggregation or Metric: Unknown",
+        );
       });
 
       it("should not resolve segments", () => {
         expect(() => aggregation("Expensive Things")).toThrow(
-          "Unknown Metric: Expensive Things",
+          "Unknown Aggregation or Metric: Expensive Things",
         );
       });
 
       it("should resolve metrics", () => {
         expect(aggregation("Foo Metric")).toEqual(metrics.FOO);
       });
+
+      it("should resolve aggregations", () => {
+        expect(aggregation("Bar Aggregation")).toEqual(
+          aggregations.BAR_AGGREGATION,
+        );
+      });
     });
 
     it("should allow resolving field with exact case matches first", () => {
@@ -470,4 +548,86 @@ describe("resolver", () => {
       expect(resolve("number", "bar")).toEqual(expressions.BAR_LOWER);
     });
   });
+
+  describe("query with aggregation in first stage > later stages", () => {
+    const query = Lib.appendStage(queryWithAggregation);
+    const aggregations = {
+      BAR_AGGREGATION: findField(query, "Bar Aggregation"),
+    };
+
+    const resolve = makeResolver({
+      query,
+      stageIndex,
+      expressionMode: "expression",
+      availableColumns: columnsForExpressionMode({
+        query,
+        stageIndex,
+        expressionMode: "expression",
+      }),
+    });
+
+    describe("type = boolean", () => {
+      const boolean = (name: string) => resolve("boolean", name);
+
+      it("should not resolve non-boolean aggregations", () => {
+        expect(() => boolean("Bar Aggregation")).toThrow(
+          "Unknown Segment or boolean column: Bar Aggregation",
+        );
+      });
+    });
+
+    describe("type = string", () => {
+      const string = (name: string) => resolve("string", name);
+
+      it("should resolve aggregations", () => {
+        expect(string("Bar Aggregation")).toEqual(aggregations.BAR_AGGREGATION);
+      });
+    });
+
+    describe("type = number", () => {
+      const number = (name: string) => resolve("number", name);
+
+      it("should resolve aggregations", () => {
+        expect(number("Bar Aggregation")).toEqual(aggregations.BAR_AGGREGATION);
+      });
+    });
+
+    describe("type = datetime", () => {
+      const datetime = (name: string) => resolve("datetime", name);
+
+      it("should resolve aggregations", () => {
+        expect(datetime("Bar Aggregation")).toEqual(
+          aggregations.BAR_AGGREGATION,
+        );
+      });
+    });
+
+    describe("type = any", () => {
+      const any = (name: string) => resolve("any", name);
+
+      it("should resolve aggregations", () => {
+        expect(any("Bar Aggregation")).toEqual(aggregations.BAR_AGGREGATION);
+      });
+    });
+
+    describe("type = expression", () => {
+      const expression = (name: string) => resolve("expression", name);
+
+      it("should resolve aggregations", () => {
+        expect(expression("Bar Aggregation")).toEqual(
+          aggregations.BAR_AGGREGATION,
+        );
+      });
+    });
+
+    describe("type = aggregation", () => {
+      const aggregation = (name: string) => resolve("aggregation", name);
+
+      it("should not resolve aggregation as it is now just a field", () => {
+        expect(() => aggregation("Bar Aggregation")).toThrow(
+          "No aggregation found in: Bar Aggregation. Use functions like Sum() or custom Metrics",
+        );
+      });
+    });
+  });
 });

frontend/src/metabase-lib/v1/expressions/test/shared.ts

@@ -1,7 +1,11 @@
 import { createMockMetadata } from "__support__/metadata";
 import { getNextId } from "__support__/utils";
 import * as Lib from "metabase-lib";
-import { createQuery, createQueryWithClauses } from "metabase-lib/test-helpers";
+import {
+  type ExpressionClauseOpts,
+  createQuery,
+  createQueryWithClauses,
+} from "metabase-lib/test-helpers";
 import {
   COMMON_DATABASE_FEATURES,
   createMockCard,
@@ -68,6 +72,44 @@ const metadata = createMockMetadata({
   ],
 });
 
+const exprs: ExpressionClauseOpts[] = [
+  {
+    name: "foo",
+    operator: "+",
+    args: [1, 2],
+  },
+  {
+    name: "bool",
+    operator: "=",
+    args: [1, 1],
+  },
+  {
+    name: "name with [brackets]",
+    operator: "+",
+    args: [1, 2],
+  },
+  {
+    name: "name with \\ slash",
+    operator: "+",
+    args: [1, 2],
+  },
+  {
+    name: "stringly",
+    operator: "concat",
+    args: ["foo", "bar"],
+  },
+  {
+    name: "bar",
+    operator: "+",
+    args: [1, 2],
+  },
+  {
+    name: "BAR",
+    operator: "+",
+    args: [1, 2],
+  },
+];
+
 export const query = createQueryWithClauses({
   query: createQuery({
     metadata,
@@ -79,47 +121,33 @@ export const query = createQueryWithClauses({
       }),
     }),
   }),
-  expressions: [
-    {
-      name: "foo",
-      operator: "+",
-      args: [1, 2],
-    },
-    {
-      name: "bool",
-      operator: "=",
-      args: [1, 1],
-    },
-    {
-      name: "name with [brackets]",
-      operator: "+",
-      args: [1, 2],
-    },
-    {
-      name: "name with \\ slash",
-      operator: "+",
-      args: [1, 2],
-    },
-    {
-      name: "stringly",
-      operator: "concat",
-      args: ["foo", "bar"],
-    },
-    {
-      name: "bar",
-      operator: "+",
-      args: [1, 2],
-    },
-    {
-      name: "BAR",
-      operator: "+",
-      args: [1, 2],
-    },
-  ],
+  expressions: exprs,
 });
+
+export const queryWithAggregation = createQueryWithClauses({
+  query: createQuery({
+    metadata,
+    query: createMockStructuredDatasetQuery({
+      database: SAMPLE_DB_ID,
+      query: createMockStructuredQuery({
+        "source-table": ORDERS_ID,
+        fields: [],
+        aggregation: [
+          [
+            "aggregation-options",
+            ["sum", ["field", ORDERS.TOTAL, null]],
+            { name: "Bar Aggregation", "display-name": "Bar Aggregation" },
+          ],
+        ],
+      }),
+    }),
+  }),
+  expressions: exprs,
+});
+
 export const stageIndex = -1;
 
-function findField(name: string) {
+export function findField(query: Lib.Query, name: string) {
   const columns = Lib.expressionableColumns(query, stageIndex);
   for (const column of columns) {
     const info = Lib.displayInfo(query, stageIndex, column);
@@ -131,6 +159,7 @@ function findField(name: string) {
 }
 
 function findFields<const T extends { [key: string]: unknown }>(
+  query: Lib.Query,
   table: T,
 ): {
   [K in keyof T]: Lib.ColumnMetadata;
@@ -140,12 +169,12 @@ function findFields<const T extends { [key: string]: unknown }>(
   };
 
   for (const name in table) {
-    res[name] = findField(name);
+    res[name] = findField(query, name);
   }
   return res;
 }
 
-function findSegment(name: string) {
+function findSegment(query: Lib.Query, name: string) {
   const segments = Lib.availableSegments(query, stageIndex);
   for (const segment of segments) {
     const info = Lib.displayInfo(query, stageIndex, segment);
@@ -156,7 +185,7 @@ function findSegment(name: string) {
   throw new Error(`Could not find segment: ${name}`);
 }
 
-function findMetric(name: string) {
+function findMetric(query: Lib.Query, name: string) {
   const metrics = Lib.availableMetrics(query, stageIndex);
   for (const metric of metrics) {
     const info = Lib.displayInfo(query, stageIndex, metric);
@@ -167,28 +196,57 @@ function findMetric(name: string) {
   throw new Error(`Could not find metric: ${name}`);
 }
 
-export const fields = {
-  orders: findFields(ORDERS),
-  people: findFields(PEOPLE),
-  products: findFields(PRODUCTS),
-};
+function findAggregation(query: Lib.Query, name: string) {
+  if (query !== queryWithAggregation) {
+    return null;
+  }
+  const aggregations = Lib.aggregableColumns(query, stageIndex);
+  for (const aggregation of aggregations) {
+    const info = Lib.displayInfo(query, stageIndex, aggregation);
+    if (info.displayName === name) {
+      return aggregation;
+    }
+  }
+  throw new Error(`Could not find aggregation: ${name}`);
+}
 
-export const expressions = {
-  BOOL: findField("bool"),
-  FOO: findField("foo"),
-  NAME_WITH_BRACKETS: findField("name with [brackets]"),
-  NAME_WITH_SLASH: findField("name with \\ slash"),
-  STRINGLY: findField("stringly"),
-  BAR_UPPER: findField("BAR"),
-  BAR_LOWER: findField("bar"),
-};
+export function findDimensions(query: Lib.Query) {
+  const fields = {
+    orders: findFields(query, ORDERS),
+    people: findFields(query, PEOPLE),
+    products: findFields(query, PRODUCTS),
+  };
 
-export const segments = {
-  EXPENSIVE_THINGS: findSegment("Expensive Things"),
-};
+  const expressions = {
+    BOOL: findField(query, "bool"),
+    FOO: findField(query, "foo"),
+    NAME_WITH_BRACKETS: findField(query, "name with [brackets]"),
+    NAME_WITH_SLASH: findField(query, "name with \\ slash"),
+    STRINGLY: findField(query, "stringly"),
+    BAR_UPPER: findField(query, "BAR"),
+    BAR_LOWER: findField(query, "bar"),
+  };
 
-export const metrics = {
-  FOO: findMetric("Foo Metric"),
-};
+  const segments = {
+    EXPENSIVE_THINGS: findSegment(query, "Expensive Things"),
+  };
+
+  const metrics = {
+    FOO: findMetric(query, "Foo Metric"),
+  };
+
+  const aggregations = {
+    BAR_AGGREGATION: findAggregation(query, "Bar Aggregation"),
+  };
+
+  return {
+    fields,
+    expressions,
+    segments,
+    metrics,
+    aggregations,
+  };
+}
+export const { fields, expressions, segments, metrics } = findDimensions(query);
 
 export const sharedMetadata = metadata;

frontend/src/metabase/common/components/AggregationPicker/AggregationPicker.tsx

@@ -193,8 +193,8 @@ export function AggregationPicker({
   ]);
 
   const availableColumns = useMemo(
-    () => Lib.expressionableColumns(query, stageIndex),
-    [query, stageIndex],
+    () => Lib.aggregableColumns(query, stageIndex, clauseIndex),
+    [query, stageIndex, clauseIndex],
   );
 
   const checkIsItemSelected = useCallback(
@@ -306,6 +306,7 @@ export function AggregationPicker({
         clause={clause}
         withName
         expressionMode="aggregation"
+        expressionIndex={clauseIndex}
         header={<ExpressionWidgetHeader onBack={closeExpressionEditor} />}
         onChangeClause={handleClauseChange}
         onClose={closeExpressionEditor}

frontend/src/metabase/query_builder/components/expressions/ExpressionWidget.unit.spec.tsx

@@ -128,7 +128,7 @@ describe("ExpressionWidget", () => {
       const doneButton = screen.getByRole("button", { name: "Done" });
       expect(doneButton).toBeDisabled();
 
-      await screen.findByText("Unknown Metric: Imaginary");
+      await screen.findByText("Unknown Aggregation or Metric: Imaginary");
     });
 
     it("should show 'no aggregation found' error if the identifier is recognized as a dimension (metabase#50753)", async () => {

src/metabase/legacy_mbql/schema.cljc

@@ -475,8 +475,7 @@
    [:numeric-expression NumericExpression]
    [:aggregation        Aggregation]
    [:value              value]
-   [:field              Field]])
-
+   [:field              Reference]])
 (def ^:private NumericExpressionArg
   [:ref ::NumericExpressionArg])
 
@@ -989,7 +988,8 @@
 (mr/def ::NumericExpression
   (one-of + - / * coalesce length floor ceil round abs power sqrt exp log case case:if datetime-diff integer float
           temporal-extract get-year get-quarter get-month get-week get-day get-day-of-week
-          get-hour get-minute get-second))
+          get-hour get-minute get-second
+          aggregation))
 
 (mr/def ::StringExpression
   (one-of substring trim ltrim rtrim replace lower upper concat regex-match-first coalesce case case:if host domain
@@ -1086,7 +1086,7 @@
                        :numeric-expression
                        :else))}
    [:numeric-expression NumericExpression]
-   [:else (one-of avg cum-sum distinct distinct-where stddev sum min max metric share count-where
+   [:else (one-of aggregation avg cum-sum distinct distinct-where stddev sum min max metric share count-where
                   sum-where case case:if median percentile ag:var cum-count count offset)]])
 
 (def ^:private UnnamedAggregation

src/metabase/lib/aggregation.cljc

@@ -25,6 +25,7 @@
   "Given `:metadata/column` column metadata for an aggregation, construct an `:aggregation` reference."
   [metadata :- ::lib.schema.metadata/column]
   (let [options {:lib/uuid        (str (random-uuid))
+                 :base-type       (:base-type metadata)
                  :effective-type  ((some-fn :effective-type :base-type) metadata)
                  :lib/source-name (:name metadata)}
         ag-uuid (:lib/source-uuid metadata)]
@@ -443,3 +444,21 @@
     (->> (lib.util/query-stage query stage-number)
          (lib.metadata.calculation/visible-columns query stage-number)
          (lib.equality/find-matching-column column-ref))))
+
+(mu/defn aggregable-columns :- [:maybe [:sequential ::lib.schema.metadata/column]]
+  "Returns the columns that can be used in aggregation expressions."
+  ([query :- ::lib.schema/query
+    aggregation-position :- [:maybe ::lib.schema.common/int-greater-than-or-equal-to-zero]]
+   (aggregable-columns query -1 aggregation-position))
+  ([query :- ::lib.schema/query
+    stage-number :- :int
+    _aggregation-position :- [:maybe ::lib.schema.common/int-greater-than-or-equal-to-zero]]
+   (let [stage (lib.util/query-stage query stage-number)
+         columns (into (vec (lib.metadata.calculation/visible-columns query stage-number stage))
+                       (aggregations-metadata query stage-number))]
+     (not-empty columns))))
+
+(defmethod lib.metadata.calculation/type-of-method :aggregation
+  [query stage-number [_aggregation _opts agg-uuid, :as _aggregation-ref]]
+  (let [expression (resolve-aggregation query stage-number agg-uuid)]
+    (lib.metadata.calculation/type-of query stage-number expression)))

src/metabase/lib/convert.cljc

@@ -182,10 +182,7 @@
                             (map-indexed (fn [idx [_tag {ag-uuid :lib/uuid}]]
                                            [idx ag-uuid]))
                             aggregations)
-        pMBQL->legacy (into {}
-                            (map-indexed (fn [idx [_tag {ag-uuid :lib/uuid}]]
-                                           [ag-uuid idx]))
-                            aggregations)]
+        pMBQL->legacy (set/map-invert legacy->pMBQL)]
     (binding [*legacy-index->pMBQL-uuid* legacy->pMBQL
               *pMBQL-uuid->legacy-index* pMBQL->legacy]
       (thunk))))
@@ -197,10 +194,35 @@
      [aggregations & body]
      `(do-with-aggregation-list ~aggregations (fn [] ~@body))))
 
+(defn- index-ref-clauses->pMBQL [clauses]
+  (letfn [(pass [state]
+            (reduce (fn [{:keys [index index->uuid converted] :as state} clause]
+                      (-> (if (get converted index)
+                            state
+                            (try
+                              (let [pMBQL (binding [*legacy-index->pMBQL-uuid* index->uuid]
+                                            (->pMBQL clause))]
+                                (-> state
+                                    (update :index->uuid assoc index (lib.options/uuid pMBQL))
+                                    (update :converted assoc index pMBQL)))
+                              (catch #?(:cljs js/Error :clj clojure.lang.ExceptionInfo) e
+                                (if (= (-> e ex-data :error) ::legacy-index->pMBQL-uuid-missing)
+                                  state
+                                  (throw e)))))
+                          (update :index inc)))
+                    (assoc state :index 0)
+                    clauses))]
+    (loop [state {:index->uuid {}, :converted (vec (repeat (count clauses) nil))}]
+      (let [{:keys [index->uuid converted] :as state'} (pass state)]
+        (cond
+          (= (count index->uuid) (count clauses)) converted
+          (= converted (:converted state))        (throw (ex-info "Couldn't index clauses" {:clauses clauses}))
+          :else                                   (recur state'))))))
+
 (defn- from-indexed-idents [stage list-key idents-key]
-  (let [idents (get stage idents-key)]
-    (->> (get stage list-key)
-         ->pMBQL
+  (let [idents (get stage idents-key)
+        clauses (get stage list-key)]
+    (->> (index-ref-clauses->pMBQL clauses)
          (map-indexed (fn [i x]
                         (if-let [ident (or (get idents i)
                                            ;; Conversion from JSON keywordizes all keys, including these numbers!
@@ -327,7 +349,8 @@
    [tag opts (or (get *legacy-index->pMBQL-uuid* aggregation-index)
                  (throw (ex-info (str "Error converting :aggregation reference: no aggregation at index "
                                       aggregation-index)
-                                 {:clause clause})))]))
+                                 {:clause clause
+                                  :error ::legacy-index->pMBQL-uuid-missing})))]))
 
 (defmethod ->pMBQL :aggregation-options
   [[_tag aggregation options]]
@@ -434,6 +457,10 @@
       [:aggregation-options inner aggregation-opts]
       inner)))
 
+(defmethod aggregation->legacy-MBQL :aggregation
+  [clause]
+  (->legacy-MBQL clause))
+
 (defmethod aggregation->legacy-MBQL :offset
   [clause]
   (->legacy-MBQL clause))

src/metabase/lib/core.cljc

@@ -84,6 +84,7 @@
 
 (shared.ns/import-fns
  [lib.aggregation
+  aggregable-columns
   aggregate
   aggregation-clause
   aggregation-column

src/metabase/lib/expression.cljc

@@ -528,6 +528,16 @@
         (map #(get % 2))
         (lib.util.match/match expr :expression)))
 
+(defn- aggregation->name
+  [query stage-number aggregation]
+  (lib.metadata.calculation/display-name query stage-number aggregation))
+
+(defn- referred-aggregations
+  [agg]
+  (into #{}
+        (map #(get % 2))
+        (lib.util.match/match agg :aggregation)))
+
 (defn- cyclic-definition
   ([node->children]
    (some #(cyclic-definition node->children %) (keys node->children)))
@@ -570,13 +580,6 @@
      (lib.util/clause? expr)
      (some #(invalid-nesting % (conj path-tags (first expr))) (nnext expr)))))
 
-(comment
-  (invalid-nesting [:sum {:lib/uuid (str (random-uuid))}
-                    [:avg {:lib/uuid (str (random-uuid))}
-                     [:field {:lib/uuid (str (random-uuid))} 123]]])
-  (lib.hierarchy/isa? :sum ::lib.schema.aggregation/aggregation-clause-tag)
-  -)
-
 (mu/defn diagnose-expression :- [:maybe [:map [:message :string]]]
   "Checks `expr` for type errors and, if `expression-mode` is :expression and
   `expression-position` is provided, for cyclic references with other expressions.
@@ -606,15 +609,26 @@
       (or (when-not (validator expr)
             {:message  (i18n/tru "Types are incompatible.")
              :friendly true})
-          (when-let [dependency-path (and (= expression-mode :expression)
-                                          expression-position
-                                          (let [exprs (expressions query stage-number)
-                                                edited-expr (nth exprs expression-position)
-                                                edited-name (expression->name edited-expr)
-                                                deps (-> (m/index-by expression->name exprs)
-                                                         (assoc edited-name expr)
-                                                         (update-vals referred-expressions))]
-                                            (cyclic-definition deps)))]
+          (when-let [dependency-path
+                     (when expression-position
+                       (clojure.core/case expression-mode
+                         :expression (let [exprs (expressions query stage-number)
+                                           edited-expr (nth exprs expression-position)
+                                           edited-name (expression->name edited-expr)
+                                           deps (-> (m/index-by expression->name exprs)
+                                                    (assoc edited-name expr)
+                                                    (update-vals referred-expressions))]
+                                       (cyclic-definition deps))
+                         :aggregation (let [aggs (:aggregation (lib.util/query-stage query stage-number))
+                                            edited-expr (assoc expr 1 (get-in aggs [expression-position 1]))
+                                            uuid->agg (-> aggs
+                                                          (assoc expression-position edited-expr)
+                                                          (->> (m/index-by lib.options/uuid)))
+                                            deps (update-vals uuid->agg referred-aggregations)]
+                                        (some->> (cyclic-definition deps)
+                                                 (map (comp #(aggregation->name query stage-number %)
+                                                            uuid->agg))))
+                         nil))]
             {:message (i18n/tru "Cycle detected: {0}" (str/join " → " dependency-path))
              :friendly true})
           (when-let [nested (invalid-nesting expr)]

src/metabase/lib/fe_util.cljc

@@ -1,7 +1,9 @@
 (ns metabase.lib.fe-util
   (:require
    [inflections.core :as inflections]
+   [medley.core :as m]
    [metabase.legacy-mbql.normalize :as mbql.normalize]
+   [metabase.lib.aggregation :as lib.aggregation]
    [metabase.lib.card :as lib.card]
    [metabase.lib.common :as lib.common]
    [metabase.lib.convert :as lib.convert]
@@ -162,12 +164,17 @@
 
 (defmethod expression-parts-method :expression
   [query stage-number expression-ref]
-  ; Set the expression name as used in the ref as the expression might
-  ; have other aliases set on it which might be wrong.
+  ;; Set the expression name as used in the ref as the expression might
+  ;; have other aliases set on it which might be wrong.
   (lib.options/with-options
     (column-metadata-from-ref query stage-number expression-ref)
     {:lib/expression-name (last expression-ref)}))
 
+(defmethod expression-parts-method :aggregation
+  [query stage-number [_tag _opts aggregation-ref]]
+  (let [aggregation-columns (lib.aggregation/aggregations-metadata query stage-number)]
+    (m/find-first (comp #{aggregation-ref} :lib/source-uuid) aggregation-columns)))
+
 (mu/defn expression-parts :- [:or ExpressionArg ExpressionParts]
   "Return the parts of the filter clause `arg` in query `query` at stage `stage-number`."
   ([query value]

src/metabase/lib/js.cljs

@@ -1268,12 +1268,18 @@
   (and (map? arg) (= :metadata/column (:lib/type arg))))
 
 (defn ^:export metric-metadata?
-  "Returns true if arg is an MLv2 metric, ie. has `:lib/type :metadata/metric`.
+  "Returns true if arg is named entity that can be used as an aggregation expression on its own, i.e., without
+  wrapping it into an aggregating function.
+  Currently, this can be an MLv2 metric (`:lib/type :metadata/metric`) or an aggregation column
+  (`:lib/type :metadata/column` and `:lib/source :source/aggregations`).
 
   > **Code health:** Single use. This is used in the expression editor to parse and
   format expression clauses."
   [arg]
-  (and (map? arg) (= :metadata/metric (:lib/type arg))))
+  (and (map? arg)
+       (or (= (:lib/type arg) :metadata/metric)
+           (and (= (:lib/type arg) :metadata/column)
+                (= (:lib/source arg) :source/aggregations)))))
 
 (defn ^:export segment-metadata?
   "Returns true if arg is an MLv2 metric, ie. has `:lib/type :metadata/segment`.
@@ -1685,8 +1691,8 @@
 (defn ^:export expressionable-columns
   "Returns a JS array of those columns that can be used in an expression in the given stage of `a-query`.
 
-  Expressions can only see other expressions on the same stage which appear earlier in the list, so you must pass
-  `expression-position` (a 0-based index) when editing an existing expression.
+  `expression-position` (a 0-based index) containing the position of the expression in the list. It could be
+  significant when editing an existing expression, but it's not currently used.
 
   When creating a new expression, `expression-position` should be `nil` (JS `null` or `undefined`).
 
@@ -1703,6 +1709,27 @@
    (fn [_]
      (to-array (lib.core/expressionable-columns a-query stage-number expression-position)))))
 
+(defn ^:export aggregable-columns
+  "Returns a JS array of those columns that can be used in an aggregation expression in the given stage of `a-query`.
+
+  `expression-position` (a 0-based index) containing the position of the expression in the list. It could be
+  significant when editing an existing expression, but it's not currently used.
+
+  When creating a new expression, `expression-position` should be `nil` (JS `null` or `undefined`).
+
+  Cached on the query and stage.
+
+  > **Code health:** Healthy"
+  [a-query stage-number expression-position]
+  (lib.cache/side-channel-cache
+    ;; Caching is based on both the stage and expression position, since they can return different sets.
+    ;; TODO: Since these caches are mainly here to avoid expensively recomputing things in rapid succession, it would
+    ;; probably suffice to cache only the last position, and evict if it's different. But the lib.cache system doesn't
+    ;; support that currently.
+   (keyword "aggregable-columns" (str "stage-" stage-number "-" expression-position)) a-query
+   (fn [_]
+     (to-array (lib.core/aggregable-columns a-query stage-number expression-position)))))
+
 (defn ^:export column-extractions
   "Column extractions are a set of transformations possible on a given `column`, based on its type.
 

src/metabase/lib/metadata/result_metadata.cljc

@@ -234,7 +234,13 @@
       (let [fe-friendly-opts (dissoc opts :base-type :effective-type)]
         (if (seq fe-friendly-opts)
           [:expression expression-name fe-friendly-opts]
-          [:expression expression-name])))))
+          [:expression expression-name]))
+
+      [:aggregation aggregation-index (opts :guard (some-fn :base-type :effective-type))]
+      (let [fe-friendly-opts (dissoc opts :base-type :effective-type)]
+        (if (seq fe-friendly-opts)
+          [:aggregation aggregation-index fe-friendly-opts]
+          [:aggregation aggregation-index])))))
 
 (mu/defn- remove-join-alias-from-broken-field-ref?
   "Following the rules for the old 'annotate' code:

src/metabase/lib/schema/aggregation.cljc

@@ -96,7 +96,8 @@
 (mbql-clause/define-tuple-mbql-clause :var :- :type/Float
   #_expr [:schema [:ref ::expression/number]])
 
-(doseq [tag [:avg
+(doseq [tag [:aggregation
+             :avg
              :count
              :cum-count
              :count-where

src/metabase/query_processor/middleware/expand_aggregations.clj

@@ -0,0 +1,50 @@
+(ns metabase.query-processor.middleware.expand-aggregations
+  (:require
+   [medley.core :as m]
+   [metabase.lib.options :as lib.options]
+   [metabase.lib.util :as lib.util]
+   [metabase.lib.walk :as lib.walk]))
+
+(defn- expand-aggregation-ref
+  [aggregation-ref aggregation]
+  (let [name-opts  (-> aggregation-ref lib.options/options (select-keys [:name :display-name]))]
+    (-> aggregation
+        (lib.options/update-options merge name-opts)
+        lib.util/fresh-uuids)))
+
+(defn- unroll-form
+  [form aggregations expanded expanding]
+  (cond
+    (lib.util/clause-of-type? form :aggregation)
+    (let [ref (get form 2)
+          expansion (get expanded ref)
+          definition (get aggregations ref)]
+      (cond
+        expansion       [(expand-aggregation-ref form expansion) expanded]
+        (expanding ref) (throw (ex-info "cyclic aggregation definition" {:aggregations aggregations
+                                                                         :cycle-nodes expanding}))
+        definition      (let [[expansion expanded] (unroll-form definition aggregations expanded (conj expanding ref))]
+                          [(expand-aggregation-ref form expansion) (assoc expanded ref expansion)])
+        :else           (throw (ex-info "dangling aggregation reference" {:aggregations aggregations
+                                                                          :reference ref}))))
+
+    (vector? form)
+    (reduce (fn [[expansions expanded] form]
+              (let [[expansion expanded] (unroll-form form aggregations expanded expanding)]
+                [(conj expansions expansion) expanded]))
+            [[] expanded]
+            form)
+
+    :else
+    [form expanded]))
+
+(defn- unroll-aggregations
+  [aggregation-list]
+  (let [indexed (m/index-by lib.options/uuid aggregation-list)]
+    (first (unroll-form (vec aggregation-list) indexed {} #{}))))
+
+(defn expand-aggregations
+  "Recursively replace aggregation references in the aggregation clause with their definitions."
+  [query]
+  (lib.walk/walk-stages query (fn [_query _path stage]
+                                (m/update-existing stage :aggregation unroll-aggregations))))

src/metabase/query_processor/preprocess.clj

@@ -19,6 +19,7 @@
    [metabase.query-processor.middleware.cumulative-aggregations :as qp.cumulative-aggregations]
    [metabase.query-processor.middleware.desugar :as desugar]
    [metabase.query-processor.middleware.enterprise :as qp.middleware.enterprise]
+   [metabase.query-processor.middleware.expand-aggregations :as expand-aggregations]
    [metabase.query-processor.middleware.expand-macros :as expand-macros]
    [metabase.query-processor.middleware.fetch-source-query :as fetch-source-query]
    [metabase.query-processor.middleware.fix-bad-references :as fix-bad-refs]
@@ -112,6 +113,7 @@
    (ensure-pmbql #'qp.constraints/maybe-add-default-userland-constraints)
    (ensure-pmbql #'validate/validate-query)
    (ensure-pmbql #'fetch-source-query/resolve-source-cards)
+   (ensure-pmbql #'expand-aggregations/expand-aggregations)
    (ensure-pmbql #'metrics/adjust)
    (ensure-pmbql #'expand-macros/expand-macros)
    (ensure-pmbql #'qp.resolve-referenced/resolve-referenced-card-resources)

src/metabase/query_processor/util/add_alias_info.clj

@@ -117,9 +117,14 @@
       [:expression expression-name])
 
     [:aggregation index opts]
-    (if-let [opts (remove-namespaced-options opts)]
-      [:aggregation index opts]
-      [:aggregation index])
+    (do
+      (when (:base-type opts)
+        (throw (ex-info "agg-ref with type" {:clause [:aggregation index opts]})))
+      (if-let [opts (-> opts
+                        (dissoc :base-type)
+                        (remove-namespaced-options))]
+        [:aggregation index opts]
+        [:aggregation index]))
 
     _
     &match))

test/metabase/lib/aggregation_test.cljc

@@ -861,3 +861,23 @@
       (is (thrown? #?(:clj Exception :cljs js/Error)
                    (with-redefs [lib.util/ref-clause? (constantly true)]
                      (lib/selected-aggregation-operators available (first (lib/aggregations query)))))))))
+
+(deftest ^:parallel aggregable-columns-test
+  (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
+                  (lib/aggregate (lib/distinct (meta/field-metadata :venues :price))))]
+    (is (=? [{:name "ID",          :effective-type :type/BigInteger, :lib/source :source/table-defaults}
+             {:name "NAME",        :effective-type :type/Text,       :lib/source :source/table-defaults}
+             {:name "CATEGORY_ID", :effective-type :type/Integer,    :lib/source :source/table-defaults}
+             {:name "LATITUDE",    :effective-type :type/Float,      :lib/source :source/table-defaults}
+             {:name "LONGITUDE",   :effective-type :type/Float,      :lib/source :source/table-defaults}
+             {:name "PRICE",       :effective-type :type/Integer,    :lib/source :source/table-defaults}
+             {:name "ID",          :effective-type :type/BigInteger, :lib/source :source/implicitly-joinable}
+             {:name "NAME",        :effective-type :type/Text,       :lib/source :source/implicitly-joinable}
+             {:name "count",       :effective-type :type/Integer,    :lib/source :source/aggregations}]
+            (lib/aggregable-columns query nil)))))
+
+(deftest ^:parallel aggregation-ref-type-of-test
+  (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :venues))
+                  (lib/aggregate (lib/distinct (meta/field-metadata :venues :price))))]
+    (is (=? :type/Integer
+            (lib/type-of query (first (lib/aggregations query)))))))

test/metabase/lib/convert_test.cljc

@@ -225,7 +225,7 @@
                 :stages   [{:lib/type     :mbql.stage/mbql
                             :source-table 1
                             :aggregation  [[:sum {:lib/uuid ag-uuid}
-                                            [:field {:lib/uuid string?
+                                            [:field {:lib/uuid (str (random-uuid))
                                                      :effective-type :type/Integer} 1]]]
                             :breakout     [[:aggregation
                                             {:display-name   "Revenue"
@@ -1274,3 +1274,13 @@
                                     :lib/stage-metadata (symbol "nil #_\"key is not present.\"")
                                     :source-metadata    (symbol "nil #_\"key is not present.\"")}]}]}
                 (-> legacy lib.convert/->pMBQL lib.convert/->legacy-MBQL lib.convert/->pMBQL)))))))
+
+(deftest ^:parallel round-trip-aggregation-reference-test
+  (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
+                  (lib/aggregate (lib/sum (meta/field-metadata :orders :total))))
+        sum   (->> (lib/aggregable-columns query nil)
+                   (m/find-first (comp #{"sum"} :name)))
+        query (lib/aggregate query (lib/with-expression-name (lib/* 2 sum) "2*sum"))
+        legacy-query (lib.convert/->legacy-MBQL query)]
+    (is (= legacy-query
+           (-> legacy-query lib.convert/->pMBQL lib.convert/->legacy-MBQL)))))

test/metabase/lib/expression_test.cljc

@@ -609,6 +609,20 @@
         [:min [:offset {:lib/uuid (str (random-uuid))} [:field 42] 1]]            "Offset"
         [:offset {:lib/uuid (str (random-uuid))} [:sum [:cum-sum [:field 42]]] 1] "CumulativeSum"))))
 
+(deftest ^:parallel diagnose-expression-cyclic-aggregation-tests
+  (testing "self loop"
+    (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
+                    (lib/aggregate (lib/sum (meta/field-metadata :orders :total))))
+          sum (->> (lib/aggregable-columns query nil)
+                   (m/find-first (comp #{"sum"} :name)))
+          expr-name "2*sum"
+          query2 (lib/aggregate query (lib/with-expression-name (lib/* 2 sum) expr-name))
+          expr (->> (lib/aggregable-columns query2 nil)
+                    (m/find-first (comp #{expr-name} :name))
+                    (lib/* 2))]
+      (is (=? {:message (str "Cycle detected: " expr-name " → " expr-name)}
+              (lib.expression/diagnose-expression query2 0 :aggregation expr 1))))))
+
 (deftest ^:parallel date-and-time-string-literals-test-1-dates
   (are [types input] (= types (lib.schema.expression/type-of input))
     #{:type/Date :type/Text} "2024-07-02"))

test/metabase/lib/fe_util_test.cljc

@@ -850,6 +850,25 @@
         (lib.filter/> column 10)
         (lib.filter/and (lib.filter/is-null column) true)))))
 
+(deftest ^:parallel aggregation-ref-parts-test
+  (let [query (-> (lib/query meta/metadata-provider (meta/table-metadata :orders))
+                  (lib/aggregate (lib/sum (meta/field-metadata :orders :total))))
+        sum   (->> (lib/aggregable-columns query nil)
+                   (m/find-first (comp #{"sum"} :name)))
+        query (lib/aggregate query (lib/with-expression-name (lib/* 2 sum) "2*sum"))]
+    (is (=? {:lib/type :mbql/expression-parts,
+             :operator :*,
+             :options  {:name "2*sum", :display-name "2*sum"},
+             :args     [2
+                        {:lib/type :metadata/column
+                         :base-type :type/Float
+                         :name "sum"
+                         :display-name "Sum of Total"
+                         :effective-type :type/Float
+                         :lib/source :source/aggregations
+                         :lib/source-uuid string?}]}
+            (lib.fe-util/expression-parts query (second (lib/aggregations query)))))))
+
 (deftest ^:parallel join-condition-clause-test
   (let [lhs (lib/ref (meta/field-metadata :orders :product-id))
         rhs (lib/ref (meta/field-metadata :products :id))]

test/metabase/lib/remove_replace_test.cljc

@@ -1,6 +1,6 @@
 (ns metabase.lib.remove-replace-test
   (:require
-   #?@(:cljs ([metabase.test-runner.assert-exprs.approximately-equal]))
+   #?(:cljs [metabase.test-runner.assert-exprs.approximately-equal])
    [clojure.test :refer [are deftest is testing]]
    [medley.core :as m]
    [metabase.lib.convert :as lib.convert]
@@ -513,15 +513,19 @@
       (is (=? (map lib.options/ident aggregations)
               (map lib.options/ident replaced-aggregations))))
     (testing "replacing with dependent should cascade keeping valid parts"
-      (is (=? {:stages [{:aggregation [[:max {} [:field {} (meta/id :venues :price)]]
-                                       (second aggregations)]
-                         :expressions [[:aggregation {:lib/expression-name "expr"} string?]]}
-                        {:filters [[:= {} [:field {} "max"] 1]]}]}
-              (-> query
-                  (as-> <> (lib/expression <> "expr" (lib/aggregation-ref <> 0)))
-                  (lib/append-stage)
-                  (lib/filter (lib/= [:field {:lib/uuid (str (random-uuid)) :base-type :type/Integer} "sum"] 1))
-                  (lib/replace-clause 0 (first aggregations) (lib/max (meta/field-metadata :venues :price)))))))
+      (let [query' (-> query
+                       (as-> <> (lib/aggregate <> (lib/with-expression-name (lib/aggregation-ref <> 0) "expr")))
+                       (lib/append-stage)
+                       (lib/filter (lib/= [:field {:lib/uuid (str (random-uuid)) :base-type :type/Integer} "sum"] 1))
+                       (lib/replace-clause 0 (first aggregations) (lib/max (meta/field-metadata :venues :price))))
+            agg0-id (get-in query' [:stages 0 :aggregation 0 1 :lib/uuid])]
+        (is (=? {:stages [{:aggregation [[:max {:lib/uuid string?} [:field {} (meta/id :venues :price)]]
+                                         (second aggregations)
+                                         [:aggregation {:name "expr", :display-name "expr"} string?]]}
+                          {:filters [[:= {} [:field {} "max"] 1]]}]}
+                query'))
+        (is (string? agg0-id))
+        (is (= agg0-id (get-in query' [:stages 0 :aggregation 2 2])))))
     (testing "replacing with dependent should cascade removing invalid parts"
       (binding [lib.schema.expression/*suppress-expression-type-check?* false]
         (is (=? {:stages [{:aggregation [[:sum {} [:field {} (meta/id :products :id)]]

test/metabase/query_processor/middleware/expand_aggregations_test.clj

@@ -0,0 +1,101 @@
+(ns metabase.query-processor.middleware.expand-aggregations-test
+  (:require
+   [clojure.test :refer [deftest is testing]]
+   [medley.core :as m]
+   [metabase.lib-be.metadata.jvm :as lib.metadata.jvm]
+   [metabase.lib.convert :as lib.convert]
+   [metabase.lib.core :as lib]
+   [metabase.lib.metadata :as lib.metadata]
+   [metabase.lib.options :as lib.options]
+   [metabase.query-processor :as qp]
+   [metabase.test :as mt])
+  (:import (clojure.lang ExceptionInfo)))
+
+(deftest ^:parallel expand-aggregations-test
+  (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+        find-col (fn [query col-name]
+                   (->> (lib/aggregable-columns query nil)
+                        (m/find-first (comp #{col-name} :display-name))))
+        find-sum-of-total (fn [query]
+                            (find-col query "Sum of Total"))
+        query (as-> (lib/query mp (lib.metadata/table mp (mt/id :orders))) $
+                (lib/aggregate $ (lib/sum (lib.metadata/field mp (mt/id :orders :total))))
+                (lib/aggregate $ (lib/with-expression-name (lib/* 2 (find-sum-of-total $)) "2*sum")))]
+    (testing "simple reference"
+      (is (= {:columns ["sum" "2*sum"]
+              :rows [[1510617.7 3021235.4]]}
+             (mt/formatted-rows+column-names [2.0 2.0] (qp/process-query query)))))
+    (testing "multiple references"
+      (is (= {:columns ["sum" "2*sum" "3*sum"]
+              :rows [[1510617.7 3021235.4 4531853.1]]}
+             (let [query (lib/aggregate query (lib/with-expression-name (lib/* 3 (find-sum-of-total query)) "3*sum"))]
+               (mt/formatted-rows+column-names [2.0 2.0 2.0] (qp/process-query query))))))
+    (testing "multiple reference levels"
+      (is (= {:columns ["sum" "2*sum" "6*sum"]
+              :rows [[1510617.7 3021235.4 9063706.2]]}
+             (let [query (lib/aggregate query (lib/with-expression-name (lib/* 3 (find-col query "2*sum")) "6*sum"))]
+               (mt/formatted-rows+column-names [2.0 2.0 2.0] (qp/process-query query))))))
+    (testing "combined references"
+      (let [query (lib/aggregate query (lib/with-expression-name (lib/* 3 (find-col query "2*sum")) "6*sum"))
+            query (lib/aggregate query (lib/with-expression-name
+                                         (lib/+ (find-col query "2*sum") (find-col query "6*sum"))
+                                         "8*sum"))]
+        (is (= {:columns ["sum" "2*sum" "6*sum" "8*sum"]
+                :rows [[1510617.7 3021235.4 9063706.2 12084941.6]]}
+               (mt/formatted-rows+column-names [2.0 2.0 2.0 2.0] (qp/process-query query))))
+        (testing "cyclic definition"
+          (let [sum8-id (-> (get-in query [:stages 0 :aggregation 3])
+                            lib.options/uuid)
+                query (assoc-in query [:stages 0 :aggregation 1 3 2] sum8-id)]
+            (is (thrown-with-msg? ExceptionInfo #"cyclic aggregation definition"
+                                  (qp/process-query query)))))
+        (testing "dangling reference"
+          (let [dangling-ref (str (random-uuid))
+                query (assoc-in query [:stages 0 :aggregation 1 3 2] dangling-ref)]
+            ;; this does not reach the expand-aggregations middleware, because the schema check fails earlier
+            (is (thrown-with-msg? ExceptionInfo (re-pattern (str "no aggregation with uuid " dangling-ref))
+                                  (qp/process-query query)))))))
+    (testing "in card"
+      (mt/with-temp [:model/Card card {:dataset_query (lib.convert/->legacy-MBQL query)
+                                       :database_id (mt/id)
+                                       :name "Orders, Sum of Total and double Sum of Total"}]
+        (let [query (lib/query mp (lib.metadata/card mp (:id card)))]
+          (is (= {:columns ["sum" "2*sum"]
+                  :rows [[1510617.7 3021235.4]]}
+                 (mt/formatted-rows+column-names [2.0 2.0] (qp/process-query query)))))))))
+
+(deftest ^:parallel expand-aggregations-metric-test
+  (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+        find-col (fn [query col-name]
+                   (->> (lib/aggregable-columns query nil)
+                        (m/find-first (comp #{col-name} :display-name))))
+        metric-query (-> (lib/query mp (lib.metadata/table mp (mt/id :orders)))
+                         (lib/aggregate (lib/sum (lib.metadata/field mp (mt/id :orders :total)))))]
+    (testing "metrics can be referenced"
+      (mt/with-temp [:model/Card metric {:dataset_query (lib.convert/->legacy-MBQL metric-query)
+                                         :database_id (mt/id)
+                                         :name "Order Total"
+                                         :type :metric}]
+        (let [query (-> (lib/query mp (lib.metadata/table mp (mt/id :orders)))
+                        (lib/aggregate (lib.metadata/metric mp (:id metric))))
+              query (lib/aggregate query (lib/with-expression-name
+                                           (lib/* 2 (find-col query "Order Total"))
+                                           "2*total"))]
+          (is (= {:columns ["sum" "2*total"]
+                  :rows [[1510617.7 3021235.4]]}
+                 (mt/formatted-rows+column-names [2.0 2.0] (qp/process-query query)))))))))
+
+(deftest ^:parallel expand-aggregations-preserve-name-test
+  (let [mp (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+        find-col (fn [query col-name]
+                   (->> (lib/aggregable-columns query nil)
+                        (m/find-first (comp #{col-name} :display-name))))
+        query (as-> (lib/query mp (lib.metadata/table mp (mt/id :orders))) $
+                (lib/aggregate $ (lib/with-expression-name
+                                   (lib/sum (lib.metadata/field mp (mt/id :orders :total)))
+                                   "Custom Sum"))
+                (lib/aggregate $ (lib/with-expression-name (lib/ref (find-col $ "Custom Sum")) "Derived")))]
+    (testing "names are preserved"
+      (is (= {:columns ["Custom Sum" "Derived"]
+              :rows [[1510617.7 1510617.7]]}
+             (mt/formatted-rows+column-names [2.0 2.0] (qp/process-query query)))))))

test/metabase/query_processor/middleware/metrics_test.clj

@@ -1089,5 +1089,5 @@
   (testing "All available aggregations are tested for filter expansion in metric"
     (is (empty? (set/difference
                  (disj (descendants @lib.hierarchy/hierarchy :metabase.lib.schema.aggregation/aggregation-clause-tag)
-                       :metric :offset)
+                       :aggregation :metric :offset)
                  (set (map :operator tested-aggregations)))))))