Skip to content

Commit

Permalink
fix(time-series table): display null values in time-series table and …
Browse files Browse the repository at this point in the history
…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 <jesse.yang@airbnb.com>
(cherry picked from commit d539fc2)
  • Loading branch information
Grace Guo authored and villebro committed Apr 3, 2022
1 parent bc65cf4 commit e97b123
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 12 deletions.
61 changes: 61 additions & 0 deletions superset-frontend/src/utils/sortNumericValues.test.ts
Original file line number Diff line number Diff line change
@@ -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']);
});
52 changes: 52 additions & 0 deletions superset-frontend/src/utils/sortNumericValues.ts
Original file line number Diff line number Diff line change
@@ -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)
);
}
33 changes: 21 additions & 12 deletions superset-frontend/src/visualizations/TimeTable/TimeTable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,23 @@ 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';
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;
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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 =
Expand Down

0 comments on commit e97b123

Please sign in to comment.