Merge pull request #2442 from metabase/fix-pivot-with-nulls

Fix broken pivot table due to null dimension values

frontend/src/card/card.controllers.js

@@ -16,7 +16,7 @@ import i from "icepick";
 import moment from "moment";
 
 import MetabaseAnalytics from "metabase/lib/analytics";
-import DataGrid from "metabase/lib/data_grid";
+import * as DataGrid from "metabase/lib/data_grid";
 import Query from "metabase/lib/query";
 import { createQuery } from "metabase/lib/query";
 import { createCard, serializeCardForUrl, deserializeCardFromUrl, cleanCopyCard, urlForCardState } from "metabase/lib/card";

frontend/src/lib/data_grid.js

@@ -7,114 +7,108 @@ function compareNumbers(a, b) {
     return a - b;
 }
 
-
-var DataGrid = {
-    filterOnPreviewDisplay: function(data) {
-        // find any columns where visibility_type = details-only
-        var hiddenColumnIdxs = _.map(data.cols, function(col, idx) { if(col.visibility_type === "details-only") return idx; });
-        hiddenColumnIdxs = _.filter(hiddenColumnIdxs, function(val) { return val !== undefined; });
-
-        // filter out our data grid using the indexes of the hidden columns
-        var filteredRows = data.rows.map(function(row, rowIdx) {
-            return row.filter(function(cell, cellIdx) {
-                if (_.contains(hiddenColumnIdxs, cellIdx)) {
-                    return false;
-                } else {
-                    return true;
-                }
-            });
+export function filterOnPreviewDisplay(data) {
+    // find any columns where visibility_type = details-only
+    var hiddenColumnIdxs = _.map(data.cols, function(col, idx) { if(col.visibility_type === "details-only") return idx; });
+    hiddenColumnIdxs = _.filter(hiddenColumnIdxs, function(val) { return val !== undefined; });
+
+    // filter out our data grid using the indexes of the hidden columns
+    var filteredRows = data.rows.map(function(row, rowIdx) {
+        return row.filter(function(cell, cellIdx) {
+            if (_.contains(hiddenColumnIdxs, cellIdx)) {
+                return false;
+            } else {
+                return true;
+            }
         });
+    });
+
+    return {
+        cols: _.filter(data.cols, function(col) { return col.visibility_type !== "details-only"; }),
+        columns: _.map(data.cols, function(col) { return col.display_name; }),
+        rows: filteredRows,
+        rows_truncated: data.rows_truncated
+    };
+}
 
-        return {
-            cols: _.filter(data.cols, function(col) { return col.visibility_type !== "details-only"; }),
-            columns: _.map(data.cols, function(col) { return col.display_name; }),
-            rows: filteredRows,
-            rows_truncated: data.rows_truncated
-        };
-    },
-
-    pivot: function(data) {
-        // find the lowest cardinality dimension and make it our "pivoted" column
-        // TODO: we assume dimensions are in the first 2 columns, which is less than ideal
-        var pivotCol = 0,
-            normalCol = 1,
-            cellCol = 2,
-            pivotColValues = DataGrid.distinctValues(data, pivotCol),
-            normalColValues = DataGrid.distinctValues(data, normalCol);
-        if (normalColValues.length <= pivotColValues.length) {
-            pivotCol = 1;
-            normalCol = 0;
-
-            var tmp = pivotColValues;
-            pivotColValues = normalColValues;
-            normalColValues = tmp;
-        }
-
-        // sort the column values sensibly
-        if (SchemaMetadata.isNumeric(data.cols[pivotCol])) {
-            pivotColValues.sort(compareNumbers);
-        } else {
-            pivotColValues.sort();
-        }
-
-        if (SchemaMetadata.isNumeric(data.cols[normalCol])) {
-            normalColValues.sort(compareNumbers);
-        } else {
-            normalColValues.sort();
-        }
+export function pivot(data) {
+    // find the lowest cardinality dimension and make it our "pivoted" column
+    // TODO: we assume dimensions are in the first 2 columns, which is less than ideal
+    var pivotCol = 0,
+        normalCol = 1,
+        cellCol = 2,
+        pivotColValues = distinctValues(data, pivotCol),
+        normalColValues = distinctValues(data, normalCol);
+    if (normalColValues.length <= pivotColValues.length) {
+        pivotCol = 1;
+        normalCol = 0;
+
+        var tmp = pivotColValues;
+        pivotColValues = normalColValues;
+        normalColValues = tmp;
+    }
 
+    // sort the column values sensibly
+    if (SchemaMetadata.isNumeric(data.cols[pivotCol])) {
+        pivotColValues.sort(compareNumbers);
+    } else {
+        pivotColValues.sort();
+    }
 
-        // make sure that the first element in the pivoted column list is null which makes room for the label of the other column
-        pivotColValues.unshift(data.cols[normalCol].display_name);
+    if (SchemaMetadata.isNumeric(data.cols[normalCol])) {
+        normalColValues.sort(compareNumbers);
+    } else {
+        normalColValues.sort();
+    }
 
-        // start with an empty grid that we'll fill with the appropriate values
-        var pivotedRows = [];
-        var emptyRow = Array.apply(null, Array(pivotColValues.length)).map(function() { return null; });
-        for (var i=0; i < normalColValues.length; i++) {
-            pivotedRows.push(_.clone(emptyRow));
-        }
 
-        // fill it up with the data
-        for (var j=0; j < data.rows.length; j++) {
-            var normalColIdx = normalColValues.lastIndexOf(data.rows[j][normalCol]);
-            var pivotColIdx = pivotColValues.lastIndexOf(data.rows[j][pivotCol]);
+    // make sure that the first element in the pivoted column list is null which makes room for the label of the other column
+    pivotColValues.unshift(data.cols[normalCol].display_name);
 
-            pivotedRows[normalColIdx][0] = data.rows[j][normalCol];
-            // NOTE: we are hard coding the expectation that the metric is in the 3rd column
-            pivotedRows[normalColIdx][pivotColIdx] = data.rows[j][2];
-        }
+    // start with an empty grid that we'll fill with the appropriate values
+    var pivotedRows = [];
+    var emptyRow = Array.apply(null, Array(pivotColValues.length)).map(function() { return null; });
+    for (var i=0; i < normalColValues.length; i++) {
+        pivotedRows.push(_.clone(emptyRow));
+    }
 
-        // provide some column metadata to maintain consistency
-        var cols = pivotColValues.map(function(val, idx) {
-            if (idx === 0) {
-                // first column is always the coldef of the normal column
-                return data.cols[normalCol];
-            }
+    // fill it up with the data
+    for (var j=0; j < data.rows.length; j++) {
+        var normalColIdx = normalColValues.lastIndexOf(data.rows[j][normalCol]);
+        var pivotColIdx = pivotColValues.lastIndexOf(data.rows[j][pivotCol]);
 
-            var colDef = _.clone(data.cols[cellCol]);
-            colDef['name'] = colDef['display_name'] = formatValue(val, { column: data.cols[pivotCol] });
-            return colDef;
-        });
+        pivotedRows[normalColIdx][0] = data.rows[j][normalCol];
+        // NOTE: we are hard coding the expectation that the metric is in the 3rd column
+        pivotedRows[normalColIdx][pivotColIdx] = data.rows[j][2];
+    }
 
-        return {
-            cols: cols,
-            columns: pivotColValues,
-            rows: pivotedRows
-        };
-    },
+    // provide some column metadata to maintain consistency
+    var cols = pivotColValues.map(function(val, idx) {
+        if (idx === 0) {
+            // first column is always the coldef of the normal column
+            return data.cols[normalCol];
+        }
 
-    distinctValues: function(data, colIdx) {
-        var vals = data.rows.map(function(r) {
-            return r[colIdx];
-        });
+        var colDef = _.clone(data.cols[cellCol]);
+        colDef['name'] = colDef['display_name'] = formatValue(val, { column: data.cols[pivotCol] }) || "";
+        return colDef;
+    });
 
-        return vals.filter(function(v, i) { return i==vals.lastIndexOf(v); });
-    },
+    return {
+        cols: cols,
+        columns: pivotColValues,
+        rows: pivotedRows
+    };
+}
 
-    cardinality: function(data, colIdx) {
-        return DataGrid.distinctValues(data, colIdx).length;
-    }
-};
+export function distinctValues(data, colIdx) {
+    var vals = data.rows.map(function(r) {
+        return r[colIdx];
+    });
 
+    return vals.filter(function(v, i) { return i==vals.lastIndexOf(v); });
+}
 
-export default DataGrid;
+export function cardinality(data, colIdx) {
+    return distinctValues(data, colIdx).length;
+}

frontend/src/visualizations/Table.jsx

@@ -4,7 +4,7 @@ import TableInteractive from "./TableInteractive.jsx";
 import TableSimple from "./TableSimple.jsx";
 
 import Query from "metabase/lib/query";
-import DataGrid from "metabase/lib/data_grid";
+import * as DataGrid from "metabase/lib/data_grid";
 
 export default class Bar extends Component {
     static displayName = "Table";

frontend/src/visualizations/TableInteractive.jsx

@@ -217,8 +217,8 @@ export default class TableInteractive extends Component {
 
     tableHeaderRenderer(columnIndex) {
         var column = this.props.data.cols[columnIndex],
-            colVal = (column && column.display_name && column.display_name.toString()) ||
-                     (column && column.name && column.name.toString());
+            colVal = (column && column.display_name && String(column.display_name)) ||
+                     (column && column.name && String(column.name)) || "";
 
         if (column.unit && column.unit !== "default") {
             colVal += ": " + capitalize(column.unit.replace(/-/g, " "))
@@ -265,7 +265,8 @@ export default class TableInteractive extends Component {
         }
 
         var tableColumns = this.props.data.cols.map((column, idx) => {
-            var colVal = (column !== null) ? column.name.toString() : null;
+            var colVal = (column && column.display_name && String(column.display_name)) ||
+                         (column && column.name && String(column.name)) || "";
             var colWidth = this.state.columnWidths[idx];
 
             if (!colWidth) {

frontend/test/unit/lib/data_grid.spec.js

+/*eslint-env jasmine */
+
+import { pivot } from "metabase/lib/data_grid";
+
+function makeData(rows) {
+    return {
+        rows: rows,
+        cols: [
+            { name: "D1", display_name: "Dimension 1", base_type: "TextField" },
+            { name: "D2", display_name: "Dimension 2", base_type: "TextField" },
+            { name: "M",  display_name: "Metric",      base_type: "IntegerField" }
+        ]
+    }
+}
+
+describe("data_grid", () => {
+    describe("pivot", () => {
+
+        it("should pivot values correctly", () => {
+            let data = makeData([
+                ["a", "x", 1],
+                ["a", "y", 2],
+                ["a", "z", 3],
+                ["b", "x", 4],
+                ["b", "y", 5],
+                ["b", "z", 6]
+            ])
+            let pivotedData = pivot(data);
+            expect(pivotedData.cols.length).toEqual(3);
+            expect(pivotedData.rows).toEqual([
+                ["x", 1, 4],
+                ["y", 2, 5],
+                ["z", 3, 6]
+            ]);
+        })
+
+        it("should not return null column names from null values", () => {
+            let data = makeData([
+                [null, null, 1]
+            ]);
+            let pivotedData = pivot(data);
+            expect(pivotedData.rows.length).toEqual(1);
+            expect(pivotedData.cols.length).toEqual(2);
+            expect(pivotedData.cols[0].name).toEqual(jasmine.any(String));
+            expect(pivotedData.cols[0].display_name).toEqual(jasmine.any(String));
+            expect(pivotedData.cols[1].name).toEqual(jasmine.any(String));
+            expect(pivotedData.cols[1].display_name).toEqual(jasmine.any(String));
+        })
+    })
+})