Preserve nested tables in table vis #24377

lukeelmers · 2018-10-22T23:25:24Z

Summary

Resolves a regression introduced by the vis legacy response handler, where table vis was no longer able to display nested tables beyond one level deep when splitting on a table.

This led to two related regressions, both addressed here: date formatting in table titles was lost, and you could no longer use the "column" layout when splitting on a table.

After discussing with @ppisljar, we agreed the best way to approach would be to set up a table-specific response handler that knew how to handle the nesting, as the legacy response handler is now used in other vis types as well. As the new handler only serves a single purpose, it is not registered along with other response handlers; this should only be used with table vis.

Ultimately, this solution is only temporary until 7.0 when we can migrate table vis to handle splitting in a manner consistent with other vis types (i.e. one level of depth when splitting).

TODO: I'd like to add a few unit tests on this use case to avoid future regressions, will try to get those added tomorrow.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] 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
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

src/core_plugins/table_vis/public/table_vis.js

lukeelmers · 2018-10-22T23:29:48Z

src/ui/public/vis/response_handlers/legacy_table.js

+    return moment(parseInt(value)).format('YYYY-MM-DD');
+  }
+  return value;
+}


Someone must have written a util of some type to handle this elsewhere. I grepped around and was unable to find how we were previously formatting these dates in table titles, so added this naïve implementation for now. But I'd greatly appreciate if someone could point me in the right direction so that I can remove this!

lukeelmers · 2018-10-22T23:32:25Z

src/ui/public/vis/response_handlers/legacy_table.js

+        const splitColumnIndex = columns.findIndex(column => column.id === splitColumn.id);
+        const rowsByColumnValue = keyRowsByValue(rows, splitColumn.id);
+        const filteredColumns = columns
+          .filter((column, i) => i !== splitColumnIndex)


In this column filter, I haven't included any of the isSplitMetric logic used in the legacy response handler because it did not appear to be necessary for the tables (I think?) -- but please correct me if I'm missing something.

i think this might not be the case ....
if we have metrics at all levels, the tabified table will look like: (for two nested term aggs, first one on os, second one on countr)

os coiunt country count win xp 230 CN 200 win xp 230 US 30 linux 120 CN 100 linux 120 CN 20

lets say os is the split column .... we should filter out that one + all the matching metrics (so all the metrics until the next bucket column)

@timroes and I were just testing and we think this isn't an issue based on our understanding (it seemed to behave consistently with both legacy and legacy_table handlers), but I will double check

elasticmachine · 2018-10-23T02:36:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar

i might be wrong about my comment, best to test it out ...

another thing that comes to mind is filters on other bucket ... you will need to preserve 'rawData' ..
so a reference to tabify table, the row and column .... take a look at vis/vis.js vis.API.filter method.

src/ui/public/vis/response_handlers/legacy_table.js

ppisljar · 2018-10-23T15:51:30Z

src/ui/public/vis/response_handlers/legacy_table.js

+        const splitColumnIndex = columns.findIndex(column => column.id === splitColumn.id);
+        const rowsByColumnValue = keyRowsByValue(rows, splitColumn.id);
+        const filteredColumns = columns
+          .filter((column, i) => i !== splitColumnIndex)


i think this might not be the case ....
if we have metrics at all levels, the tabified table will look like: (for two nested term aggs, first one on os, second one on countr)

os coiunt country count win xp 230 CN 200 win xp 230 US 30 linux 120 CN 100 linux 120 CN 20

lets say os is the split column .... we should filter out that one + all the matching metrics (so all the metrics until the next bucket column)

elasticmachine · 2018-10-23T21:52:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-23T23:09:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar · 2018-10-24T05:42:38Z

tested it out, as expected doens't work correctly with metricsAtAllLevels and doesn't apply the other bucket filters correctly.

timroes · 2018-10-24T06:22:38Z

@ppisljar Other bucket not working is not related to this PR at all, and wasn't working beforehand either. Could you please be more specific about metricsAtAllLevels is not working? Which steps/configuration did you use and what is the expected outcome?

elasticmachine · 2018-10-24T07:30:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

The new legacy response handler introduced a regression in how nested tables were handled within table vis. This adds a new table-specific response handler to ensure splitting is preserved. This is a short term solution and will be removed once we are able to update table splitting to be consistent with other vis types.

…handler.

…ndler.

lukeelmers · 2018-10-24T19:13:28Z

@ppisljar @timroes split buckets are now correctly hidden with metricsAtAllLevels. i also rebased on latest master which includes a functional test Tim wrote for this exact use case.

Functional tests passed locally so hopefully that means a green build is on its way 🤞

elasticmachine · 2018-10-24T21:28:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2018-10-24T22:09:44Z

retest

elasticmachine · 2018-10-25T00:25:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes

Tested, added tests and looks good to me.

