[Lens] New summary row feature for datatable #101075

dej611 · 2021-06-01T17:21:50Z

Summary

Fixes #84558

This PR adds the summary row feature to the Lens datatable visualization.

List of features:

compute sum, average, count, minimum, maximum for numeric values
- each column can set its own function
- strict number validation of columns (field meta + actual rows data)
- works with transposed columns
- Formatters are inherited from columns
- count uses the default numeric formatter
- Column alignment configuration is used
do not export the summary row in CSV
No dynamic coloring for summary rows

Lens editor:

Transposed columns:

Last value is not supported yet

Open questions:

`Last value` (and similarly `Intervals` custom ranges)

Initially I've also added the count + unique_count for string columns, but had to remove it due to issues with dimension transitions.
I think there's some potential in having them, but I guess that for V1 is ok to not have it.

In terms of implementation detail, I think, the visualization ought to know a bit more meta information about the dimension update, in order to reset the right options, or it is pretty easy to land on inconsistent states. This was the issue I faced when transitioning from one operation to another.

What is `Count` counting?

Currently count takes into account only rows with actual data, ignoring empty rows. Is that ok?

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

flash1293 · 2021-06-02T09:15:24Z

This looks pretty good! For transposed columns, space per column gets tight quickly (we have the same issue with the headers). I'm fine with solving this along with the headers though. Some tools seem to move the summary label to the non-transposed column and only the show the value per transposed column, but it would probably not work well in all cases.

I think avoiding summary rows for string columns in the first version is a good call as it adds quite a bit of complexity and seems like an edge case. Let's keep it simple as long as we can.

Can we rename Count to Value count and make sure it's handling last value array values in the expected way? Just to not re-use a name that has another meaning in the same UI.

dej611 · 2021-06-03T12:23:17Z

@elasticmachine merge upstream

dej611 · 2021-06-03T12:24:41Z

Renamed Count => Values count
Enabled empty string as valid label string
Configuration override when transitioning to invalid states (string columns)

flash1293

This looks almost good to me, just some uncertainty around the handling of array values. It looks like right now we don't treat the current column as "summarizeable" if there are array values, but it seems like it's a small addition to do so and remove an element of surprise ("why is the summary row disappearing if I change my time range"?). If this is complex to do I think we can also leave it like it works right now.

flash1293 · 2021-06-03T15:18:56Z

x-pack/plugins/lens/public/datatable_visualization/visualization.tsx

@@ -358,6 +361,8 @@ export const getDatatableVisualization = ({
                reverse: false, // managed at UI level
              };

+              const HasNoSummaryRow = column.summaryRow == null || column.summaryRow === 'none';


nit: hasNoSummaryRow

flash1293 · 2021-06-03T15:19:55Z

x-pack/plugins/lens/public/datatable_visualization/utils.ts

+    isNumeric &&
+    currentData?.rows.every(({ [accessor]: value }) => typeof value === 'number' || value == null);
+
+  return { isNumeric, hasAllNumericValues: hasFieldNumericValues };


Nit: This is causing the summary row to disappear completely from table and config flyout if there is an array value (which can happen because of last value). Did this happen on purpose? Or should we treat each array value as a separate value in terms of summary calculation?

Maybe this function could workout whether numeric arrays are available as well and report that.
I planned to use this function also for other feature checks as well where non-numeric columns may lead to issues.

On the other hand, considering numeric arrays as valid numeric values may be a good solution. In this case I'd probably show a warning about the presence of array values and compute them as valid values.

…lens/total-row-table

dej611 · 2021-06-03T17:26:25Z

Added support for array numeric values (Last value)
- When array values are detected a warning sign is shown with some explanation on how array values are handled

flash1293 · 2021-06-04T07:06:21Z

@elasticmachine merge upstream

flash1293 · 2021-06-04T07:10:02Z

The test failure looks like a real problem - maybe the test above has to close the flyout? Looks like the color menu is still open:

I'm not sure whether we need a warning here - the behavior seems intuitive to me. It's what I would expect to happen, so I don't think we need to make people aware. What do you think?

dej611 · 2021-06-04T08:14:25Z

The test failure looks like a real problem - maybe the test above has to close the flyout? Looks like the color menu is still open:

I'm not sure whether we need a warning here - the behavior seems intuitive to me. It's what I would expect to happen, so I don't think we need to make people aware. What do you think?

Yes, I did expect it some failure yesterday. I had problems to run the functional tests locally yesterday so I pushed to have a report here 😅

flash1293 · 2021-06-04T11:42:40Z

I'm not sure whether we need a warning here - the behavior seems intuitive to me. It's what I would expect to happen, so I don't think we need to make people aware. What do you think?

@dej611 Did you miss this comment?

dej611 · 2021-06-04T12:21:39Z

Yes I saw it, but when attempting to do it I realised it may be non so intuitive to compute the summaries on arrays.
I went for the most complete solution here. I think it's worth having the warning message.

flash1293 · 2021-06-04T14:40:12Z

non so intuitive to compute the summaries on arrays

Can you explain why? cc @ghudgins - I think it's too scary of a tool for the situation.

dej611 · 2021-06-04T15:51:43Z

Removed warning message in case of array values

elasticmachine · 2021-06-04T15:53:18Z

Pinging @elastic/kibana-app (Team:KibanaApp)

dej611 · 2021-06-07T08:14:20Z

@elasticmachine merge upstream

flash1293

Seems like there is some leftover logic which is causing a bug

flash1293 · 2021-06-07T12:25:11Z

x-pack/plugins/lens/public/datatable_visualization/utils.ts

+    const value = row[accessor];
+    hasFieldOnlyNumberValues = hasFieldOnlyNumberValues && isValidNumber(value);
+    hasFieldNumericValues =
+      (hasFieldNumericValues && hasFieldOnlyNumberValues) ||


This logic is not correct - if there are arrays in the current column and the last row doesn't have a value, hasFieldNumericValues will become false: (first FEMALE column doesn't have a summary because the last row is undefined)

