Use row index to sort x values (#10675)

frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js

@@ -12,7 +12,6 @@ import {
   computeSplit,
   computeMaxDecimalsForValues,
   getFriendlyName,
-  getXValues,
   colorShades,
 } from "./utils";
 
@@ -36,7 +35,6 @@ import { NULL_DIMENSION_WARNING, unaggregatedDataWarning } from "./warnings";
 import { keyForSingleSeries } from "metabase/visualizations/lib/settings/series";
 
 import {
-  HACK_parseTimestamp,
   forceSortedGroupsOfGroups,
   initChart, // TODO - probably better named something like `initChartParent`
   makeIndexMap,
@@ -48,9 +46,10 @@ import {
   isHistogramBar,
   isStacked,
   isNormalized,
+  getDatas,
   getFirstNonEmptySeries,
+  getXValues,
   isDimensionTimeseries,
-  isDimensionNumeric,
   isRemappedToString,
   isMultiCardSeries,
 } from "./renderer_utils";
@@ -94,25 +93,6 @@ function checkSeriesIsValid({ series, maxSeries }) {
   }
 }
 
-function getDatas({ settings, series }, warn) {
-  return series.map(s =>
-    s.data.rows.map(row => {
-      const newRow = [
-        // don't parse as timestamp if we're going to display as a quantitative scale, e.x. years and Unix timestamps
-        isDimensionTimeseries(series) && !isQuantitative(settings)
-          ? HACK_parseTimestamp(row[0], s.data.cols[0].unit, warn)
-          : isDimensionNumeric(series)
-          ? row[0]
-          : String(row[0]),
-        ...row.slice(1),
-      ];
-      // $FlowFixMe: _origin not typed
-      newRow._origin = row._origin;
-      return newRow;
-    }),
-  );
-}
-
 function getXInterval({ settings, series }, xValues) {
   if (isTimeseries(settings)) {
     // compute the interval
@@ -133,7 +113,7 @@ function getXInterval({ settings, series }, xValues) {
 }
 
 function getXAxisProps(props, datas) {
-  const rawXValues = getXValues(datas);
+  const rawXValues = getXValues(props);
   const isHistogram = isHistogramBar(props);
   const xInterval = getXInterval(props, rawXValues);
 

frontend/src/metabase/visualizations/lib/renderer_utils.js

@@ -94,6 +94,82 @@ export function reduceGroup(group, key, warnUnaggregated) {
   );
 }
 
+export function parseXValue(xValue, options, warn) {
+  const { parsedValue, warning } = memoizedParseXValue(xValue, options);
+  if (warning !== undefined) {
+    warn(warning);
+  }
+  return parsedValue;
+}
+
+const memoizedParseXValue = _.memoize(
+  (xValue, { isNumeric, isTimeseries, isQuantitative, unit }) => {
+    // don't parse as timestamp if we're going to display as a quantitative
+    // scale, e.x. years and Unix timestamps
+    if (isTimeseries && !isQuantitative) {
+      return parseTimestampAndWarn(xValue, unit);
+    }
+    const parsedValue = isNumeric ? xValue : String(xValue);
+    return { parsedValue };
+  },
+  // create cache key from args
+  // we need typeof so "2" and 2 don't have the same cache key
+  (x, options) => [x, typeof x, ...Object.values(options)].join(),
+);
+
+function getParseOptions({ settings, data }) {
+  return {
+    isNumeric: dimensionIsNumeric(data),
+    isTimeseries: dimensionIsTimeseries(data),
+    isQuantitative: isQuantitative(settings),
+    unit: data.cols[0].unit,
+  };
+}
+
+export function getDatas({ settings, series }, warn) {
+  return series.map(({ data }) => {
+    const parseOptions = getParseOptions({ settings, data });
+    return data.rows.map(row => {
+      const [x, ...rest] = row;
+      const newRow = [parseXValue(x, parseOptions, warn), ...rest];
+      newRow._origin = row._origin;
+      return newRow;
+    });
+  });
+}
+
+export function getXValues({ settings, series }) {
+  // if _.raw isn't set then we already have the raw series
+  const { _raw: rawSeries = series } = series;
+  const warn = () => {}; // no op since warning in handled by getDatas
+  const uniqueValues = new Set();
+  let isAscending = true;
+  let isDescending = true;
+  for (const { data } of rawSeries) {
+    const parseOptions = getParseOptions({ settings, data });
+    let lastValue;
+    for (const [x] of data.rows) {
+      const value = parseXValue(x, parseOptions, warn);
+      if (lastValue !== undefined) {
+        isAscending = isAscending && lastValue <= value;
+        isDescending = isDescending && value <= lastValue;
+      }
+      lastValue = value;
+      uniqueValues.add(value);
+    }
+  }
+  let xValues = Array.from(uniqueValues);
+  if (isDescending) {
+    // JavaScript's .sort() sorts lexicographically by default (e.x. 1, 10, 2)
+    // We could implement a comparator but _.sortBy handles strings, numbers, and dates correctly
+    xValues = _.sortBy(xValues, x => x).reverse();
+  } else if (isAscending) {
+    // default line/area charts to ascending since otherwise lines could be wonky
+    xValues = _.sortBy(xValues, x => x);
+  }
+  return xValues;
+}
+
 // Crossfilter calls toString on each moment object, which calls format(), which is very slow.
 // Replace toString with a function that just returns the unparsed ISO input date, since that works
 // just as well and is much faster
@@ -101,18 +177,16 @@ function moment_fast_toString() {
   return this._i;
 }
 
-export function HACK_parseTimestamp(value, unit, warn) {
+function parseTimestampAndWarn(value, unit) {
   if (value == null) {
-    warn(nullDimensionWarning());
-    return null;
+    return { parsedValue: null, warning: nullDimensionWarning() };
   }
   const m = parseTimestamp(value, unit);
   if (!m.isValid()) {
-    warn(invalidDateWarning(value));
-    return null;
+    return { parsedValue: null, warning: invalidDateWarning(value) };
   }
   m.toString = moment_fast_toString;
-  return m;
+  return { parsedValue: m };
 }
 
 /************************************************************ PROPERTIES ************************************************************/
@@ -143,8 +217,6 @@ export const getFirstNonEmptySeries = series =>
   _.find(series, s => !datasetContainsNoResults(s.data));
 export const isDimensionTimeseries = series =>
   dimensionIsTimeseries(getFirstNonEmptySeries(series).data);
-export const isDimensionNumeric = series =>
-  dimensionIsNumeric(getFirstNonEmptySeries(series).data);
 
 function hasRemappingAndValuesAreStrings({ cols }, i = 0) {
   const column = cols[i];

frontend/src/metabase/visualizations/lib/utils.js

@@ -146,38 +146,6 @@ export function getFriendlyName(column) {
   return column.display_name;
 }
 
-export function getXValues(datas) {
-  // get all xValues
-  const xValuesSet = new Set();
-  // detect if every series' dimension is strictly ascending or descending and use that to sort xValues
-  let isAscending = true;
-  let isDescending = true;
-  // this is fairly optimized since it iterates over every row in the results
-  for (let i = 0, datasLength = datas.length; i < datasLength; i++) {
-    const rows = datas[i];
-    let lastValue = rows.length > 0 ? rows[0][0] : null;
-    xValuesSet.add(lastValue);
-    // skip the first row so we can compare
-    for (let j = 1, rowsLength = rows.length; j < rowsLength; j++) {
-      const value = rows[j][0];
-      xValuesSet.add(value);
-      isAscending = isAscending && lastValue <= value;
-      isDescending = isDescending && lastValue >= value;
-      lastValue = value;
-    }
-  }
-  let xValues = Array.from(xValuesSet);
-  if (isDescending) {
-    // JavaScript's .sort() sorts lexicographically by default (e.x. 1, 10, 2)
-    // We could implement a comparator but _.sortBy handles strings, numbers, and dates correctly
-    xValues = _.sortBy(xValues, x => x).reverse();
-  } else if (isAscending) {
-    // default line/area charts to ascending since otherwise lines could be wonky
-    xValues = _.sortBy(xValues, x => x);
-  }
-  return xValues;
-}
-
 export function isSameSeries(seriesA, seriesB) {
   return (
     (seriesA && seriesA.length) === (seriesB && seriesB.length) &&

frontend/test/metabase/visualizations/lib/renderer_utils.unit.spec.js

+import moment from "moment";
+
+import {
+  getXValues,
+  parseXValue,
+} from "metabase/visualizations/lib/renderer_utils";
+
+describe("getXValues", () => {
+  function getXValuesForRows(listOfRows) {
+    const series = listOfRows.map(rows => ({ data: { rows, cols: [{}] } }));
+    series._raw = series;
+    const settings = {};
+    return getXValues({ settings, series });
+  }
+
+  it("should not change the order of a single series of ascending numbers", () => {
+    expect(getXValuesForRows([[[1], [2], [11]]])).toEqual([1, 2, 11]);
+  });
+  it("should not change the order of a single series of descending numbers", () => {
+    expect(getXValuesForRows([[[11], [2], [1]]])).toEqual([11, 2, 1]);
+  });
+  it("should not change the order of a single series of non-ordered numbers", () => {
+    expect(getXValuesForRows([[[2], [1], [11]]])).toEqual([2, 1, 11]);
+  });
+
+  it("should not change the order of a single series of ascending strings", () => {
+    expect(getXValuesForRows([[["1"], ["2"], ["11"]]])).toEqual([
+      "1",
+      "2",
+      "11",
+    ]);
+  });
+  it("should not change the order of a single series of descending strings", () => {
+    expect(getXValuesForRows([[["1"], ["2"], ["11"]]])).toEqual([
+      "1",
+      "2",
+      "11",
+    ]);
+  });
+  it("should not change the order of a single series of non-ordered strings", () => {
+    expect(getXValuesForRows([[["2"], ["1"], ["11"]]])).toEqual([
+      "2",
+      "1",
+      "11",
+    ]);
+  });
+
+  it("should correctly merge multiple series of ascending numbers", () => {
+    expect(getXValuesForRows([[[2], [11], [12]], [[1], [2], [11]]])).toEqual([
+      1,
+      2,
+      11,
+      12,
+    ]);
+  });
+  it("should correctly merge multiple series of descending numbers", () => {
+    expect(getXValuesForRows([[[12], [11], [2]], [[11], [2], [1]]])).toEqual([
+      12,
+      11,
+      2,
+      1,
+    ]);
+  });
+  it("should use raw row ordering rather than broken out series", () => {
+    const series = [
+      // these are broken out series. the ordering here is ignored
+      { data: { rows: [["bar"]], cols: [{}] } },
+      { data: { rows: [["foo"]], cols: [{}] } },
+    ];
+    series._raw = [{ data: { rows: [["foo"], ["bar"]], cols: [{}] } }];
+    const settings = {};
+    expect(getXValues({ settings, series })).toEqual(["foo", "bar"]);
+  });
+  it("should sort values according to parsed value", () => {
+    expect(
+      getXValuesForRows([
+        [["2019-W33"], ["2019-08-13"]],
+        [["2019-08-11"], ["2019-W33"]],
+      ]).map(x => x.format()),
+    ).toEqual([
+      "2019-08-11T00:00:00Z",
+      "2019-08-12T00:00:00Z",
+      "2019-08-13T00:00:00Z",
+    ]);
+  });
+});
+
+describe("parseXValue", () => {
+  it("should use options as part of the cache key", () => {
+    const value1 = parseXValue("2018-08-23", { isTimeseries: true });
+    const value2 = parseXValue("2018-08-23", { isTimeseries: false });
+    expect(moment.isMoment(value1)).toBe(true);
+    expect(moment.isMoment(value2)).toBe(false);
+  });
+
+  it("should warn repeatedly (despite caching)", () => {
+    const warn = jest.fn();
+    parseXValue("2018-W60", { isTimeseries: true }, warn);
+    parseXValue("2018-W60", { isTimeseries: true }, warn);
+    expect(warn.mock.calls.length).toBe(2);
+  });
+});

frontend/test/metabase/visualizations/lib/utils.unit.spec.js

@@ -3,7 +3,6 @@ import {
   computeMaxDecimalsForValues,
   getCardAfterVisualizationClick,
   getColumnCardinality,
-  getXValues,
   getFriendlyName,
   getDefaultDimensionsAndMetrics,
 } from "metabase/visualizations/lib/utils";
@@ -160,45 +159,6 @@ describe("metabase/visualization/lib/utils", () => {
     });
   });
 
-  describe("getXValues", () => {
-    it("should not change the order of a single series of ascending numbers", () => {
-      expect(getXValues([[[1], [2], [11]]])).toEqual([1, 2, 11]);
-    });
-    it("should not change the order of a single series of descending numbers", () => {
-      expect(getXValues([[[11], [2], [1]]])).toEqual([11, 2, 1]);
-    });
-    it("should not change the order of a single series of non-ordered numbers", () => {
-      expect(getXValues([[[2], [1], [11]]])).toEqual([2, 1, 11]);
-    });
-
-    it("should not change the order of a single series of ascending strings", () => {
-      expect(getXValues([[["1"], ["2"], ["11"]]])).toEqual(["1", "2", "11"]);
-    });
-    it("should not change the order of a single series of descending strings", () => {
-      expect(getXValues([[["1"], ["2"], ["11"]]])).toEqual(["1", "2", "11"]);
-    });
-    it("should not change the order of a single series of non-ordered strings", () => {
-      expect(getXValues([[["2"], ["1"], ["11"]]])).toEqual(["2", "1", "11"]);
-    });
-
-    it("should correctly merge multiple series of ascending numbers", () => {
-      expect(getXValues([[[2], [11], [12]], [[1], [2], [11]]])).toEqual([
-        1,
-        2,
-        11,
-        12,
-      ]);
-    });
-    it("should correctly merge multiple series of descending numbers", () => {
-      expect(getXValues([[[12], [11], [2]], [[11], [2], [1]]])).toEqual([
-        12,
-        11,
-        2,
-        1,
-      ]);
-    });
-  });
-
   describe("getColumnCardinality", () => {
     it("should get column cardinality", () => {
       const cols = [{}];