Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(time-series table): display null values in time-series table and sortable #19024

Merged
merged 4 commits into from
Mar 9, 2022
Merged

fix(time-series table): display null values in time-series table and sortable #19024

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2eda2ba
fix: display null values in time-series table and sortable
Mar 4, 2022
28f8d4d
add unit test
Mar 7, 2022
e0a26d0
fix unit test
Mar 8, 2022
4bd809b
Add sortNumericValues with different nan treatment
ktmud Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions superset-frontend/src/visualizations/TimeTable/SortNumberWithMixedTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* 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 { Row } from 'react-table';

const sortNumberWithMixedTypes = (rowA: Row, rowB: Row, columnId: string) => {
const rowAVal = rowA.values[columnId].props['data-value'];
const rowBVal = rowB.values[columnId].props['data-value'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move Row and columnId out of this utility function to make it more generic? I'd imagine it can be useful in other places as well.

import { JSONPrimitive } from '@superset-ui/core';

export default function sortNumberWithMixedTypes(
  valueA: JSONPrimitive | undefined,
  valueB: JSONPrimitive | undefined
) {
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sortType is prop from react-table useSortBy: https://github.com/TanStack/react-table/blob/alpha/docs/api/useSortBy.md
I am not sure I can change my function signature?

Copy link
Member

@ktmud ktmud Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can always have wrapper function for sortType:

sortType: useCallback((rowA, rowB, columnId) =>
  sortNumberWithMixedTypes(
    rowA.values[columnId].props['data-value'],
    rowB.values[columnId].props['data-value'],
  ), []);

Copy link
Author

@graceguo-supercat graceguo-supercat Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

ESLint: React Hook "useCallback" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function.(react-hooks/rules-of-hooks)

this sortType must be memoized.

Copy link
Member

@ktmud ktmud Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it needs to be memorized, which is why I added useCallback.

You can either move the useCallback up a level or move the wrapper outside of the component as a simple one liner in the component file without using useCallback.

Point is, such utility functions should not depend on too specific input data types if we want to maximize reusability.

if (typeof rowAVal === 'number' && typeof rowBVal === 'number') {
return rowAVal - rowBVal;
}
if (typeof rowAVal === 'number') {
return 1;
}
if (typeof rowBVal === 'number') {
return -1;
}
return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanna add the test case where both are not a number?

also, can these ever be strings or objects or other types? or only ever number or null?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

return (
  (rowAVal == null) - (rowBVal == null) ||
  Number.isNaN(Number(rowAVal)) - Number.isNaN(Number(rowBVal)) ||
  Number(rowAVal) - Number(rowBVal)
);

This should handle all cases.

Copy link
Author

@graceguo-supercat graceguo-supercat Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the time-series table viz type, i think the cell values could be string, number, null, number mixed with null.
I agree, this should handle all cases. But Typescript doesn't like

(rowAVal == null) - (rowBVal == null)

any suggestion?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked, the first column metric doesn't use this sorting function. So the cell values could be null, number, no string type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can wrap them with Number as well. It should convert false to 0 and true to 1.

return (
  Number(rowAVal == null) - Number(rowBVal == null) ||
  Number.isNaN(Number(rowAVal)) - Number.isNaN(Number(rowBVal)) ||
  Number(rowAVal) - Number(rowBVal)
);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry...typescript doesn't like

Number.isNaN(Number(rowAVal)) - Number.isNaN(Number(rowBVal))

};

export default sortNumberWithMixedTypes;
24 changes: 12 additions & 12 deletions superset-frontend/src/visualizations/TimeTable/TimeTable.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import moment from 'moment';
import FormattedNumber from './FormattedNumber';
import SparklineCell from './SparklineCell';
import './TimeTable.less';
import sortNumberWithMixedTypes from './SortNumberWithMixedTypes';

const ACCESSIBLE_COLOR_BOUNDS = ['#ca0020', '#0571b0'];

Expand Down Expand Up @@ -126,11 +127,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 +189,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
66 changes: 66 additions & 0 deletions superset-frontend/src/visualizations/TimeTable/sort.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/**
* 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 sortFn from './SortNumberWithMixedTypes';

describe('sort Number and mixed types', () => {
const columnId = 'mLnVxkc1g';
const rowA = {
values: {
metric: 'Albania',
mLnVxkc1g: {
props: {
'data-value': null,
},
},
},
};
const rowB = {
values: {
metric: 'Afghanistan',
mLnVxkc1g: {
props: {
'data-value': -0.6749999999999972,
},
},
},
};
const rowC = {
values: {
metric: 'Malawi',
mLnVxkc1g: {
props: {
'data-value': 4.852999999999994,
},
},
},
};

it('should treat null values as smallest', () => {
// @ts-ignore
expect(sortFn(rowA, rowB, columnId)).toBe(-1);
// @ts-ignore
expect(sortFn(rowA, rowC, columnId)).toBe(-1);
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to ts ignore here? if the rows are the improper types, then you can define them as the correct ones above instead of ignoring types

Copy link
Author

@graceguo-supercat graceguo-supercat Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Row type is very complicated object, i am not sure how to mock my input data:

export interface UseTableRowProps<D extends object> {
    cells: Array<Cell<D>>;
    allCells: Array<Cell<D>>;
    values: Record<IdType<D>, CellValue>;
    getRowProps: (propGetter?: RowPropGetter<D>) => TableRowProps;
    index: number;
    original: D;
    id: string;
    subRows: Array<Row<D>>;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const rowA = {} as unknown as UseTableRowProps;?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow this trick works!!

expect(sortFn(rowB, rowC, columnId)).toBe(
rowB.values[columnId].props['data-value'] -
rowC.values[columnId].props['data-value'],
);
});
});