elasticmachine · 2018-10-25T10:46:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Add legacy response handler for table vis. The new legacy response handler introduced a regression in how nested tables were handled within table vis. This adds a new table-specific response handler to ensure splitting is preserved. This is a short term solution and will be removed once we are able to update table splitting to be consistent with other vis types. * Ensure formatted dates are preserved in table titles. * Update legacy table response handler based on feedback. * Ensure AggConfigResult.rawData is preserved in legacy table response handler. * Move legacy table response handler to core_plugins. * Legacy table response handler - style cleanup. * Remove unneeded aggConfigResult.rawData from legacy table response handler. * Add basic unit tests for legacy table response handler. * In table vis, exclude split columns when showing metrics at all levels. * Add functional tests

lukeelmers commented Oct 22, 2018

View reviewed changes

src/core_plugins/table_vis/public/table_vis.js Show resolved Hide resolved

lukeelmers commented Oct 22, 2018

View reviewed changes

lukeelmers requested review from ppisljar and timroes October 22, 2018 23:33

lukeelmers added blocker review regression v6.5.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure bug Fixes for quality problems that affect the customer experience Feature:Data Table Data table visualization feature labels Oct 22, 2018

lukeelmers mentioned this pull request Oct 23, 2018

fixing other bucket filters #24217

Merged

ppisljar requested changes Oct 23, 2018

View reviewed changes

lukeelmers force-pushed the fix/table-nesting branch from 605d2b8 to 766f0df Compare October 23, 2018 18:36

lukeelmers self-assigned this Oct 23, 2018

lukeelmers removed the review label Oct 23, 2018

lukeelmers added 7 commits October 24, 2018 12:49

Ensure formatted dates are preserved in table titles.

559cb63

Update legacy table response handler based on feedback.

23b74f2

Ensure AggConfigResult.rawData is preserved in legacy table response …

1c81fd2

…handler.

Move legacy table response handler to core_plugins.

d56ee34

Legacy table response handler - style cleanup.

7a8919e

Remove unneeded aggConfigResult.rawData from legacy table response ha…

b2aaebc

…ndler.

lukeelmers added 2 commits October 24, 2018 12:49

Add basic unit tests for legacy table response handler.

fd3d309

In table vis, exclude split columns when showing metrics at all levels.

09d9640

lukeelmers force-pushed the fix/table-nesting branch from c90c5c8 to 09d9640 Compare October 24, 2018 19:14

Tim Roes added 2 commits October 25, 2018 08:13

Merge branch 'master' into pr/24377

6ebabba

Add functional tests

589e88d

timroes added the v7.0.0 label Oct 25, 2018

timroes approved these changes Oct 25, 2018

View reviewed changes

timroes merged commit 40c232b into elastic:master Oct 25, 2018

timroes mentioned this pull request Oct 25, 2018

[6.x] Preserve nested tables in table vis (#24377) #24571

Merged

lukeelmers deleted the fix/table-nesting branch October 25, 2018 14:13

timroes mentioned this pull request Oct 25, 2018

Rewrite table visualization in React/EUI #16639

Closed

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve nested tables in table vis #24377

Preserve nested tables in table vis #24377

lukeelmers commented Oct 22, 2018 •

edited

Loading

lukeelmers Oct 22, 2018

lukeelmers Oct 22, 2018 •

edited

Loading

ppisljar Oct 23, 2018

lukeelmers Oct 23, 2018

elasticmachine commented Oct 23, 2018

ppisljar left a comment

ppisljar Oct 23, 2018

elasticmachine commented Oct 23, 2018

elasticmachine commented Oct 23, 2018

ppisljar commented Oct 24, 2018

timroes commented Oct 24, 2018

elasticmachine commented Oct 24, 2018

lukeelmers commented Oct 24, 2018

elasticmachine commented Oct 24, 2018

lukeelmers commented Oct 24, 2018

elasticmachine commented Oct 25, 2018

timroes left a comment

elasticmachine commented Oct 25, 2018

Preserve nested tables in table vis #24377

Preserve nested tables in table vis #24377

Conversation

lukeelmers commented Oct 22, 2018 • edited Loading

Summary

Checklist

For maintainers

lukeelmers Oct 22, 2018

Choose a reason for hiding this comment

lukeelmers Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

ppisljar Oct 23, 2018

Choose a reason for hiding this comment

lukeelmers Oct 23, 2018

Choose a reason for hiding this comment

elasticmachine commented Oct 23, 2018

💚 Build Succeeded

ppisljar left a comment

Choose a reason for hiding this comment

ppisljar Oct 23, 2018

Choose a reason for hiding this comment

elasticmachine commented Oct 23, 2018

💚 Build Succeeded

elasticmachine commented Oct 23, 2018

💚 Build Succeeded

ppisljar commented Oct 24, 2018

timroes commented Oct 24, 2018

elasticmachine commented Oct 24, 2018

💚 Build Succeeded

lukeelmers commented Oct 24, 2018

elasticmachine commented Oct 24, 2018

💔 Build Failed

lukeelmers commented Oct 24, 2018

elasticmachine commented Oct 25, 2018

💚 Build Succeeded

timroes left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 25, 2018

💚 Build Succeeded

lukeelmers commented Oct 22, 2018 •

edited

Loading

lukeelmers Oct 22, 2018 •

edited

Loading