Also I found this logic extremely hard to follow and it seems like it's not used anywhere - hasNumberValues is never accessed and it seems like this function is only returning a complex object because of this and can be simplified to this for all consumers:

export function isNumericField(currentData: Datatable | undefined, accessor: string) { return currentData?.rows.every(row => { const val = row[accessor]; return isValidNumber(val) || Array.isArray(val) && val.every(isValidNumber); }); }

The specific distinction in this function was propaedeutical for more than this specific case, that's the reason it is in the root folder.
I've built on top of this PR a fix for dynamic coloring using this utility.

I've reworked it a bit to fix this specific issue and speed up the computation in case it's not valid. In case you think it's best to have something lighter and more feature specific I can rewrite it as well.

If we don't know whether we are ever going to need it, please let's remove it for now. I only found this issue by chance because the data I was testing with had that specific case.

dej611 · 2021-06-07T16:37:08Z

Just noticed that the Summary row is using a different component than another "select-like" field as Value Format:

Should the Summary row be converted into a ComboBox in this case, or it's ok as is?

dej611 · 2021-06-07T17:11:14Z

Migrated to combobox. If Select is preferred I can always revert the last commit

flash1293 · 2021-06-08T07:54:54Z

Seems like some tests have to be adjusted for combo box

flash1293 · 2021-06-08T12:19:02Z

x-pack/plugins/lens/public/datatable_visualization/summary.ts

