fix: graceful scale fallbacks and warnings (#704)

fix #678
elastic · Jun 15, 2020 · ed49bbb · ed49bbb
1 parent a5f48f7
commit ed49bbb
Show file tree

Hide file tree

Showing 9 changed files with 183 additions and 37 deletions.
diff --git a/src/chart_types/partition_chart/renderer/canvas/partition.tsx b/src/chart_types/partition_chart/renderer/canvas/partition.tsx
@@ -120,12 +120,7 @@ class PartitionComponent extends React.Component<PartitionProps> {
         indices.forEach((i) => datumIndices.add(i));
       }
     });
-    /*
-     *console.log(
-     *  pickedShapes.map((s) => s.value),
-     *  [...datumIndices.values()],
-     *);
-     */
+
     return pickedShapes; // placeholder
   }
 

diff --git a/src/chart_types/xy_chart/domains/x_domain.test.ts b/src/chart_types/xy_chart/domains/x_domain.test.ts
@@ -443,6 +443,61 @@ describe('X Domain', () => {
     );
     expect(mergedDomain.domain).toEqual([0, 1, 2, 5, 7]);
   });
+
+  test('Should fallback to ordinal scale if not array of numbers', () => {
+    const ds1: BasicSeriesSpec = {
+      chartType: ChartTypes.XYAxis,
+      specType: SpecTypes.Series,
+      id: 'ds1',
+      groupId: 'g1',
+      seriesType: SeriesTypes.Bar,
+      xAccessor: 'x',
+      yAccessors: ['y'],
+      xScaleType: ScaleType.Linear,
+      yScaleType: ScaleType.Linear,
+      yScaleToDataExtent: false,
+      data: [
+        { x: 0, y: 0 },
+        { x: 'a', y: 0 },
+        { x: 2, y: 0 },
+        { x: 5, y: 0 },
+      ],
+    };
+    const ds2: BasicSeriesSpec = {
+      chartType: ChartTypes.XYAxis,
+      specType: SpecTypes.Series,
+      id: 'ds2',
+      groupId: 'g2',
+      seriesType: SeriesTypes.Bar,
+      xAccessor: 'x',
+      yAccessors: ['y'],
+      xScaleType: ScaleType.Linear,
+      yScaleType: ScaleType.Linear,
+      yScaleToDataExtent: false,
+      data: [
+        { x: 0, y: 0 },
+        { x: 7, y: 0 },
+      ],
+    };
+    const specDataSeries = [ds1, ds2];
+
+    const { xValues } = getSplittedSeries(specDataSeries);
+    const mergedDomain = mergeXDomain(
+      [
+        {
+          seriesType: SeriesTypes.Bar,
+          xScaleType: ScaleType.Linear,
+        },
+        {
+          seriesType: SeriesTypes.Bar,
+          xScaleType: ScaleType.Ordinal,
+        },
+      ],
+      xValues,
+    );
+    expect(mergedDomain.domain).toEqual([0, 'a', 2, 5, 7]);
+    expect(mergedDomain.scaleType).toEqual(ScaleType.Ordinal);
+  });
   test('Should merge multi bar/line ordinal series correctly', () => {
     const ds1: BasicSeriesSpec = {
       chartType: ChartTypes.XYAxis,

diff --git a/src/chart_types/xy_chart/domains/x_domain.ts b/src/chart_types/xy_chart/domains/x_domain.ts
@@ -18,8 +18,9 @@
  */
 
 import { ScaleType } from '../../../scales/constants';
-import { compareByValueAsc, identity, isNumberArray } from '../../../utils/commons';
+import { compareByValueAsc, identity } from '../../../utils/commons';
 import { computeContinuousDataDomain, computeOrdinalDataDomain, Domain } from '../../../utils/domain';
+import { Logger } from '../../../utils/logger';
 import { isCompleteBound, isLowerBound, isUpperBound } from '../utils/axis_type_utils';
 import { BasicSeriesSpec, DomainRange, SeriesTypes } from '../utils/specs';
 import { XDomain } from './types';
@@ -36,6 +37,7 @@ export function mergeXDomain(
   specs: Pick<BasicSeriesSpec, 'seriesType' | 'xScaleType'>[],
   xValues: Set<string | number>,
   customXDomain?: DomainRange | Domain,
+  fallbackScale?: ScaleType,
 ): XDomain {
   const mainXScaleType = convertXScaleTypes(specs);
   if (!mainXScaleType) {
@@ -46,7 +48,13 @@ export function mergeXDomain(
   let seriesXComputedDomains;
   let minInterval = 0;
 
-  if (mainXScaleType.scaleType === ScaleType.Ordinal) {
+  if (mainXScaleType.scaleType === ScaleType.Ordinal || fallbackScale === ScaleType.Ordinal) {
+    if (mainXScaleType.scaleType !== ScaleType.Ordinal) {
+      Logger.warn(
+        `Each X value in a ${mainXScaleType.scaleType} x scale needs be be a number. Using ordinal x scale as fallback.`,
+      );
+    }
+
     seriesXComputedDomains = computeOrdinalDataDomain(values, identity, false, true);
     if (customXDomain) {
       if (Array.isArray(customXDomain)) {
@@ -58,11 +66,7 @@ export function mergeXDomain(
   } else {
     seriesXComputedDomains = computeContinuousDataDomain(values, identity, true);
     let customMinInterval: undefined | number;
-    if (!isNumberArray(values)) {
-      throw new Error(
-        `Each X value in a ${mainXScaleType.scaleType} x scale needs be be a number. String or objects are not allowed`,
-      );
-    }
+
     if (customXDomain) {
       if (Array.isArray(customXDomain)) {
         throw new TypeError('xDomain for continuous scale should be a DomainRange object, not an array');
@@ -91,7 +95,7 @@ export function mergeXDomain(
         seriesXComputedDomains = [computedDomainMin, customXDomain.max];
       }
     }
-    const computedMinInterval = findMinInterval(values);
+    const computedMinInterval = findMinInterval(values as number[]);
     if (customMinInterval != null) {
       // Allow greater custom min iff xValues has 1 member.
       if (xValues.size > 1 && customMinInterval > computedMinInterval) {
@@ -107,7 +111,7 @@ export function mergeXDomain(
 
   return {
     type: 'xDomain',
-    scaleType: mainXScaleType.scaleType,
+    scaleType: fallbackScale ?? mainXScaleType.scaleType,
     isBandScale: mainXScaleType.isBandScale,
     domain: seriesXComputedDomains,
     minInterval,

diff --git a/src/chart_types/xy_chart/state/utils/utils.ts b/src/chart_types/xy_chart/state/utils/utils.ts
@@ -180,13 +180,15 @@ export function computeSeriesDomains(
   deselectedDataSeries: SeriesIdentifier[] = [],
   customXDomain?: DomainRange | Domain,
 ): SeriesDomainsAndData {
-  const { splittedSeries, xValues, seriesCollection } = deselectedDataSeries
-    ? getSplittedSeries(seriesSpecs, deselectedDataSeries)
-    : getSplittedSeries(seriesSpecs, []);
+  const {
+    splittedSeries,
+    xValues, seriesCollection,
+    fallbackScale,
+  } = getSplittedSeries(seriesSpecs, deselectedDataSeries);
   const splittedDataSeries = [...splittedSeries.values()];
   const specsArray = [...seriesSpecs.values()];
 
-  const xDomain = mergeXDomain(specsArray, xValues, customXDomain);
+  const xDomain = mergeXDomain(specsArray, xValues, customXDomain, fallbackScale);
   const yDomain = mergeYDomain(splittedSeries, specsArray, customYDomainsByGroupId);
 
   const formattedDataSeries = getFormattedDataseries(

diff --git a/src/chart_types/xy_chart/utils/series.test.ts b/src/chart_types/xy_chart/utils/series.test.ts
@@ -683,26 +683,26 @@ describe('Series', () => {
     expect(getSortedDataSeriesColorsValuesMap(seriesCollection)).toEqual(undefinedSortedColorValues);
   });
   test('clean datum shall parse string as number for y values', () => {
-    let datum = cleanDatum([0, 1, 2], 0, 1, 2);
+    let datum = cleanDatum([0, 1, 2], 0, 1, [], 2);
     expect(datum).toBeDefined();
     expect(datum?.y1).toBe(1);
     expect(datum?.y0).toBe(2);
-    datum = cleanDatum([0, '1', 2], 0, 1, 2);
+    datum = cleanDatum([0, '1', 2], 0, 1, [], 2);
     expect(datum).toBeDefined();
     expect(datum?.y1).toBe(1);
     expect(datum?.y0).toBe(2);
 
-    datum = cleanDatum([0, '1', '2'], 0, 1, 2);
+    datum = cleanDatum([0, '1', '2'], 0, 1, [], 2);
     expect(datum).toBeDefined();
     expect(datum?.y1).toBe(1);
     expect(datum?.y0).toBe(2);
 
-    datum = cleanDatum([0, 1, '2'], 0, 1, 2);
+    datum = cleanDatum([0, 1, '2'], 0, 1, [], 2);
     expect(datum).toBeDefined();
     expect(datum?.y1).toBe(1);
     expect(datum?.y0).toBe(2);
 
-    datum = cleanDatum([0, 'invalid', 'invalid'], 0, 1, 2);
+    datum = cleanDatum([0, 'invalid', 'invalid'], 0, 1, [], 2);
     expect(datum).toBeDefined();
     expect(datum?.y1).toBe(null);
     expect(datum?.y0).toBe(null);

diff --git a/src/chart_types/xy_chart/utils/series.ts b/src/chart_types/xy_chart/utils/series.ts
@@ -23,6 +23,7 @@ import { ColorOverrides } from '../../../state/chart_state';
 import { Accessor, AccessorFn, getAccessorValue } from '../../../utils/accessor';
 import { Datum, Color } from '../../../utils/commons';
 import { GroupId, SpecId } from '../../../utils/ids';
+import { Logger } from '../../../utils/logger';
 import { ColorConfig } from '../../../utils/themes/theme';
 import { splitSpecsByGroupId, YBasicSeriesSpec } from '../domains/y_domain';
 import { LastValues } from '../state/utils/types';
@@ -150,6 +151,7 @@ export function splitSeries({
   const series = new Map<SeriesKey, RawDataSeries>();
   const colorsValues = new Set<string>();
   const xValues = new Set<string | number>();
+  const nonNumericValues: any[] = [];
 
   data.forEach((datum) => {
     const splitAccessors = getSplitAccessors(datum, splitSeriesAccessors);
@@ -164,6 +166,7 @@ export function splitSeries({
           datum,
           xAccessor,
           accessor,
+          nonNumericValues,
           y0Accessors && y0Accessors[index],
           markSizeAccessor,
         );
@@ -175,7 +178,14 @@ export function splitSeries({
         }
       });
     } else {
-      const cleanedDatum = cleanDatum(datum, xAccessor, yAccessors[0], y0Accessors && y0Accessors[0], markSizeAccessor);
+      const cleanedDatum = cleanDatum(
+        datum,
+        xAccessor,
+        yAccessors[0],
+        nonNumericValues,
+        y0Accessors && y0Accessors[0],
+        markSizeAccessor,
+      );
       if (cleanedDatum !== null && cleanedDatum.x !== null && cleanedDatum.x !== undefined) {
         xValues.add(cleanedDatum.x);
         const seriesKey = updateSeriesMap(series, splitAccessors, yAccessors[0], cleanedDatum, specId);
@@ -184,6 +194,12 @@ export function splitSeries({
     }
   });
 
+  if (nonNumericValues.length > 0) {
+    Logger.warn(`Found non-numeric y value${nonNumericValues.length > 1 ? 's' : ''} in dataset for spec "${specId}"`,
+      `(${nonNumericValues.map((v) => JSON.stringify(v)).join(', ')})`
+    );
+  }
+
   return {
     rawDataSeries: [...series.values()],
     colorsValues,
@@ -266,6 +282,7 @@ export function cleanDatum(
   datum: Datum,
   xAccessor: Accessor | AccessorFn,
   yAccessor: Accessor,
+  nonNumericValues: any[],
   y0Accessor?: Accessor,
   markSizeAccessor?: Accessor | AccessorFn,
 ): RawDataSeriesDatum | null {
@@ -280,20 +297,26 @@ export function cleanDatum(
   }
 
   const mark = markSizeAccessor === undefined ? null : getAccessorValue(datum, markSizeAccessor);
-  const y1 = castToNumber(datum[yAccessor]);
+  const y1 = castToNumber(datum[yAccessor], nonNumericValues);
   const cleanedDatum: RawDataSeriesDatum = { x, y1, datum, y0: null, mark };
   if (y0Accessor) {
-    cleanedDatum.y0 = castToNumber(datum[y0Accessor as keyof typeof datum]);
+    cleanedDatum.y0 = castToNumber(datum[y0Accessor as keyof typeof datum], nonNumericValues);
   }
   return cleanedDatum;
 }
 
-function castToNumber(value: any): number | null {
+function castToNumber(value: any, nonNumericValues: any[]): number | null {
   if (value === null || value === undefined) {
     return null;
   }
   const num = Number(value);
-  return isNaN(num) ? null : num;
+
+  if (isNaN(num)) {
+    nonNumericValues.push(value);
+
+    return null;
+  }
+  return num;
 }
 
 /** @internal */
@@ -406,10 +429,12 @@ export function getSplittedSeries(
   splittedSeries: Map<SpecId, RawDataSeries[]>;
   seriesCollection: Map<SeriesKey, SeriesCollectionValue>;
   xValues: Set<string | number>;
+  fallbackScale?: ScaleType;
 } {
   const splittedSeries = new Map<SpecId, RawDataSeries[]>();
   const seriesCollection = new Map<SeriesKey, SeriesCollectionValue>();
   const xValues: Set<any> = new Set();
+  let isNumberArray = true;
   let isOrdinalScale = false;
   // eslint-disable-next-line no-restricted-syntax
   for (const spec of seriesSpecs) {
@@ -437,6 +462,9 @@ export function getSplittedSeries(
 
     // eslint-disable-next-line no-restricted-syntax
     for (const xValue of dataSeries.xValues) {
+      if (isNumberArray && typeof xValue !== 'number') {
+        isNumberArray = false;
+      }
       xValues.add(xValue);
     }
   }
@@ -445,7 +473,8 @@ export function getSplittedSeries(
     splittedSeries,
     seriesCollection,
     // keep the user order for ordinal scales
-    xValues: isOrdinalScale ? xValues : new Set([...xValues].sort()),
+    xValues: (isOrdinalScale || !isNumberArray) ? xValues : new Set([...xValues].sort()),
+    fallbackScale: (!isOrdinalScale && !isNumberArray) ? ScaleType.Ordinal : undefined,
   };
 }
 

diff --git a/src/state/chart_state.ts b/src/state/chart_state.ts
@@ -30,6 +30,7 @@ import { Spec, PointerEvent } from '../specs';
 import { DEFAULT_SETTINGS_SPEC } from '../specs/constants';
 import { Color } from '../utils/commons';
 import { Dimensions } from '../utils/dimensions';
+import { Logger } from '../utils/logger';
 import { Point } from '../utils/point';
 import { StateActions } from './actions';
 import { CHART_RENDERED } from './actions/chart';
@@ -408,8 +409,7 @@ function findMainChartType(specs: SpecList): ChartTypes | null {
   // https://stackoverflow.com/questions/55012174/why-doesnt-object-keys-return-a-keyof-type-in-typescript
   const chartTypes = Object.keys(types).filter((type) => type !== ChartTypes.Global);
   if (chartTypes.length > 1) {
-    // eslint-disable-next-line no-console
-    console.warn('Multiple chart type on the same configuration');
+    Logger.warn('Multiple chart type on the same configuration');
     return null;
   }
   return chartTypes[0] as ChartTypes;

diff --git a/src/utils/commons.ts b/src/utils/commons.ts
@@ -326,11 +326,6 @@ export function mergePartial<T>(
   return getPartialValue<T>(baseClone, partial, additionalPartials);
 }
 
-/** @internal */
-export function isNumberArray(value: unknown): value is number[] {
-  return Array.isArray(value) && value.every((element) => typeof element === 'number');
-}
-
 /** @internal */
 export function getUniqueValues<T>(fullArray: T[], uniqueProperty: keyof T): T[] {
   return fullArray.reduce<{