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

Jump to bottom

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

Changes from 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 20 additions & 8 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 @@ -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') {
Copy link
Member

@ktmud ktmud Mar 4, 2022 •
edited

Choose a reason for hiding this comment

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

This could be simplified as:

return (
  (rowAVal == null) - (rowBVal == null) ||
  Number.isNaN(rowAVal) - Number.isNaN(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.

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

return rowAVal - rowBVal;
}
if (typeof rowAVal === 'number') {
return 1;
}
if (typeof rowBVal === 'number') {
return -1;
}
return 0;
},
})),
],
Expand Down Expand Up @@ -192,14 +201,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