Determine `pivot-cols` and `pivot-rows` on the backend using viz settings (#34583)

e2e/test/scenarios/visualizations/pivot_tables.cy.spec.js

@@ -9,6 +9,7 @@ import {
   dragField,
   leftSidebar,
   main,
+  modal,
 } from "e2e/support/helpers";
 
 import { SAMPLE_DB_ID } from "e2e/support/cypress_data";
@@ -22,6 +23,8 @@ const {
   PEOPLE,
   REVIEWS,
   REVIEWS_ID,
+  ANALYTIC_EVENTS,
+  ANALYTIC_EVENTS_ID,
 } = SAMPLE_DATABASE;
 
 const QUESTION_NAME = "Cypress Pivot Table";
@@ -57,6 +60,7 @@ describe("scenarios > visualizations > pivot tables", { tags: "@slow" }, () => {
   beforeEach(() => {
     restore();
     cy.signInAsAdmin();
+    cy.intercept("POST", "/api/card").as("createCard");
   });
 
   it("should be created from an ad-hoc question", () => {
@@ -1098,6 +1102,73 @@ describe("scenarios > visualizations > pivot tables", { tags: "@slow" }, () => {
 
     main().findByText("User → Address").should("be.visible");
   });
+
+  it("should return the same number of rows when running as an ad-hoc query vs a saved card (metabase#34278)", () => {
+    const query = {
+      type: "query",
+      query: {
+        "source-table": ANALYTIC_EVENTS_ID,
+        aggregation: [["count"]],
+        breakout: [
+          ["field", ANALYTIC_EVENTS.BUTTON_LABEL, { "base-type": "type/Text" }],
+          ["field", ANALYTIC_EVENTS.PAGE_URL, { "base-type": "type/Text" }],
+          [
+            "field",
+            ANALYTIC_EVENTS.TIMESTAMP,
+            { "base-type": "type/DateTime", "temporal-unit": "day" },
+          ],
+          ["field", ANALYTIC_EVENTS.EVENT, { "base-type": "type/Text" }],
+          ["field", ANALYTIC_EVENTS.ACCOUNT_ID, { "base-type": "type/Text" }],
+          ["field", ANALYTIC_EVENTS.ID, { "base-type": "type/Text" }],
+        ],
+      },
+      database: SAMPLE_DB_ID,
+    };
+
+    visitQuestionAdhoc({
+      dataset_query: query,
+      display: "pivot",
+      visualization_settings: {
+        "pivot_table.column_split": {
+          rows: [
+            ["field", ANALYTIC_EVENTS.PAGE_URL, { "base-type": "type/Text" }],
+            [
+              "field",
+              ANALYTIC_EVENTS.BUTTON_LABEL,
+              { "base-type": "type/Text" },
+            ],
+            ["field", ANALYTIC_EVENTS.ACCOUNT_ID, { "base-type": "type/Text" }],
+            [
+              "field",
+              ANALYTIC_EVENTS.TIMESTAMP,
+              { "base-type": "type/DateTime", "temporal-unit": "day" },
+            ],
+            ["field", ANALYTIC_EVENTS.ID, { "base-type": "type/Text" }],
+          ],
+          columns: [
+            ["field", ANALYTIC_EVENTS.EVENT, { "base-type": "type/Text" }],
+          ],
+          values: [["aggregation", 0]],
+        },
+      },
+    });
+    cy.wait("@pivotDataset");
+
+    cy.findByTestId("question-row-count").should(
+      "have.text",
+      "Showing first 52,711 rows",
+    );
+
+    cy.findByTestId("qb-header-action-panel").findByText("Save").click();
+    modal().button("Save").click();
+    cy.wait("@createCard");
+    cy.reload();
+
+    cy.findByTestId("question-row-count").should(
+      "have.text",
+      "Showing first 52,711 rows",
+    );
+  });
 });
 
 const testQuery = {

src/metabase/api/dataset.clj

@@ -182,7 +182,11 @@
   (let [info {:executed-by api/*current-user-id*
               :context     :ad-hoc}]
     (qp.streaming/streaming-response [context :api]
-      (qp.pivot/run-pivot-query (assoc query :async? true) info context))))
+      (qp.pivot/run-pivot-query (assoc query
+                                       :async? true
+                                       :constraints (qp.constraints/default-query-constraints))
+                                info
+                                context))))
 
 (defn- parameter-field-values
   [field-ids query]

src/metabase/query_processor/card.clj

@@ -203,19 +203,20 @@
                   (^:once fn* [query info]
                    (qp.streaming/streaming-response [context export-format (u/slugify (:card-name info))]
                                                     (qp-runner query info context))))
-        card  (api/read-check (t2/select-one [Card :id :name :dataset_query :database_id
-                                              :cache_ttl :collection_id :dataset :result_metadata]
+        card  (api/read-check (t2/select-one [Card :id :name :dataset_query :database_id :cache_ttl :collection_id
+                                              :dataset :result_metadata :visualization_settings]
                                              :id card-id))
         query (-> (assoc (query-for-card card parameters constraints middleware {:dashboard-id dashboard-id}) :async? true)
                   (update :middleware (fn [middleware]
                                         (merge
                                          {:js-int-to-string? true :ignore-cached-results? ignore_cache}
                                          middleware))))
-        info  (cond-> {:executed-by  api/*current-user-id*
-                       :context      context
-                       :card-id      card-id
-                       :card-name    (:name card)
-                       :dashboard-id dashboard-id}
+        info  (cond-> {:executed-by            api/*current-user-id*
+                       :context                context
+                       :card-id                card-id
+                       :card-name              (:name card)
+                       :dashboard-id           dashboard-id
+                       :visualization-settings (:visualization_settings card)}
                 (and (:dataset card) (seq (:result_metadata card)))
                 (assoc :metadata/dataset-metadata (:result_metadata card)))]
     (api/check-not-archived card)

src/metabase/query_processor/pivot.clj

@@ -11,6 +11,7 @@
    [metabase.query-processor.middleware.resolve-database-and-driver
     :as qp.resolve-database-and-driver]
    [metabase.query-processor.store :as qp.store]
+   [metabase.query-processor.util :as qp.util]
    [metabase.util :as u]
    [metabase.util.i18n :refer [trs tru]]
    [metabase.util.log :as log]))
@@ -121,7 +122,7 @@
 
 (defn- generate-queries
   "Generate the additional queries to perform a generic pivot table"
-  [{{all-breakouts :breakout} :query, :keys [pivot-rows pivot-cols query], :as outer-query}]
+  [{{all-breakouts :breakout} :query, :keys [query], :as outer-query} {:keys [pivot-rows pivot-cols]}]
   (try
     (for [breakout-indices (u/prog1 (breakout-combinations (count all-breakouts) pivot-rows pivot-cols)
                              (log/tracef "Using breakout combinations: %s" (pr-str <>)))
@@ -211,6 +212,20 @@
       (qp/process-query-and-save-with-max-results-constraints! first-query info context)
       (qp/process-query (dissoc first-query :info) context))))
 
+(defn pivot-options
+  "Given a pivot table query and a card ID, looks at the `pivot_table.column_split` key in the card's visualization
+  settings and generates pivot-rows and pivot-cols to use for generating subqueries."
+  [query viz-settings]
+  (let [column-split      (:pivot_table.column_split viz-settings)
+        breakout          (mapv qp.util/field-ref->key
+                                (-> query :query :breakout))
+        index-in-breakout (fn [field-ref]
+                            (.indexOf ^clojure.lang.PersistentVector breakout (qp.util/field-ref->key field-ref)))
+        pivot-rows        (mapv index-in-breakout (:rows column-split))
+        pivot-cols        (mapv index-in-breakout (:columns column-split))]
+    {:pivot-rows pivot-rows
+     :pivot-cols pivot-cols}))
+
 (defn run-pivot-query
   "Run the pivot query. Unlike many query execution functions, this takes `context` as the first parameter to support
    its application via `partial`.
@@ -225,10 +240,13 @@
      (qp.store/with-metadata-provider (qp.resolve-database-and-driver/resolve-database-id query)
        (let [context                 (merge (context.default/default-context) context)
              query                   (mbql.normalize/normalize query)
+             pivot-options           (or
+                                      (not-empty (select-keys query [:pivot-rows :pivot-cols]))
+                                      (pivot-options query (get info :visualization-settings)))
              main-breakout           (:breakout (:query query))
              col-determination-query (add-grouping-field query main-breakout 0)
              all-expected-cols       (qp/query->expected-cols col-determination-query)
-             all-queries             (generate-queries query)]
+             all-queries             (generate-queries query pivot-options)]
          (process-multiple-queries
           all-queries
           info

test/metabase/api/pivots.clj

@@ -11,44 +11,69 @@
         ;; Disable on Redshift due to OutOfMemory issue (see #18834)
         :redshift))
 
+(def pivot-query-options
+  "Pivot rows and columns for `pivot-query`"
+  {:pivot_rows [1 0]
+   :pivot_cols [2]})
+
 (defn pivot-query
   "A basic pivot table query"
-  []
-  (mt/dataset sample-dataset
-    (-> (mt/mbql-query orders
-                       {:aggregation [[:count] [:sum $orders.quantity]]
-                        :breakout    [$orders.user_id->people.state
-                                      $orders.user_id->people.source
-                                      $orders.product_id->products.category]})
-        (assoc :pivot_rows [1 0]
-               :pivot_cols [2]))))
+  ([]
+   (pivot-query true))
+
+  ([include-pivot-options?]
+   (mt/dataset sample-dataset
+     (merge
+      (mt/mbql-query orders
+                     {:aggregation [[:count] [:sum $orders.quantity]]
+                      :breakout    [$orders.user_id->people.state
+                                    $orders.user_id->people.source
+                                    $orders.product_id->products.category]})
+      (when include-pivot-options?
+        pivot-query-options)))))
 
 (defn filters-query
   "A pivot table query with a filter applied"
-  []
-  (-> (mt/mbql-query orders
-        {:aggregation [[:count]]
-         :breakout    [$orders.user_id->people.state
-                       $orders.user_id->people.source]
-         :filter      [:and [:= $orders.user_id->people.source "Google" "Organic"]]})
-      (assoc :pivot_rows [0]
-             :pivot_cols [1])))
+  ([]
+   (filters-query true))
+
+  ([include-pivot-options?]
+   (merge
+    (mt/mbql-query orders
+                   {:aggregation [[:count]]
+                    :breakout    [$orders.user_id->people.state
+                                  $orders.user_id->people.source]
+                    :filter      [:and [:= $orders.user_id->people.source "Google" "Organic"]]})
+    (when include-pivot-options?
+      {:pivot_rows [0]
+       :pivot_cols [1]}))))
 
 (defn parameters-query
   "A pivot table query with parameters"
-  []
-  (-> (mt/mbql-query orders
-        {:aggregation [[:count]]
-         :breakout    [$orders.user_id->people.state
-                       $orders.user_id->people.source]
-         :filter      [:and [:= $orders.user_id->people.source "Google" "Organic"]]
-         :parameters  [{:type   "category"
-                        :target [:dimension $orders.product_id->products.category]
-                        :value  "Gadget"}]})
-      (assoc :pivot_rows [0]
-             :pivot_cols [1])))
+  ([]
+   (parameters-query true))
+
+  ([include-pivot-options?]
+   (merge
+    (mt/mbql-query orders
+       {:aggregation [[:count]]
+        :breakout    [$orders.user_id->people.state
+                      $orders.user_id->people.source]
+        :filter      [:and [:= $orders.user_id->people.source "Google" "Organic"]]
+        :parameters  [{:type   "category"
+                       :target [:dimension $orders.product_id->products.category]
+                       :value  "Gadget"}]})
+    (when include-pivot-options?
+      {:pivot_rows [0]
+       :pivot_cols [1]}))))
 
 (defn pivot-card
   "A dashboard card query with a pivot table"
   []
-  {:dataset_query (pivot-query)})
+  (let [pivot-query (pivot-query false)
+        breakout    (-> pivot-query :query :breakout)]
+    {:dataset_query pivot-query
+     :visualization_settings
+     {:pivot_table.column_split
+      {:rows    [(get breakout 1) (get breakout 0)]
+       :columns [(get breakout 2)]}}}))

test/metabase/query_processor/pivot_test.clj

@@ -153,10 +153,16 @@
                                       :pivot-cols [2])
                                (assoc-in [:query :aggregation] [[:count] [:sum (mt/$ids $orders.quantity)]])
                                (assoc-in [:query :source-table] (mt/$ids $$orders))))
-                actual   (map (fn [actual-val] (dissoc actual-val :database)) (#'qp.pivot/generate-queries request))]
+                actual   (map (fn [actual-val] (dissoc actual-val :database))
+                              (#'qp.pivot/generate-queries request {:pivot-rows [1 0] :pivot-cols [2]}))]
             (is (= 6 (count actual)))
             (is (= expected actual))))))))
 
+(deftest pivot-options-test
+  (testing "`pivot-options` correctly generates pivot-rows and pivot-cols from a card's viz settings"
+    (is (= {:pivot-rows [1 0] :pivot-cols [2]}
+           (qp.pivot/pivot-options (api.pivots/pivot-query false) (:visualization_settings (api.pivots/pivot-card)))))))
+
 (deftest dont-return-too-many-rows-test
   (testing "Make sure pivot queries don't return too many rows (#14329)"
     (let [results (qp.pivot/run-pivot-query (test-query))