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

graceguo-supercat · 2022-03-04T02:20:33Z

SUMMARY

Currently Time-series table display null value as number 0. I had a fix #18039 but it wasn't handle sorting correctly. in airbnb we found when null value mixed with number, the sort function didn't work, so i reverted the previous PR.

This PR is still try to fix the null value issue with similar approach, and add extra logic fix sorting: if null value is mixed with number values, all null value will be pushed to the smaller end.

TESTING INSTRUCTIONS

CI and manual test

ADDITIONAL INFORMATION

Has associated issue: fix: handle null values in time-series table #18039
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2022-03-04T03:07:07Z

Codecov Report

Merging #19024 (4bd809b) into master (50bb86d) will decrease coverage by 0.06%.
The diff coverage is 47.61%.

@@            Coverage Diff             @@
##           master   #19024      +/-   ##
==========================================
- Coverage   66.56%   66.50%   -0.07%     
==========================================
  Files        1641     1643       +2     
  Lines       63495    63455      -40     
  Branches     6425     6448      +23     
==========================================
- Hits        42265    42200      -65     
- Misses      19550    19585      +35     
+ Partials     1680     1670      -10

Flag	Coverage Δ
javascript	`51.23% <47.61%> (-0.14%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rontend/src/visualizations/TimeTable/TimeTable.jsx	`0.00% <0.00%> (ø)`
superset-frontend/src/utils/sortNumericValues.ts	`100.00% <100.00%> (ø)`
...ts/nativeFilters/FilterBar/FilterControls/utils.ts	`25.00% <0.00%> (-45.00%)`	⬇️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx	`64.67% <0.00%> (-14.73%)`	⬇️
...d/components/nativeFilters/FilterCard/ScopeRow.tsx	`58.82% <0.00%> (-13.91%)`	⬇️
...Filters/FilterBar/FilterControls/FilterControl.tsx	`70.96% <0.00%> (-11.18%)`	⬇️
...tend/src/filters/components/Time/transformProps.ts	`50.00% <0.00%> (-7.15%)`	⬇️
...d/src/filters/components/Time/TimeFilterPlugin.tsx	`47.05% <0.00%> (-6.28%)`	⬇️
superset-frontend/src/components/Select/utils.ts	`52.94% <0.00%> (-6.15%)`	⬇️
...oard/components/nativeFilters/FilterCard/Styles.ts	`80.00% <0.00%> (-4.62%)`	⬇️
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50bb86d...4bd809b. Read the comment docs.

etr2460

before we make this fix could we either:

add unit tests for this function
migrate the component to TS

or even both? since we know related changes have broken this chart before. thanks!

ktmud · 2022-03-04T18:55:08Z

superset-frontend/src/visualizations/TimeTable/TimeTable.jsx

@@ -129,7 +129,16 @@ 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;
+          if (typeof rowAVal === 'number' && typeof rowBVal === 'number') {


This could be simplified as:

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

@ktmud thank you for the simplified expression, but i feel it is a little hard to digest. I would prefer straightforward expression, given there is no significant space or complexity optimization. And after user research, we might want to add extra logic to always push null values at the bottom (instead of sorting as smallest), i think my original expression is easier to update and understood.

My proposed expression already included logics to sort null and NaN to the bottom.

Push null and NaN to the bottom is not decided yet. I think we will run user research to decide sort or push.

graceguo-supercat · 2022-03-07T19:47:45Z

before we make this fix could we either:

add unit tests for this function

migrate the component to TS

or even both? since we know related changes have broken this chart before. thanks!

i added unit test.
I tried migration, but triggered a lot of errors, I am afraid of causing other bugs. I don't want to lose the focus of this Sorting issue.

etr2460 · 2022-03-08T17:28:56Z

superset-frontend/src/visualizations/TimeTable/sort.test.ts

+    // @ts-ignore
+    expect(sortFn(rowA, rowB, columnId)).toBe(-1);
+    // @ts-ignore
+    expect(sortFn(rowA, rowC, columnId)).toBe(-1);
+    // @ts-ignore


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

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>>; }

const rowA = {} as unknown as UseTableRowProps;?

wow this trick works!!

etr2460 · 2022-03-08T17:30:05Z

superset-frontend/src/visualizations/TimeTable/SortNumberWithMixedTypes.ts

+  if (typeof rowBVal === 'number') {
+    return -1;
+  }
+  return 0;


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?

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

This should handle all cases.

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?

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

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) );

sorry...typescript doesn't like

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

ktmud · 2022-03-08T17:39:55Z

superset-frontend/src/visualizations/TimeTable/SortNumberWithMixedTypes.ts

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


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 ) { }

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?

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'], ), []);

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.

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.

ktmud · 2022-03-09T00:09:47Z

superset-frontend/src/visualizations/TimeTable/TimeTable.jsx

+    { descending, nanTreatment: 'asSmallest' },
+  ) *
+  // react-table sort function always expects -1 for smaller number
+  (descending ? -1 : 1);


@graceguo-supercat @etr2460 I took a different approach and added the option to change how null/nans are treated. Please take a look

graceguo-supercat · 2022-03-09T00:56:38Z

i am ok with new solution, it is a more general comparator sort string/number/NaN/null like a number type way.

etr2460

lgtm, thanks for the tests, the iteration, and all the work here to make sure we don't introduce any more bugs!

…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)

fix: display null values in time-series table and sortable

2eda2ba

pull-request-size bot added the size/S label Mar 4, 2022

graceguo-supercat requested a review from ktmud March 4, 2022 02:20

graceguo-supercat changed the title ~~fix (time-series table): display null values in time-series table and sortable~~ fix(time-series table): display null values in time-series table and sortable Mar 4, 2022

etr2460 reviewed Mar 4, 2022

View reviewed changes

ktmud reviewed Mar 4, 2022

View reviewed changes

add unit test

28f8d4d

pull-request-size bot added size/L and removed size/S labels Mar 7, 2022

graceguo-supercat requested review from ktmud and etr2460 March 7, 2022 19:48

etr2460 reviewed Mar 8, 2022

View reviewed changes

ktmud reviewed Mar 8, 2022

View reviewed changes

Grace Guo and others added 2 commits March 8, 2022 14:13

fix unit test

e0a26d0

Add sortNumericValues with different nan treatment

4bd809b

ktmud force-pushed the gg-fix-time-table-null-values branch from 9475984 to 4bd809b Compare March 9, 2022 00:08

ktmud reviewed Mar 9, 2022

View reviewed changes

etr2460 approved these changes Mar 9, 2022

View reviewed changes

graceguo-supercat merged commit d539fc2 into apache:master Mar 9, 2022

villebro added the lts-v1 label Mar 29, 2022

mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 labels Mar 13, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

graceguo-supercat commented Mar 4, 2022

codecov bot commented Mar 4, 2022 •

edited

Loading

etr2460 left a comment

ktmud Mar 4, 2022 •

edited

Loading

graceguo-supercat Mar 7, 2022

ktmud Mar 7, 2022

graceguo-supercat Mar 8, 2022

graceguo-supercat commented Mar 7, 2022 •

edited

Loading

etr2460 Mar 8, 2022

graceguo-supercat Mar 8, 2022 •

edited

Loading

etr2460 Mar 8, 2022

graceguo-supercat Mar 8, 2022

etr2460 Mar 8, 2022

ktmud Mar 8, 2022

graceguo-supercat Mar 8, 2022 •

edited

Loading

graceguo-supercat Mar 8, 2022

ktmud Mar 8, 2022

graceguo-supercat Mar 8, 2022

ktmud Mar 8, 2022

graceguo-supercat Mar 8, 2022

ktmud Mar 8, 2022 •

edited

Loading

graceguo-supercat Mar 8, 2022 •

edited

Loading

ktmud Mar 8, 2022 •

edited

Loading

ktmud Mar 9, 2022

graceguo-supercat commented Mar 9, 2022

etr2460 left a comment

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

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

Conversation

graceguo-supercat commented Mar 4, 2022

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 4, 2022 • edited Loading

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

ktmud Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

graceguo-supercat Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

graceguo-supercat Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

ktmud Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Mar 9, 2022

etr2460 left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2022 •

edited

Loading

ktmud Mar 4, 2022 •

edited

Loading

graceguo-supercat commented Mar 7, 2022 •

edited

Loading

graceguo-supercat Mar 8, 2022 •

edited

Loading

graceguo-supercat Mar 8, 2022 •

edited

Loading

ktmud Mar 8, 2022 •

edited

Loading

graceguo-supercat Mar 8, 2022 •

edited

Loading

ktmud Mar 8, 2022 •

edited

Loading