feat: Adds sortable table detail page #691

Golodhros · 2020-09-28T17:31:37Z

Summary of Changes

Adds ability to sort the table columns on the Table detail page

Tests

Added tests

CheckList

Make sure you have checked all steps below to ensure a timely review.

PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
PR includes a summary of changes, including screenshots of any UI changes.
PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
In case of new functionality, my PR adds documentation that describes how to use it.
- All the public python functions and the classes in the PR contain docstrings that explain what it does
PR passes all tests documented in the developer guide

amundsen_application/static/js/components/ColumnList/index.tsx

amundsen_application/static/js/pages/TableDetailPage/index.tsx

allisonsuarez · 2020-09-28T21:36:40Z

amundsen_application/static/js/components/common/Table/index.tsx

@@ -8,8 +8,14 @@ import { UpIcon, DownIcon } from '../SVGIcons';

 import './styles.scss';

-type TextAlignmentValues = 'left' | 'right' | 'center';
+// export type SortDirection = 'asc' | 'desc';


Why are these commented out?

I will implement it shortly!

ttannis · 2020-09-28T23:28:27Z

amundsen_application/static/js/components/ColumnList/index.tsx

@@ -153,17 +197,26 @@ const ColumnList: React.FC<ColumnListProps> = ({
        name: item.name,
        database,
      },
-      usage: hasItemStats ? item.stats[0].stat_val : '',
+      sort_order: item.sort_order,
+      usage: hasItemStats ? +item.stats[0].stat_val : null,


I don't think the api guarantees the column usage will be the first statistic, so perhaps we should have a helper method to intentionally search for the column_usage statistic and extract the value.

Good call!
Right now this will work, but as we add more stats it will definitely break.
I will add something now.

+1, currently we have single stat at Lyft, but we will ingest more stat later. I think we should either define the query / metric name in config for sorting or redefine the usage not part of stat. I wonder whether we could choose the former?

ttannis · 2020-09-28T23:31:52Z

amundsen_application/static/js/components/ColumnList/index.tsx

      isEditable: item.is_editable,
      editText,
      editUrl,
      index,
    };
  });
  const statsCount = formattedData.filter((item) => !!item.stats).length;
-  const hasStats = statsCount >= SHOW_STATS_THRESHOLD;
+  const hasStats =


nit: Is this more accurately hasUsage?

Yep, will change

ttannis · 2020-09-28T23:38:07Z

Looks like tests are failing because of amundsencommon? cc @allisonsuarez since I think you were working on it. On my end only concern is my comment about reliable logic to extract the column usage from the stats.

feng-tao · 2020-09-28T23:46:03Z

Looks like tests are failing because of amundsencommon? cc @allisonsuarez since I think you were working on it. On my end only concern is my comment about reliable logic to extract the column usage from the stats.

let me take a look at the code as well. I was raising a question: could we sort based on a metric which defined by usage? We name column usage but other OSS could name it differently.

Also is @ttannis 's concern extracting column usage in stats requiring certain hack? do you think if we put query / column usage as separate node helps?

Golodhros · 2020-09-29T03:33:43Z

amundsen_application/static/js/components/ColumnList/index.tsx

@@ -76,6 +90,50 @@ type ExpandedRowProps = {
 };

 const SHOW_STATS_THRESHOLD = 1;
+const USAGE_STAT_TYPE = 'column_usage';


This would probably be the item to move to the config, however I am wary of creating that abstraction now, without knowing more about what it is to come.

could you add a todo for moving it to config as follow up as column_usage is just a stat name used internally for query usage?

I can understand that. I am alright with it as it since the community will be able to follow existing examples to make this more flexible if/when needed.

feng-tao · 2020-09-29T16:44:50Z

amundsen_application/static/js/components/ColumnList/index.tsx

@@ -76,6 +90,50 @@ type ExpandedRowProps = {
 };

 const SHOW_STATS_THRESHOLD = 1;
+const USAGE_STAT_TYPE = 'column_usage';


could you add a todo for moving it to config as follow up as column_usage is just a stat name used internally for query usage?

feng-tao · 2020-09-29T16:50:02Z

amundsen_application/static/js/components/ColumnList/index.tsx

+    : stringSortingFunction;
+};
+
+const getUsageStat = (item) => {


it seems sometimes we have type for input and sometimes not?

feng-tao · 2020-09-29T16:50:46Z

amundsen_application/static/js/components/ColumnList/index.tsx

      isEditable: item.is_editable,
      editText,
      editUrl,
      index,
    };
  });
  const statsCount = formattedData.filter((item) => !!item.stats).length;
-  const hasStats = statsCount >= SHOW_STATS_THRESHOLD;
+  const hasUsageStat =
+    getTableSortCriterias().usage && statsCount >= SHOW_STATS_THRESHOLD;


so if the column has 0 usage, it won't show stat?

If no element on the data has usage, we won't show the whole column, right.

feng-tao · 2020-09-29T16:53:13Z

amundsen_application/static/js/config/config-default.ts

+          name: 'Alphabetical',
+          key: 'name',
+          direction: SortDirection.descending,
+        },


if we want to sort the column based on column usage, are we going to add new criteria at here as well?

Yep, that change is on the private repo, only for us right now.

ttannis · 2020-09-29T17:05:40Z

