feat(plugin-chart-table): Implement showing totals #1034

kgabryje · 2021-03-30T09:28:33Z

Implement sending an additional query for total aggregations of metrics. Can be toggled with a checkbox in "Data" tab. Totals are displayed as a sticky row at the bottom of the table. Totals row is not affected by paging or sorting.

Additionaly, this PR contains a fix of an import in superset-ui-core.

Screen.Recording.2021-03-30.at.10.05.27.mov

CC: @villebro @ktmud @junlincc

vercel · 2021-03-30T09:28:37Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/5AKiPUPLrY8UE6tFJpjyGgbCY7sC
✅ Preview: https://superset-ui-git-fork-kgabryje-feat-table-chart-totals-superset.vercel.app

codecov · 2021-03-30T09:41:18Z

Codecov Report

Merging #1034 (c3e0d7e) into master (4e9ec36) will decrease coverage by 0.08%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
- Coverage   27.74%   27.65%   -0.09%     
==========================================
  Files         428      428              
  Lines        8758     8797      +39     
  Branches     1314     1335      +21     
==========================================
+ Hits         2430     2433       +3     
- Misses       6155     6183      +28     
- Partials      173      181       +8

Impacted Files	Coverage Δ
...et-ui-chart-controls/src/shared-controls/index.tsx	`38.04% <0.00%> (-0.42%)`	⬇️
...ins/plugin-chart-table/src/DataTable/DataTable.tsx	`40.44% <0.00%> (-0.93%)`	⬇️
...ugin-chart-table/src/DataTable/hooks/useSticky.tsx	`4.34% <0.00%> (-0.66%)`	⬇️
plugins/plugin-chart-table/src/Styles.tsx	`100.00% <ø> (ø)`
plugins/plugin-chart-table/src/controlPanel.tsx	`0.00% <0.00%> (ø)`
plugins/plugin-chart-table/src/TableChart.tsx	`46.07% <22.22%> (-3.93%)`	⬇️
...chart-controls/src/shared-controls/dndControls.tsx	`28.57% <25.00%> (-0.60%)`	⬇️
plugins/plugin-chart-table/src/transformProps.ts	`61.62% <25.00%> (-4.63%)`	⬇️
plugins/plugin-chart-table/src/buildQuery.ts	`60.00% <50.00%> (-1.71%)`	⬇️
...lugins/plugin-chart-table/src/utils/formatValue.ts	`64.28% <50.00%> (-2.39%)`	⬇️
... and 1 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 4e9ec36...c3e0d7e. Read the comment docs.

villebro

Exciting stuff! Looks good, just a few small comments

villebro · 2021-03-30T09:30:31Z

packages/superset-ui-chart-controls/src/constants.ts

@@ -17,7 +17,7 @@
 * under the License.
 */
 import { t, QueryMode } from '@superset-ui/core';
-import { DTTM_ALIAS } from '@superset-ui/core/src/query/buildQueryObject';
+import { DTTM_ALIAS } from '@superset-ui/core/lib/query/buildQueryObject';


While I think we're slowly moving away from a dedicated temporal column, we might want to consider exporting this on superset-ui/core

It's a hotfix that doesn't really belong in this PR, so I went for "low effort" solution. But you're right, I'll do it properly 🙂

Thanks for catching this. I agree it should be exported in @superset-ui/core properly.

Let me fix this in a separate PR as this is preventing apache/superset#13758 to go through.

villebro · 2021-03-30T10:57:11Z

plugins/plugin-chart-table/src/DataTable/DataTable.tsx

