From d539fc217a52bb1fc82ebe0f6d5aa2f03031db28 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Tue, 8 Mar 2022 17:03:49 -0800 Subject: [PATCH] fix(time-series table): display null values in time-series table and sortable (#19024) * fix: display null values in time-series table and sortable * add unit test * fix unit test * Add sortNumericValues with different nan treatment Co-authored-by: Jesse Yang --- .../src/utils/sortNumericValues.test.ts | 61 +++++++++++++++++++ .../src/utils/sortNumericValues.ts | 52 ++++++++++++++++ .../visualizations/TimeTable/TimeTable.jsx | 33 ++++++---- 3 files changed, 134 insertions(+), 12 deletions(-) create mode 100644 superset-frontend/src/utils/sortNumericValues.test.ts create mode 100644 superset-frontend/src/utils/sortNumericValues.ts diff --git a/superset-frontend/src/utils/sortNumericValues.test.ts b/superset-frontend/src/utils/sortNumericValues.test.ts new file mode 100644 index 0000000000000..5582afe22b5ad --- /dev/null +++ b/superset-frontend/src/utils/sortNumericValues.test.ts @@ -0,0 +1,61 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import sortNumericValues from './sortNumericValues'; + +test('should always sort null and NaNs to bottom', () => { + expect([null, 1, 2, '1', '5', NaN].sort(sortNumericValues)).toEqual([ + 1, + '1', + 2, + '5', + NaN, + null, + ]); + expect( + [null, 1, 2, '1', '5', NaN].sort((a, b) => + sortNumericValues(a, b, { descending: true }), + ), + ).toEqual(['5', 2, 1, '1', NaN, null]); +}); + +test('should treat null and NaN as smallest numbers', () => { + expect( + [null, 1, 2, '1', '5', NaN].sort((a, b) => + sortNumericValues(a, b, { nanTreatment: 'asSmallest' }), + ), + ).toEqual([null, NaN, 1, '1', 2, '5']); + expect( + [null, 1, 2, '1', '5', NaN].sort((a, b) => + sortNumericValues(a, b, { nanTreatment: 'asSmallest', descending: true }), + ), + ).toEqual(['5', 2, 1, '1', NaN, null]); +}); + +test('should treat null and NaN as largest numbers', () => { + expect( + [null, 1, 2, '1', '5', NaN].sort((a, b) => + sortNumericValues(a, b, { nanTreatment: 'asLargest' }), + ), + ).toEqual([1, '1', 2, '5', NaN, null]); + expect( + [null, 1, 2, '1', '5', NaN].sort((a, b) => + sortNumericValues(a, b, { nanTreatment: 'asLargest', descending: true }), + ), + ).toEqual([null, NaN, '5', 2, 1, '1']); +}); diff --git a/superset-frontend/src/utils/sortNumericValues.ts b/superset-frontend/src/utils/sortNumericValues.ts new file mode 100644 index 0000000000000..a1cc404f72971 --- /dev/null +++ b/superset-frontend/src/utils/sortNumericValues.ts @@ -0,0 +1,52 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { JsonPrimitive } from '@superset-ui/core'; + +export type NaNTreatment = 'alwaysLast' | 'asSmallest' | 'asLargest'; + +/** + * Array.sort(...) comparator for potential numeric values with the ability to + * treat null and NaN as the smallest or largest values or always sort to bottom. + */ +export default function sortNumericValues( + valueA: JsonPrimitive, + valueB: JsonPrimitive, + { + descending = false, + nanTreatment = 'alwaysLast', + }: { descending?: boolean; nanTreatment?: NaNTreatment } = {}, +) { + let orderByIsNaN = + Number(valueA == null) - Number(valueB == null) || + Number(Number.isNaN(Number(valueA))) - Number(Number.isNaN(Number(valueB))); + + // if A is null or NaN and B is not, `orderByIsNaN` is 1, + // which will make A come after B in the sorted array, + // since we want to treat A as smallest number, we need to flip the sign + // when sorting in ascending order. + if (nanTreatment === 'asSmallest' && !descending) { + orderByIsNaN = -orderByIsNaN; + } + if (nanTreatment === 'asLargest' && descending) { + orderByIsNaN = -orderByIsNaN; + } + return ( + orderByIsNaN || (Number(valueA) - Number(valueB)) * (descending ? -1 : 1) + ); +} diff --git a/superset-frontend/src/visualizations/TimeTable/TimeTable.jsx b/superset-frontend/src/visualizations/TimeTable/TimeTable.jsx index b02eb985a0abc..d12022f74c60e 100644 --- a/superset-frontend/src/visualizations/TimeTable/TimeTable.jsx +++ b/superset-frontend/src/visualizations/TimeTable/TimeTable.jsx @@ -27,6 +27,7 @@ import { MetricOption, } from '@superset-ui/chart-controls'; import moment from 'moment'; +import sortNumericValues from 'src/utils/sortNumericValues'; import FormattedNumber from './FormattedNumber'; import SparklineCell from './SparklineCell'; @@ -34,6 +35,15 @@ import './TimeTable.less'; const ACCESSIBLE_COLOR_BOUNDS = ['#ca0020', '#0571b0']; +const sortNumberWithMixedTypes = (rowA, rowB, columnId, descending) => + sortNumericValues( + rowA.values[columnId].props['data-value'], + rowB.values[columnId].props['data-value'], + { descending, nanTreatment: 'asSmallest' }, + ) * + // react-table sort function always expects -1 for smaller number + (descending ? -1 : 1); + function colorFromBounds(value, bounds, colorBounds = ACCESSIBLE_COLOR_BOUNDS) { if (bounds) { const [min, max] = bounds; @@ -126,11 +136,7 @@ const TimeTable = ({ )} ), - sortType: (rowA, rowB, columnId) => { - const rowAVal = rowA.values[columnId].props['data-value']; - const rowBVal = rowB.values[columnId].props['data-value']; - return rowAVal - rowBVal; - }, + sortType: sortNumberWithMixedTypes, })), ], [columnConfigs], @@ -192,14 +198,17 @@ const TimeTable = ({ } else { v = reversedEntries[timeLag][valueField]; } - if (column.comparisonType === 'diff') { - v = recent - v; - } else if (column.comparisonType === 'perc') { - v = recent / v; - } else if (column.comparisonType === 'perc_change') { - v = recent / v - 1; + if (typeof v === 'number' && typeof recent === 'number') { + if (column.comparisonType === 'diff') { + v = recent - v; + } else if (column.comparisonType === 'perc') { + v = recent / v; + } else if (column.comparisonType === 'perc_change') { + v = recent / v - 1; + } + } else { + v = null; } - v = v || 0; } else if (column.colType === 'contrib') { // contribution to column total v =