Looks like tests are failing because of amundsencommon? cc @allisonsuarez since I think you were working on it. On my end only concern is my comment about reliable logic to extract the column usage from the stats.

let me take a look at the code as well. I was raising a question: could we sort based on a metric which defined by usage? We name column usage but other OSS could name it differently.

Also is @ttannis 's concern extracting column usage in stats requiring certain hack? do you think if we put query / column usage as separate node helps?

I wouldn't call it a hack, rather extra logic to explicitly search the stats array for column_usage. Having it as a separate node would make it such that we don't need that extra logic, but I still believe your original perspective that column usage is a statistic in the metadata is fair. The case here is that the UI design has chosen to display usage differently from other column statistics, so I think it is appropriate for the frontend to have to have go parse the data because of how our designs have chosen to display them.

In short, no strong opinion on changing it!

ttannis

Pending passing CI checks.

Golodhros · 2020-09-29T17:10:51Z

Looks like tests are failing because of amundsencommon? cc @allisonsuarez since I think you were working on it. On my end only concern is my comment about reliable logic to extract the column usage from the stats.

let me take a look at the code as well. I was raising a question: could we sort based on a metric which defined by usage? We name column usage but other OSS could name it differently.
Also is @ttannis 's concern extracting column usage in stats requiring certain hack? do you think if we put query / column usage as separate node helps?

I wouldn't call it a hack, rather extra logic to explicitly search the stats array for column_usage. Having it as a separate node would make it such that we don't need that extra logic, but I still believe your original perspective that column usage is a statistic in the metadata is fair. The case here is that the UI design has chosen to display usage differently from other column statistics, so I think it is appropriate for the frontend to have to have go parse the data because of how our designs have chosen to display them.

In short, no strong opinion on changing it!

Done!

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Golodhros requested review from allisonsuarez, dikshathakur3119, feng-tao and ttannis as code owners September 28, 2020 17:31

ttannis reviewed Sep 28, 2020

View reviewed changes

amundsen_application/static/js/components/ColumnList/index.tsx Outdated Show resolved Hide resolved

amundsen_application/static/js/pages/TableDetailPage/index.tsx Show resolved Hide resolved

amundsen_application/static/js/pages/TableDetailPage/index.tsx Outdated Show resolved Hide resolved

Golodhros force-pushed the mi-sorting-table branch 2 times, most recently from f05dce2 to b8e9a7c Compare September 28, 2020 20:56

allisonsuarez reviewed Sep 28, 2020

View reviewed changes

ttannis reviewed Sep 28, 2020

View reviewed changes

Golodhros commented Sep 29, 2020

View reviewed changes

feng-tao reviewed Sep 29, 2020

View reviewed changes

ttannis approved these changes Sep 29, 2020

View reviewed changes

Marcos Iglesias added 13 commits September 29, 2020 11:35

Basic usage sorting, TS issues and dummy buttons

bc17264

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Uses monospace font on usage column values

bd6d20a

Basic SortBy dropdown

8e8ab4d

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Sorting working with dropdown control

5fc3eaf

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Updating required sort_order property

88917a1

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Watching for active tab and fixing TypeScript issues

70cce2e

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Parametrizes sort by selector

f096a7b

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Using an enum for column alignement

e915594

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Removing ts-ignore marks

c6adc65

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Adds alphabetical sorting

2587a1b

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Betterer log

fc16be2

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Adds logic for extracting usage information

0084c57

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Adds TODO, solves pointer and closing of popover

467a79a

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>

Golodhros force-pushed the mi-sorting-table branch from ab2a628 to 467a79a Compare September 29, 2020 18:35

Golodhros merged commit 36aebd2 into master Sep 29, 2020

feng-tao deleted the mi-sorting-table branch September 29, 2020 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds sortable table detail page #691

feat: Adds sortable table detail page #691

Golodhros commented Sep 28, 2020 •

edited

Loading

allisonsuarez Sep 28, 2020

Golodhros Sep 28, 2020

ttannis Sep 28, 2020

Golodhros Sep 28, 2020

feng-tao Sep 29, 2020

ttannis Sep 28, 2020

Golodhros Sep 28, 2020

ttannis commented Sep 28, 2020

feng-tao commented Sep 28, 2020

Golodhros Sep 29, 2020

feng-tao Sep 29, 2020

ttannis Sep 29, 2020

feng-tao Sep 29, 2020

feng-tao Sep 29, 2020

feng-tao Sep 29, 2020

Golodhros Sep 29, 2020

feng-tao Sep 29, 2020

Golodhros Sep 29, 2020

ttannis commented Sep 29, 2020

ttannis left a comment •

edited

Loading

Golodhros commented Sep 29, 2020

feat: Adds sortable table detail page #691

feat: Adds sortable table detail page #691

Conversation

Golodhros commented Sep 28, 2020 • edited Loading

Summary of Changes

Tests

CheckList

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

Choose a reason for hiding this comment

ttannis commented Sep 28, 2020

feng-tao commented Sep 28, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttannis commented Sep 29, 2020

ttannis left a comment • edited Loading

Choose a reason for hiding this comment

Golodhros commented Sep 29, 2020

Golodhros commented Sep 28, 2020 •

edited

Loading

ttannis left a comment •

edited

Loading