Partially fix columns and viz settings mismatch (#42344)

From now we allow mismatch between `base-type` if we need to match columns and columnSettings

e2e/test/scenarios/question/settings.cy.spec.js

@@ -138,8 +138,21 @@ describe("scenarios > question > settings", () => {
                 "source-table": PRODUCTS_ID,
                 condition: [
                   "=",
-                  ["field-id", ORDERS.PRODUCT_ID],
-                  ["joined-field", "Products", ["field-id", PRODUCTS.ID]],
+                  [
+                    "field",
+                    ORDERS.PRODUCT_ID,
+                    {
+                      "base-type": "type/Integer",
+                    },
+                  ],
+                  [
+                    "field",
+                    PRODUCTS.ID,
+                    {
+                      "base-type": "type/BigInteger",
+                      "join-alias": "Products",
+                    },
+                  ],
                 ],
                 alias: "Products",
               },

e2e/test/scenarios/visualizations-tabular/reproductions/42049-wrong-columns-order.cy.spec.js

+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+import { restore, createQuestion } from "e2e/support/helpers";
+const { ORDERS, ORDERS_ID } = SAMPLE_DATABASE;
+
+// unskip once metabase#42049 is addressed
+describe.skip("issue 42049", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+  });
+
+  it("should not mess up columns order (metabase#42049)", () => {
+    cy.intercept("POST", "/api/card/*/query", req => {
+      req.on("response", res => {
+        const createdAt = res.body.data.cols[1];
+
+        createdAt.field_ref[1] = "created_at"; // simulate named field ref
+
+        res.send();
+      });
+    }).as("cardQuery");
+
+    createQuestion(
+      {
+        query: {
+          "source-table": ORDERS_ID,
+          fields: [
+            ["field", ORDERS.ID, { "base-type": "type/BigInteger" }],
+            ["field", ORDERS.CREATED_AT, { "base-type": "type/DateTime" }],
+            ["field", ORDERS.QUANTITY, { "base-type": "type/Integer" }],
+          ],
+        },
+        visualization_settings: {
+          "table.columns": [
+            {
+              name: "ID",
+              fieldRef: ["field", ORDERS.ID, null],
+              enabled: true,
+            },
+            {
+              name: "CREATED_AT",
+              fieldRef: [
+                "field",
+                ORDERS.CREATED_AT,
+                {
+                  "temporal-unit": "default",
+                },
+              ],
+              enabled: true,
+            },
+            {
+              name: "QUANTITY",
+              fieldRef: ["field", ORDERS.QUANTITY, null],
+              enabled: true,
+            },
+          ],
+        },
+      },
+      { visitQuestion: true },
+    );
+
+    cy.log("verify initial columns order");
+
+    cy.findAllByTestId("header-cell").as("headerCells");
+    cy.get("@headerCells").eq(0).should("have.text", "ID");
+    cy.get("@headerCells").eq(1).should("have.text", "Created At");
+    cy.get("@headerCells").eq(2).should("have.text", "Quantity");
+
+    cy.findByRole("button", { name: "Filter" }).click();
+
+    cy.findByRole("dialog").within(() => {
+      cy.findByRole("button", { name: "Last month" }).click();
+      cy.findByRole("button", { name: "Apply filters" }).click();
+    });
+
+    cy.wait("@cardQuery");
+    cy.get("@cardQuery.all").should("have.length", 2);
+
+    cy.log("verify columns order after applying the filter");
+
+    cy.findAllByTestId("header-cell").as("headerCells");
+    cy.get("@headerCells").eq(0).should("have.text", "ID");
+    cy.get("@headerCells").eq(1).should("have.text", "Created At");
+    cy.get("@headerCells").eq(2).should("have.text", "Quantity");
+  });
+});

frontend/src/metabase-lib/v1/queries/utils/dataset.ts

