Ensure extra columns are not shown in table vis when showPartialRows:true

[lukeelmers]

Closes #22168

Summary

Resolves a regression where clicking show partial rows on the data table vis would automatically render columns with metrics at all levels -- even if you didn't check metrics at all levels.

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

[lukeelmers]

Since params are being thrown around willy-nilly and hard to track, I added this to reduce confusion for people working on Tabify in the future. (A TypeScript rewrite would be very helpful here).

[lukeelmers]

These are split into separate params, because within table vis there is a distinction between the two. For example, the following is possible:

vis.params.showMetricsAtAllLevels === false;
vis.params.showPartialRows === true;
vis.isHierarchical(); // still returns true

This is due to how we configure the table vis:
https://github.com/elastic/kibana/blob/9b08fc719a9fb4c0c1dce55be73d395e7cd6b54a/src/core_plugins/table_vis/public/table_vis.js#L114-L116

[ppisljar]

this is actually wrong i think ....
hierarchical === metricsAtAllLevels (unless someone knows of any reasons it should be ? )

in courier request handler vis.isHierarchical() is used to figure out if we need to build a request with metrics on every level. (or it was, and its now refactored to look at metricsAtAllLevels parameter). we should use this same parameter in tabify (to figure out the same thing) ...
and partialRows should be completely independent ... as its just removing rows where we don't have value for every column.

[elasticmachine]

Pinging @elastic/kibana-app

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[lukeelmers]

^^ seems to be a legit failure, looking into it

Uncaught TypeError: Cannot read property 'value' of undefined

in build_hierarchical_data.js

[lukeelmers]

Next build should be green 🤞

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[trevan]

I tested this and it looks good.

[lukeelmers]

Thanks for reviewing @trevan! Much appreciated

[ppisljar]

i think this introduces some more confusion with metricsAtAllLevels and isHierarchical ...

hierarchical === metricsAtAllLevels (unless someone knows of any reasons it should be ? )

in courier request handler vis.isHierarchical() is used to figure out if we need to build a request with metrics on every level. (or it was, and its now refactored to look at metricsAtAllLevels parameter). we should use this same parameter in tabify (to figure out the same thing) ...
and partialRows should be completely independent ... as its just removing rows where we don't have value for every column.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[lukeelmers]

@ppisljar I renamed some of the params that are being passed around to reduce confusion.

Rather than using the showMetricsAtAllLevels vis param, which pretty much only exists for data table, I created a columnsForAllBuckets arg for tabify that specifically exists to indicate whether we need to have tabify return a column for each bucket/level.

I moved the logic for checking the existence of params.showMetricsAtAllLevels to the visualize data loader, which feels more appropriate than having tabify concern itself with the details of the vis.

The overall effect is the same, but hopefully this makes its purpose clearer so it doesn't get mixed up with isHierarchical.

EDIT: We also used to have a minimalColumns setting -- this is essentially what I was trying to do with columnsForAllBuckets

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[lukeelmers]

retest

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[lukeelmers]

retest

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[snide]

Don't know why we got pinged on this one. Normally it's if the Pr has Sass files. In any case, nothing looks off by me.

[timroes]

LGTM, left some minor suggestions. @trevan if you want to give this a final review if it satisfies your needs too, please feel free

[trevan]

@timroes, it looks good

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request