+            <td colSpan={totalsHeaderSpan}>Totals</td>
+            {columnsMeta.slice(totalsHeaderSpan).map(column => (
+              <td className={column.dataType === GenericDataType.NUMERIC ? 'dt-metric' : ''}>
+                {(totals as Record<string, any>)[column.key]}


Here we'd probably need to apply the same number formatter as in the actual cells:

villebro · 2021-03-30T11:01:40Z

plugins/plugin-chart-table/src/DataTable/DataTable.tsx

+      {totals && (
+        <tfoot>
+          <tr key="totals" className="dt-totals">
+            <td colSpan={totalsHeaderSpan}>Totals</td>


missing translation

Suggested change

<td colSpan={totalsHeaderSpan}>Totals</td>

<td colSpan={totalsHeaderSpan}>{t('Totals')}</td>

villebro · 2021-03-30T11:06:06Z

plugins/plugin-chart-table/src/DataTable/DataTable.tsx

@@ -193,6 +199,8 @@ export default function DataTable<D extends object>({
    return (wrapStickyTable ? wrapStickyTable(getNoResults) : getNoResults()) as JSX.Element;
  }

+  const totalsHeaderSpan = totals && columnsMeta.length - Object.keys(totals).length;


Here we need to remove percentage metrics, otherwise regular metrics won't show up:

Suggested change

const totalsHeaderSpan = totals && columnsMeta.length - Object.keys(totals).length;

const totalsHeaderSpan =

totals && columnsMeta.filter(col => !col.isPercentMetric).length - Object.keys(totals).length;

before:

after:

ktmud

Exciting about this feature, too! Thanks for the quick turn around.

I'd assume there will be an accompanying Superset PR that updates the query action to run SQL with totals? Could you link it here when it's ready?

ktmud · 2021-03-30T15:39:34Z

plugins/plugin-chart-table/src/DataTable/DataTable.tsx


 export interface DataTableProps<D extends object> extends TableOptions<D> {
  tableClassName?: string;
+  columnsMeta: DataColumnMeta[];


Please don't add @superset-ui specific stuff to DataTable. We should strive for handling all these in the TableChart . The goal was to have DataTable be a standalone package that can be used outside of Superset, too.

ktmud · 2021-03-30T15:40:40Z

plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx

@@ -134,7 +142,9 @@ function StickyWrap({
  }, [thead]);

  const theadRef = useRef<HTMLTableSectionElement>(null); // original thead for layout computation
+  const tfootRef = useRef<HTMLTableSectionElement>(null); // original thead for layout computation


Suggested change

const tfootRef = useRef<HTMLTableSectionElement>(null); // original thead for layout computation

const tfootRef = useRef<HTMLTableSectionElement>(null); // original tfoot for layout computation

ktmud · 2021-03-30T15:41:26Z

plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx

+      innerWidth: widths.reduce(sum),
+      scrollBarSize,
+    });
+    // real container height, include table header and space for


Suggested change

// real container height, include table header and space for

// real container height, include table header, footer and space for

ktmud · 2021-03-30T15:45:47Z

plugins/plugin-chart-table/src/DataTable/hooks/useSticky.tsx

@@ -242,10 +256,26 @@ function StickyWrap({
      </div>
    );

+    footerTable = (


Should this be

Suggested change

footerTable = (

footerTable = tfoot && (

?

ktmud · 2021-03-30T15:49:37Z

plugins/plugin-chart-table/src/buildQuery.ts

+        row_offset: 0,
+        post_processing: [],
+      });
+    }


I think we should only run extra queries when backend pagination is enabled. The totals should also respect row_limit because otherwise you will get a table with incorrect sums and no way to browse remaining rows.

It's also possible that users who used row limit may only want to see the totals of the top n rows.

I looked into different ways of restricting the total to the row_limit, but it gets pretty ugly pretty quickly. Based on my quick experimentation one would either have to wrap the query in a subquery, but then the aggregate would need to be reaggregated (SELECT AVG("AVG(col2)") from (SELECT col1, AVG(col2) as "AVG(col2)" GROUP BY col1 LIMIT 10)). Alternatively the raw value would be left unaggregated in the subquery and that would work just fine for simple metrics (SELECT AVG("col2") from (SELECT col1, col2 LIMIT 10)), but then that wouldn't work for custom SQL metrics without parsing. So I think for now we should just add a good disclaimer in the description that the total applies to all rows despite the row limit, and then let the users limit the query with filters.

Thank you Ville! I added a more descriptive tooltip to the totals checkbox - "Show total aggregations of selected metrics. Note that row limit does not apply to the result." CC @ktmud @junlincc

I mean, for tables without backend pagination, can we maybe just not send the "total" query and compute the sums in the frontend?

I'm not sure we can assume all aggregations are additive - if there is an average, min, max, percentile or any other non-additive aggregation then displaying a sum will be confusing. For this reason I think the only really comprehensive solution is to perform the aggregation in the database where the cells are calculated.

Unless we decide to always show totals as sums (which might not be what the user expects), we'd need to send all records in a response to correctly calculate totals like average or standard deviation on frontend. In that case, I think sending an additional query and performing the calculations on backend is a "lesser evil"

That makes sense. Just checked and it seems Tableau also has the totals on disaggregated data as the default, but provides an option to calculate a so-called "two-pass total". I can see if we want to implement this, the ColumnConfigControl would come in handy.

ktmud · 2021-03-30T15:52:45Z

packages/superset-ui-chart-controls/src/constants.ts

@@ -17,7 +17,7 @@
 * under the License.
 */
 import { t, QueryMode } from '@superset-ui/core';
-import { DTTM_ALIAS } from '@superset-ui/core/src/query/buildQueryObject';
+import { DTTM_ALIAS } from '@superset-ui/core/lib/query/buildQueryObject';


Let me fix this in a separate PR as this is preventing apache/superset#13758 to go through.

junlincc · 2021-03-30T19:14:17Z

as these are aggregated in the database, they will be comparable to the cell values. For instance, if we have sum(col), avg(col) and stddev(col) as metrics, the first will be additive, but the second and third won’t but will display the total average and standard deviation respectively.

moving conversation to open. @villebro please educate me, what do you mean by aggregated column is comparable to the cells value?
and by 'total average' you mean average of all cells, instead of sum of all average? sorry for all the dumb questions. 😅

@kgabryje thank you for the great improvement, do you mind adding more details of how total works in different aggregation in the descriptions? we may need to add tooltips here and there, in separate PR.

kgabryje · 2021-04-01T11:01:57Z

what do you mean by aggregated column is comparable to the cells value?
and by 'total average' you mean average of all cells, instead of sum of all average? sorry for all the dumb questions. 😅

@junlincc The total aggregation is an aggregation based on all cell values. So a total of an average is not a sum of average values, but an average of all values. Same goes for other agg functions.
I changed a tooltip in totals checkbox to "Show total aggregations of selected metrics". Do you think that's descriptive enough?

villebro

LGTM!

villebro · 2021-04-01T15:38:56Z

plugins/plugin-chart-table/src/transformProps.ts

-  const rowCount = queriesData?.[1]?.data?.[0]?.rowcount as number;
+  const rowCount = serverPagination
+    ? (queriesData?.[1]?.data?.[0]?.rowcount as number)
+    : queriesData?.[0]?.rowcount;
+  const totals =
+    showTotals &&
+    queryMode === QueryMode.aggregate &&
+    queriesData?.[queriesData.length - 1]?.data[0];


To make finding the correct query easier, I think we should add an optional id field to QueryObject that gets passed to the result. I can open a PR for this.

junlincc · 2021-04-01T16:22:25Z

I changed a tooltip in totals checkbox to "Show total aggregations of selected metrics". Do you think that's descriptive enough?

Yes for now. @mihir174 and I can take it(tooltip) from here. Thank you so much!

ktmud · 2021-04-01T18:21:34Z

plugins/plugin-chart-table/src/transformProps.ts

+    : queriesData?.[0]?.rowcount;
+  const totals =
+    showTotals && queryMode === QueryMode.aggregate
+      ? queriesData?.[queriesData.length - 1]?.data[0]


Can we maybe do more explicit array expansion to avoid all these queriesData?.[xxx]?

const [baseQuery, countQuery, totalQuery] = queriesData || [];

ktmud

LGTM

ktmud · 2021-04-02T17:22:16Z

packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx

+
+export const dnd_timeseries_limit_metric: SharedControlConfig<'DndMetricSelect'> = {
+  type: 'DndMetricSelect',
+  label: t('Sort By'),


Suggested change

label: t('Sort By'),

label: t('Sort by'),

Sentence case

) * feat(plugin-chart-table): implement totals row * Fix typo * Fix totals with percentage metrics * Code review fixes * Use dnd with percentage metrics and sortby controls * Make totals checkbox tooltip more descriptive * Remove console.log * Change totals tooltip * Fix typing error * Use array destructuring * Fix typo

kgabryje requested a review from a team as a code owner March 30, 2021 09:28

pull-request-size bot added the size/L label Mar 30, 2021

vercel bot deployed to Preview March 30, 2021 09:28 View deployment

villebro reviewed Mar 30, 2021

View reviewed changes

ktmud reviewed Mar 30, 2021

View reviewed changes

ktmud mentioned this pull request Mar 30, 2021

fix(chart-controls): export/import DTTM_ALIAS properly #1035

Merged

kgabryje force-pushed the feat/table-chart-totals branch from a2de804 to 5d0a79d Compare March 31, 2021 15:39

vercel bot had a problem deploying to Preview March 31, 2021 15:42 Failure

kgabryje closed this Apr 1, 2021

kgabryje reopened this Apr 1, 2021

vercel bot had a problem deploying to Preview April 1, 2021 10:59 Failure

vercel bot had a problem deploying to Preview April 1, 2021 11:38 Failure

vercel bot had a problem deploying to Preview April 1, 2021 15:11 Failure

kgabryje force-pushed the feat/table-chart-totals branch from 840c438 to 023793e Compare April 1, 2021 15:27

vercel bot had a problem deploying to Preview April 1, 2021 15:27 Failure

villebro approved these changes Apr 1, 2021

View reviewed changes

vercel bot deployed to Preview April 1, 2021 16:04 View deployment

ktmud reviewed Apr 1, 2021

View reviewed changes

kgabryje added 7 commits April 2, 2021 11:44

feat(plugin-chart-table): implement totals row

f7a5813

Fix typo

426af1b

Fix totals with percentage metrics

6ef9348

Code review fixes

00a336d

Use dnd with percentage metrics and sortby controls

3551b6e

Make totals checkbox tooltip more descriptive

a7bbb82

Remove console.log

efba16e

kgabryje added 3 commits April 2, 2021 11:44

Change totals tooltip

bf3d103

Fix typing error

3c95100

Use array destructuring

a3aafc1

kgabryje force-pushed the feat/table-chart-totals branch from 89e7469 to a3aafc1 Compare April 2, 2021 09:49

vercel bot deployed to Preview April 2, 2021 09:49 View deployment

ktmud approved these changes Apr 2, 2021

View reviewed changes

ktmud reviewed Apr 2, 2021

View reviewed changes

Fix typo

c3e0d7e

vercel bot deployed to Preview April 6, 2021 10:34 View deployment

kgabryje merged commit 68a5e00 into apache-superset:master Apr 6, 2021

villebro mentioned this pull request Apr 9, 2021

fix(viz): bump superset-ui 0.17.30 apache/superset#14045

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(plugin-chart-table): Implement showing totals #1034

feat(plugin-chart-table): Implement showing totals #1034

kgabryje commented Mar 30, 2021

vercel bot commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 30, 2021 •

edited

Loading

villebro left a comment

villebro Mar 30, 2021

kgabryje Mar 30, 2021

ktmud Mar 30, 2021

ktmud Mar 30, 2021

villebro Mar 30, 2021

villebro Mar 30, 2021

villebro Mar 30, 2021

ktmud left a comment

ktmud Mar 30, 2021

kgabryje Apr 1, 2021

ktmud Mar 30, 2021

ktmud Mar 30, 2021

ktmud Mar 30, 2021

ktmud Mar 30, 2021

villebro Apr 1, 2021 •

edited

Loading

kgabryje Apr 1, 2021

ktmud Apr 1, 2021

villebro Apr 1, 2021

kgabryje Apr 2, 2021

ktmud Apr 2, 2021

ktmud Mar 30, 2021

junlincc commented Mar 30, 2021

kgabryje commented Apr 1, 2021

villebro left a comment

villebro Apr 1, 2021

junlincc commented Apr 1, 2021

ktmud Apr 1, 2021

kgabryje Apr 2, 2021

ktmud left a comment

ktmud Apr 2, 2021

	<td colSpan={totalsHeaderSpan}>Totals</td>
	<td colSpan={totalsHeaderSpan}>{t('Totals')}</td>

	const totalsHeaderSpan = totals && columnsMeta.length - Object.keys(totals).length;
	const totalsHeaderSpan =
	totals && columnsMeta.filter(col => !col.isPercentMetric).length - Object.keys(totals).length;

	const tfootRef = useRef<HTMLTableSectionElement>(null); // original thead for layout computation
	const tfootRef = useRef<HTMLTableSectionElement>(null); // original tfoot for layout computation

	// real container height, include table header and space for
	// real container height, include table header, footer and space for

feat(plugin-chart-table): Implement showing totals #1034

feat(plugin-chart-table): Implement showing totals #1034

Conversation

kgabryje commented Mar 30, 2021

vercel bot commented Mar 30, 2021 • edited Loading

codecov bot commented Mar 30, 2021 • edited Loading

Codecov Report

villebro left a 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

ktmud left a 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

villebro Apr 1, 2021 • 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

Choose a reason for hiding this comment

junlincc commented Mar 30, 2021

kgabryje commented Apr 1, 2021

villebro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junlincc commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 30, 2021 •

edited

Loading

villebro Apr 1, 2021 •

edited

Loading