+  const summaryValue = computeFinalValue(columnArgs.summaryRow, columnArgs.columnId, table.rows);
+  // ignore the coluymn formatter for the count case
+  if (columnArgs.summaryRow === 'count') {
+    return summaryValue;


can we use the default number formatter here?

Otherwise the values don't get formatted at all:

flash1293 · 2021-06-08T12:20:57Z

x-pack/plugins/lens/public/datatable_visualization/components/dimension_editor.tsx

+              options={getSummaryRowOptions()}
+              selectedOptions={[
+                {
+                  label: fallbackSummaryLabel,


This logic is picking up the user supplied summary label as the selected summary function:

Especially confusing for no label:

flash1293 · 2021-06-08T12:22:26Z

x-pack/plugins/lens/public/datatable_visualization/components/table_basic.tsx

+        summaryRowValue: config.summaryRowValue,
+        ...getFinalSummaryConfiguration(config.columnId, config, firstTable),
+      }))
+      .filter(({ summaryRow }) => summaryRow !== 'none');


If the only column with a summary row is hidden, it will show a weird empty row (note the thin gray line below the thick black one):

Should be easy to fix take into account the hidden flag and filter columnsWithSummary

dej611 · 2021-06-08T13:06:23Z

Addressed the reported issues w/ unit tests to cover them:

kibanamachine · 2021-06-08T15:06:57Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	670	672	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.3MB	1.3MB	+6.7KB

History

💚 Build #129706 succeeded cf47e1d
💔 Build #129696 failed 8ebeede
💔 Build #129559 failed 36b9745
💚 Build #129402 succeeded 7b664c9
💚 Build #129246 succeeded 842add2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

flash1293

LGTM, great work!

* ✨ New summary row feature for datatable * ✨ Allow empty strings behind flag + tests * 🐛 Address the transition problem + refactor * ✅ Add some unit tests * ✅ Add first functional tests * 👌 first feedback addressed * ✨ Make it handle numeric array values * 📝 Improved message * ✅ Fix functional test * 🔥 Remove warning message for last value * 🚨 Remove unused import * 🐛 Fix a bug with last value * 👌 Integrated feedback * 💄 Migrated to combobox * ✅ Fix unit tests + restore right data-test-id * 🏷️ Fix type issue * 👌 Address all issues reported Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-09T12:30:40Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* ✨ New summary row feature for datatable * ✨ Allow empty strings behind flag + tests * 🐛 Address the transition problem + refactor * ✅ Add some unit tests * ✅ Add first functional tests * 👌 first feedback addressed * ✨ Make it handle numeric array values * 📝 Improved message * ✅ Fix functional test * 🔥 Remove warning message for last value * 🚨 Remove unused import * 🐛 Fix a bug with last value * 👌 Integrated feedback * 💄 Migrated to combobox * ✅ Fix unit tests + restore right data-test-id * 🏷️ Fix type issue * 👌 Address all issues reported Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

* master: clarify which parts of TM are experimental (elastic#101757) Add sh scripts with _bulk_action route usage examples (elastic#101736) [Uptime] Only register route in side nav if uptime show capability is true (elastic#101709) Use KIBANA_DOCS in doc link service (elastic#101667) [Alerting][Event log] Persisting duration information for active alerts in event log (elastic#101387) Address design issues in Discover/Graph (elastic#101584) Optimize performance for document table (elastic#101715) Change file data visualizer links to point to new location in home application (elastic#101393) [Fleet] Tighten policy permissions, take II (elastic#97366) [ML] Add debounce to the severity control update (elastic#101581) [Fleet] Fix routing issues with `getPath` and `history.push` (elastic#101658) [APM] Add link-to/transaction route (elastic#101731) [Index Patterns] Runtime fields CRUD REST API (elastic#101164) [ILM] Refactor types and fix missing aria labels (elastic#101518) [Lens] New summary row feature for datatable (elastic#101075) Blocks save event filter with a white space name (elastic#101599) Improve security server types (elastic#101661) [APM] Replace side nav with tabs on Settings page (elastic#101460) [APM] Only register items in side nav if user has permissions to see app (elastic#101707) [Security solution][Endpoint] Add back button when to the event filters list (elastic#101280)

✨ New summary row feature for datatable

2b39b45

dej611 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 1, 2021

wylieconlon changed the title ~~[Lens]] New summary row feature for datatable~~ [Lens] New summary row feature for datatable Jun 1, 2021

dej611 added 2 commits June 3, 2021 14:19

✨ Allow empty strings behind flag + tests

0ec6e9c

🐛 Address the transition problem + refactor

e684272

Merge branch 'master' into lens/total-row-table

a64d921

✅ Add some unit tests

0e14bd4

flash1293 reviewed Jun 3, 2021

View reviewed changes

dej611 added 5 commits June 3, 2021 17:34

✅ Add first functional tests

79e715b

Merge branch 'lens/total-row-table' of github.com:dej611/kibana into …

bc58cd4

…lens/total-row-table

👌 first feedback addressed

cea3233

✨ Make it handle numeric array values

48e85c3

📝 Improved message

743606a

Merge branch 'master' into lens/total-row-table

d7c6946

✅ Fix functional test

9417ccc

🔥 Remove warning message for last value

678fd95

dej611 marked this pull request as ready for review June 4, 2021 15:53

dej611 requested a review from a team June 4, 2021 15:53

🚨 Remove unused import

842add2

Merge branch 'master' into lens/total-row-table

7b664c9

flash1293 requested changes Jun 7, 2021

View reviewed changes

dej611 added 2 commits June 7, 2021 15:34

🐛 Fix a bug with last value

bd069e4

👌 Integrated feedback

9a0e2a1

💄 Migrated to combobox

36b9745

dej611 added 3 commits June 8, 2021 10:27

✅ Fix unit tests + restore right data-test-id

8ebeede

Merge remote-tracking branch 'upstream/master' into lens/total-row-table

4056906

🏷️ Fix type issue

cf47e1d

flash1293 requested changes Jun 8, 2021

View reviewed changes

👌 Address all issues reported

77a4017

flash1293 approved these changes Jun 9, 2021

View reviewed changes

dej611 merged commit e3c3b6f into elastic:master Jun 9, 2021

dej611 deleted the lens/total-row-table branch June 9, 2021 12:27

kibanamachine mentioned this pull request Jun 9, 2021

[7.x] [Lens] New summary row feature for datatable (#101075) #101740

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] New summary row feature for datatable #101075

[Lens] New summary row feature for datatable #101075

dej611 commented Jun 1, 2021 •

edited

Loading

flash1293 commented Jun 2, 2021

dej611 commented Jun 3, 2021

dej611 commented Jun 3, 2021

flash1293 left a comment

flash1293 Jun 3, 2021

flash1293 Jun 3, 2021

dej611 Jun 3, 2021

dej611 commented Jun 3, 2021

flash1293 commented Jun 4, 2021

flash1293 commented Jun 4, 2021

dej611 commented Jun 4, 2021

flash1293 commented Jun 4, 2021

dej611 commented Jun 4, 2021 •

edited

Loading

flash1293 commented Jun 4, 2021

dej611 commented Jun 4, 2021 •

edited

Loading

elasticmachine commented Jun 4, 2021

dej611 commented Jun 7, 2021

flash1293 left a comment

flash1293 Jun 7, 2021

dej611 Jun 7, 2021

flash1293 Jun 7, 2021

dej611 commented Jun 7, 2021

dej611 commented Jun 7, 2021 •

edited

Loading

flash1293 commented Jun 8, 2021

flash1293 Jun 8, 2021 •

edited

Loading

flash1293 Jun 8, 2021

flash1293 Jun 8, 2021

dej611 commented Jun 8, 2021 •

edited

Loading

kibanamachine commented Jun 8, 2021

flash1293 left a comment

kibanamachine commented Jun 9, 2021

[Lens] New summary row feature for datatable #101075

[Lens] New summary row feature for datatable #101075

Conversation

dej611 commented Jun 1, 2021 • edited Loading

Summary

Open questions:

Last value (and similarly Intervals custom ranges)

What is Count counting?

Checklist

For maintainers

flash1293 commented Jun 2, 2021

dej611 commented Jun 3, 2021

dej611 commented Jun 3, 2021

flash1293 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

dej611 commented Jun 3, 2021

flash1293 commented Jun 4, 2021

flash1293 commented Jun 4, 2021

dej611 commented Jun 4, 2021

flash1293 commented Jun 4, 2021

dej611 commented Jun 4, 2021 • edited Loading

flash1293 commented Jun 4, 2021

dej611 commented Jun 4, 2021 • edited Loading

elasticmachine commented Jun 4, 2021

dej611 commented Jun 7, 2021

flash1293 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

dej611 commented Jun 7, 2021

dej611 commented Jun 7, 2021 • edited Loading

flash1293 commented Jun 8, 2021

flash1293 Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Jun 8, 2021 • edited Loading

kibanamachine commented Jun 8, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

flash1293 left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 9, 2021

💚 Backport successful

dej611 commented Jun 1, 2021 •

edited

Loading

`Last value` (and similarly `Intervals` custom ranges)

What is `Count` counting?

dej611 commented Jun 4, 2021 •

edited

Loading

dej611 commented Jun 4, 2021 •

edited

Loading

dej611 commented Jun 7, 2021 •

edited

Loading

flash1293 Jun 8, 2021 •

edited

Loading

dej611 commented Jun 8, 2021 •

edited

Loading