[Lens] Suggest reduced versions of current data table

[flash1293]

This PR adds support for also suggesting reduced versions of the current data table. This makes it possible to switch from a data table with more than 2 splits to an xy charts with some of the splits dropped.

The logic for reducing the columns is:

• Start throwing away bucket columns from right to left
• Suggest a table with all metric columns and just the first metric columns for each version of reduced bucket columns

It also unifies the way suggestions are built because we know have three places that use them (workspace panel, suggestion panel and chart switch)

[elasticmachine]

💚 Build Succeeded

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

[wylieconlon]

I'm getting into a loop that I think involves layers not getting removed or some kind of race condition. Here are the steps to reproduce:

1. Create two layers with 2 columns each (drag 2 fields in)
2. Switch to the data table
3. The data loss indicator shows up on all the XY charts
4. After switching back to an XY chart, the loss indicator shows on the table visualization

Another issue that is not directly related to this PR is that our suggestion titles are really hard to understand. I think the solution to this to hide the axis scale, but to show a label along the X & Y axes with a title like "Split by "

[flash1293]

Good catch @wylieconlon The race condition here was that the SWITCH_VISUALIZATION action was overriding the datasource state changes made by the remove layers call. I switched them around and restructured the code a bit to make it clearer which layers are kept.

[elasticmachine]

💚 Build Succeeded

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

[chrisdavies]

LGTM w/ possible tweaks. I think that 100% of my feedback is nit-picky and ignorable, so have a look and decide for yourself on that! I like your placement of comments around the tricker bits of code. Also, I learned a new thing:

'datasourceId' in action && action.datasourceId

I think that's much nicer than what I've been doing:

(action as { datasourceId: number }).datasourceId

[chrisdavies]

In my metric PR, it became pretty clear that things would be a lot easier if we actually stored the ids on the objects (e.g. datasource.id, layer.id, etc) rather than only storing them in the keys of a dictionary. I wonder if we should make that change at some point?

Anyway, this is a nit, so take it or leave it, but you're essentially doing the filter twice (line 56 and line 66). The whole thing would be tidier (I think) like this:

const datasources = Object.entries(datasourceMap).map(([id, datasource]) => ({ id, ...datasource }))
  .filter(({ id }) => datasourceStates[id] && !datasourceStates[id].isLoading);

const allLayerIds = _(datasources)
  .map(datasource => datasource.getLayers(datasourceStates[datasource.id].state))
  .flatten()
  .value();

const datasourceTableSuggestions = _(datasources)
  .map((datasource, datasourceIndex) => {
    const datasourceState = datasourceStates[datasource.id].state;
    const suggestions = (field
      ? datasource.getDatasourceSuggestionsForField(datasourceState, field)
      : datasource.getDatasourceSuggestionsFromCurrentState(datasourceState)
    );

    // TODO have the datasource in there by default
    return suggestions.map((suggestion, suggestionIndex) => ({
      ...suggestion,
      datasourceId: datasource.id,
    }));
  })
  .flatten()
  .map((suggestion, datasourceSuggestionId) => ({ ...suggestion, datasourceSuggestionId }))
  .value();

[flash1293]

You are right, it makes things easier. However, I would leave the refactoring to a separate PR as it would touch a lot of places and this one is already way too big. The duplicate filter is a good point, I unified that.

[chrisdavies]

The reduce below appears to be just doing a flatten? At the risk of overusing lodash, maybe we should wrap this in lodash, so it looks like this

return _(Object.entries(visualizationMap))
  .map(...)
  .flatten()
  .values()
  .sort((a, b) => b.score - a.score);

Anyway, it eliminates a little cognitive load while reading this while adding a little in a different place (you have to remember that you're dealing with a lodash wrapped collection). So, take it or leave it.

[flash1293]

The I want to do is using flatMap and we are done here - it's a shame it's not supported by our setup currently. I try to introduce as little lodash as reasonable, but as we have flatten in a thousand places already it's probably also fine here. Changed.

[wylieconlon]

I've been asking about this for a little while- we were supposed to get a new version of core-js that will polyfill Array.flatMap and Array.flatten for us, but I think the ops/platform team hasn't upgraded yet

[chrisdavies]

Nit: it seems that the frame should do the if layerIds.length > 0 check, so this could become:

frame.removeLayers(_.difference(Object.keys(frame.datasourceLayers), suggestion.keptLayerIds));

[chrisdavies]

This is fine, but maybe the 3 should be a const MAX_SUGGESTIONS_DISPLAYED or something? On the other hand, this is unambiguous.

[chrisdavies]

No need to return an array, since flatten handles raw values and arrays interchangeably.

[chrisdavies]

This is the second time I've seen this .map(...) logic used to assign datasourceSuggestionId in this PR. I wonder if it's worth pulling out into a helper as in my code snippet above? Probably not? Just calling it to your attention.

[chrisdavies]

Nit: You're kind of doing a map and filter inside a single map with your if statement on line 305. I wonder if it would be easier to follow if we did a filter?

return _(Object.entries(state.layers || {}))
  .filter(([_id, layer]) => layer.columnOrder.length)
  .map(([layerId, layer]) => {
    const onlyMetric = layer.columnOrder.every(columnId => !layer.columns[columnId].isBucketed);
    const onlyBucket = layer.columnOrder.every(columnId => layer.columns[columnId].isBucketed);
    if (onlyMetric || onlyBucket) {
      // intermediary chart, don't try to suggest reduced versions
      return buildSuggestion({ state, layerId, isMultiRow: false, datasourceSuggestionId: index });
    }

    return createSimplifiedTableSuggestions(state, layerId);
  })
  .flatten()
  .map(assignDatasourceSuggestionIds)
  .value();

[chrisdavies]

Same comment about not needing to return an array, but again, not super important.

[chrisdavies]

Nit: I found this a bit hard to follow. I wonder if we should simplify:

function buildLayerByColumnOrder(layer: IndexPatternLayer, columnOrder: string[]) {
  return {
    ...layer,
    columnOrder,
    columns: _.pick(layer.columns, columnOrder),
  };
}

Is that clearer or harder? The reader will have to remember what pick does, etc. But I think it's an improvement?

[elasticmachine]

💔 Build Failed

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

[flash1293]

Thanks @chrisdavies for your thorough review! I added most of your nits as they make total sense, but I tried to limit the introduction of lodash as much as possible. In an ideal world the standard lib provides enough tools (I think 90% of our lodash usage is flatten, hopefully we get to node.js 11 soon, then all of these are gone), and I still have hope to get there one day.

[elasticmachine]

💚 Build Succeeded

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

[wylieconlon]

LGTM

[wylieconlon]

I've been asking about this for a little while- we were supposed to get a new version of core-js that will polyfill Array.flatMap and Array.flatten for us, but I think the ops/platform team hasn't upgraded yet