@@ -9,11 +9,14 @@ import type {
 export const datasetContainsNoResults = (data: DatasetData) =>
   data.rows == null || data.rows.length === 0;
 
-export function getColumnSettingKey({
-  key,
-  name,
-  fieldRef,
-}: TableColumnOrderSetting) {
+export function getColumnSettingKey(
+  { key, name, fieldRef }: TableColumnOrderSetting,
+  ignoreBaseType = false,
+) {
+  if (ignoreBaseType) {
+    return getColumnKey({ name, field_ref: normalize(fieldRef) }, true);
+  }
+
   return key ?? getColumnKey({ name, field_ref: normalize(fieldRef) });
 }
 
@@ -22,11 +25,11 @@ export function findColumnIndexesForColumnSettings(
   columnSettings: TableColumnOrderSetting[],
 ) {
   const columnIndexByKey = new Map(
-    columns.map((column, index) => [getColumnKey(column), index]),
+    columns.map((column, index) => [getColumnKey(column, true), index]),
   );
   return columnSettings.map(
     columnSetting =>
-      columnIndexByKey.get(getColumnSettingKey(columnSetting)) ?? -1,
+      columnIndexByKey.get(getColumnSettingKey(columnSetting, true)) ?? -1,
   );
 }
 
@@ -36,11 +39,11 @@ export function findColumnSettingIndexesForColumns(
 ) {
   const columnSettingIndexByKey = new Map(
     columnSettings.map((columnSetting, index) => [
-      getColumnSettingKey(columnSetting),
+      getColumnSettingKey(columnSetting, true),
       index,
     ]),
   );
   return columns.map(
-    column => columnSettingIndexByKey.get(getColumnKey(column)) ?? -1,
+    column => columnSettingIndexByKey.get(getColumnKey(column, true)) ?? -1,
   );
 }

frontend/src/metabase-lib/v1/queries/utils/dataset.unit.spec.ts

+import {
+  createMockColumn,
+  createMockTableColumnOrderSetting,
+} from "metabase-types/api/mocks";
+import { ORDERS } from "metabase-types/api/mocks/presets";
+
+import {
+  findColumnIndexesForColumnSettings,
+  findColumnSettingIndexesForColumns,
+} from "./dataset";
+
+describe("dataset utils", () => {
+  describe("findColumnIndexesForColumnSettings", () => {
+    it("finds columnIndex for ColumnSettings without base-type", () => {
+      const column = createMockColumn({
+        id: ORDERS.TOTAL,
+        name: "TOTAL",
+        display_name: "Total",
+        field_ref: [
+          "field",
+          ORDERS.TOTAL,
+          {
+            "base-type": "type/Number",
+          },
+        ],
+      });
+      const columnSetting = createMockTableColumnOrderSetting({
+        name: "TOTAL",
+        key: `["ref",["field",${ORDERS.TOTAL},null]]`,
+        fieldRef: ["field", ORDERS.TOTAL, null],
+        enabled: true,
+      });
+
+      expect(
+        findColumnIndexesForColumnSettings([column], [columnSetting]),
+      ).toEqual([0]);
+    });
+
+    it("finds columnIndex for ColumnSettings with base-type", () => {
+      const column = createMockColumn({
+        id: ORDERS.TOTAL,
+        name: "TOTAL",
+        display_name: "Total",
+        field_ref: [
+          "field",
+          ORDERS.TOTAL,
+          {
+            "base-type": "type/Number",
+          },
+        ],
+      });
+      const columnSetting = createMockTableColumnOrderSetting({
+        name: "TOTAL",
+        key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+        fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+        enabled: true,
+      });
+
+      expect(
+        findColumnIndexesForColumnSettings([column], [columnSetting]),
+      ).toEqual([0]);
+    });
+
+    it("finds findColumnIndexesForColumnSettings for Columns with different base-type", () => {
+      const column = createMockColumn({
+        id: ORDERS.TOTAL,
+        name: "TOTAL",
+        display_name: "Total",
+        field_ref: [
+          "field",
+          ORDERS.TOTAL,
+          {
+            "base-type": "type/Text",
+          },
+        ],
+      });
+      const columnSetting = createMockTableColumnOrderSetting({
+        name: "TOTAL",
+        key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+        fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+        enabled: true,
+      });
+
+      expect(
+        findColumnIndexesForColumnSettings([column], [columnSetting]),
+      ).toEqual([0]);
+    });
+  });
+
+  describe("findColumnSettingIndexesForColumns", () => {
+    it("finds columnSettingsIndex for Column without base-type", () => {
+      const column = createMockColumn({
+        id: ORDERS.TOTAL,
+        name: "TOTAL",
+        display_name: "Total",
+        field_ref: ["field", ORDERS.TOTAL, null],
+      });
+      const columnSetting = createMockTableColumnOrderSetting({
+        name: "TOTAL",
+        key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+        fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+        enabled: true,
+      });
+
+      expect(
+        findColumnSettingIndexesForColumns([column], [columnSetting]),
+      ).toEqual([0]);
+    });
+
+    it("finds columnSettingsIndex for Columns with base-type", () => {
+      const column = createMockColumn({
+        id: ORDERS.TOTAL,
+        name: "TOTAL",
+        display_name: "Total",
+        field_ref: [
+          "field",
+          ORDERS.TOTAL,
+          {
+            "base-type": "type/Number",
+          },
+        ],
+      });
+      const columnSetting = createMockTableColumnOrderSetting({
+        name: "TOTAL",
+        key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+        fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+        enabled: true,
+      });
+
+      expect(
+        findColumnSettingIndexesForColumns([column], [columnSetting]),
+      ).toEqual([0]);
+    });
+
+    it("finds columnSettingsIndex for Columns with different base-type", () => {
+      const column = createMockColumn({
+        id: ORDERS.TOTAL,
+        name: "TOTAL",
+        display_name: "Total",
+        field_ref: [
+          "field",
+          ORDERS.TOTAL,
+          {
+            "base-type": "type/Text",
+          },
+        ],
+      });
+      const columnSetting = createMockTableColumnOrderSetting({
+        name: "TOTAL",
+        key: `["ref",["field",${ORDERS.TOTAL},{"base-type":"type/Number"}]]`,
+        fieldRef: ["field", ORDERS.TOTAL, { "base-type": "type/Number" }],
+        enabled: true,
+      });
+
+      expect(
+        findColumnSettingIndexesForColumns([column], [columnSetting]),
+      ).toEqual([0]);
+    });
+  });
+});

frontend/src/metabase-lib/v1/queries/utils/get-column-key.ts

@@ -12,6 +12,7 @@ import type { DatasetColumn } from "metabase-types/api";
 
 export const getColumnKey = (
   column: Pick<DatasetColumn, "name" | "field_ref">,
+  ignoreBaseType = false,
 ) => {
   let fieldRef = column.field_ref;
 
@@ -31,7 +32,7 @@ export const getColumnKey = (
     isExpressionReference(fieldRef) ||
     isAggregationReference(fieldRef)
   ) {
-    fieldRef = getBaseDimensionReference(fieldRef);
+    fieldRef = getBaseDimensionReference(fieldRef, ignoreBaseType);
   }
 
   const isLegacyRef =

frontend/src/metabase-lib/v1/references.ts

@@ -111,11 +111,12 @@ export const BASE_DIMENSION_REFERENCE_OMIT_OPTIONS = [
 
 export const getBaseDimensionReference = (
   mbql: DimensionReferenceWithOptions,
+  ignoreBaseType: boolean,
 ) =>
-  getDimensionReferenceWithoutOptions(
-    mbql,
-    BASE_DIMENSION_REFERENCE_OMIT_OPTIONS,
-  );
+  getDimensionReferenceWithoutOptions(mbql, [
+    ...BASE_DIMENSION_REFERENCE_OMIT_OPTIONS,
+    ...(ignoreBaseType ? ["base-type"] : []),
+  ]);
 
 /**
  * Whether this Field clause has a string Field name (as opposed to an integer Field ID). This generally means the

frontend/src/metabase-lib/viz/column_settings.ts

@@ -87,6 +87,7 @@ function syncTableColumnSettings(
     fieldRef: col.field_ref,
     enabled: true,
   }));
+
   return {
     ...settings,
     "table.columns": [...existingColumnSettings, ...addedColumnSettings],