[lens] Dimension panel lets users select operations and fields individually

[wylieconlon]

Popover with options dependent on the kind of field selected:

Does not currently validate that queries can be combined across different dimensions, for example a "value" and "average" query cannot be used together.

[elasticmachine]

💔 Build Failed

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

[flash1293]

Shouldn't this id also be prefixed by the indexpattern id?

[wylieconlon]

The prefixes in the others are not indexpattern ID- but that should be part of this.

[wylieconlon]

Still a WIP, will update with reviewable code later

[wylieconlon]

The prefixes in the others are not indexpattern ID- but that should be part of this.

[elasticmachine]

💔 Build Failed

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

[snide]

Small comment on the sass files.

[snide]

Want to follow our BEM formatting here. .lnsIndexPattern__dimensionPopover

[elasticmachine]

💔 Build Failed

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

[wylieconlon]

Looking for feedback on the test coverage and behavior here. Please review when you get a chance @chrisdavies @flash1293

[elasticmachine]

💔 Build Failed

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

[elasticmachine]

💔 Build Failed

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

[flash1293]

Looking very good so far, I think we can merge it short term and add the missing stuff in separate PRs (arguments, column ids and so on)

[flash1293]

Why is 3 equivalent to last? Any reason not to go to Number.MAX_SAFE_INTEGER?

In a more complex chart we could have a dimension for a chart split, two series splits and a x axis for example.

[wylieconlon]

We talked about this a while ago- we currently have 0, 1, and 2 defined as the only possible DimensionPriority. The dimension priority only affects aggregations (as shown here), and we agreed that visualizations should only be able to indicate the hierarchy of up to 3 aggregations. I could use the max int, sure.

[chrisdavies]

Either way, we shouldn't use a magic number here. 3 should be a const or a special "end" enum value.

[wylieconlon]

Take another look

[flash1293]

We are testing whether the helpers getColumnOrder and getPotentialColums work correctly in isolation, but not whether they are used correctly within the dimension panel itself. In addition to the direct unit tests I think a few higher level test are useful, e.g. whether the suggestedOrder prop is correctly used when set on a dimensional panel or whether only the "right" fields are shown depending on the operations filter

[wylieconlon]

Take another look, I think I've tested this more completely in the latest. Make sure to open up the new __mocks__/operations.ts file

[flash1293]

LGTM - I think a higher level functional test as discussed offline can be added in a separate PR

[flash1293]

It looks like esaggs wants to have the metric columns first in the aggs parameter. So for mapping we have to change the order to first metric columns, then the bucket columns from most outer bucket to inner bucket.
Seems like this has something to do with esaggs mapping directly to the UI of the old editor.

It doesn't work correctly if we don't do this:

esaggs index="d3d7af60-4c81-11e8-b3d7-01146121b73d" metricsAtAllLevels="false" partialRows="false" 
  aggConfigs="[{\"id\":\"54d00fc9-2020-47cb-a24b-8172abe3d62e\",\"enabled\":true,\"type\":\"date_histogram\",\"schema\":\"segment\",\"params\":{\"field\":\"timestamp\",\"timeRange\":{\"from\":\"now-1d\",\"to\":\"now\"},\"useNormalizedEsInterval\":true,\"interval\":\"1h\",\"drop_partials\":false,\"min_doc_count\":1,\"extended_bounds\":{}}},{\"id\":\"0a082a80-c422-47db-813b-454c3d492277\",\"enabled\":true,\"type\":\"count\",\"schema\":\"metric\",\"params\":{}}]"

(first date_histogram, then count)

results in a data table with only the date histogram. If you configure the same thing in the old editor, the aggs are switched around. Multiple bucketing aggs have their "real" order, though.

Edit: It also works if the order doesn't match, my bad. Looks like it doesn't matter where the metric columns are located.

[wylieconlon]

I looked into this a bit more. The issue here is actually that the arguments to the Terms aggregation specify orderBy: '1' which is not a valid thing to order by. So I need to update that clause of the Terms aggregation.

[wylieconlon]

I'd like to get some feedback from @chrisdavies and @flash1293 about whether this snapshot test is adding value to your workflow. It got in my way a lot, and the snapshot is quite large, so it feels like it might be worth eliminating.

[flash1293]

I agree that this snapshot isn't really adding anything meaningful the tests below don't capture already.

[wylieconlon]

Removing

[elasticmachine]

💚 Build Succeeded

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

[flash1293]

Just fixed some trivial merge conflicts. As a first shot this LGTM.

Stuff we can change before merge

• look at a few nits
• rename rollup stuff to something generic
• remove complete snapshot

Stuff we should do after merge

• Add configuration to operations (interval for date_histogram etc.)
• Catch and auto-fix invalid configurations (mixture of raw and aggregation, only a single aggregation column without metric)
• High level "integration style" test

[flash1293]

LGTM - I think a higher level functional test as discussed offline can be added in a separate PR

[flash1293]

nit: We could give this object its own type.

[wylieconlon]

I'm not a big fan of this suggestion because I was already struggling to organize the types here. I will not make this change unless we have another use case for it. I will rename this from rollupRestrictions to aggregationRestrictions

[flash1293]

I didn't expect indexPattern.fields to be ever a string so I looked into the origin of this type. It turns out in x-pack/plugins/lens/public/indexpattern_plugin/loader.ts Object.assign is used to build this object which for some reason mixes the interfaces of its parameters. An object spread doesn't do this:

        return {
          ...attributes,
          timeFieldName: attributes.timeFieldName || undefined,
          id,
          type,
          title: attributes.title,
          fields: (JSON.parse(attributes.fields) as IndexPatternField[]).filter(
            ({ type: fieldType, esTypes }) =>
              fieldType !== 'string' || (esTypes && esTypes.includes('keyword'))
          ),
          typeMeta: attributes.typeMeta
            ? (JSON.parse(attributes.typeMeta) as SavedRollupInfo)
            : undefined,
        }

Then indexPattern.fields is IndexPatternField[] all the time and the code here can be simplified to

  if (!indexPattern.typeMeta) {
    return indexPattern;
  }

  const typeMeta = indexPattern.typeMeta;

  const aggs = Object.keys(typeMeta.aggs);

  const newFields = [...indexPattern.fields];
  newFields.forEach((field, index) => {
    const restrictionsObj: IndexPatternField['rollupRestrictions'] = {};
    aggs.forEach(agg => {
      if (typeMeta.aggs[agg] && typeMeta.aggs[agg][field.name]) {
        restrictionsObj[agg] = typeMeta.aggs[agg][field.name];
      }
    });
// ....

[wylieconlon]

This is what I was trying to get to, I'll update this.

[elasticmachine]

💚 Build Succeeded

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

[elasticmachine]

💚 Build Succeeded

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