[Lens] Implement types for reference-based operations #83603

wylieconlon · 2020-11-18T00:16:07Z

This PR introduces the fullReference operation type and adds unit tests for this type. The full list of functionality for this operation type is here:

Provide a list of referenceIds when inserting a reference or replacing with a reference
Recursively delete columns when moving away from a reference
Validate that all references exist and meet the requirements set by their parents
Validate that all references have the right parent state, such as date histograms
Collects expression output for reference-based functions
Publishes the correct list of columns to visualizations
Renames the previous "references" when talking about invalid fields
Introduces getDefaultName function which isn't actually used in this PR
Update column order for reference-based operations

Split into a separate PR:

UI for editing referenced-based columns
Better transitions when replacing an existing column with a reference-based column, instead of resetting every time
Handling of the incompleteColumns object in the UI
Use the getDefaultName function in the UI, to allow resetting the name

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
Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-11-18T00:16:09Z

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

flash1293

Took a look and this looks pretty good to me. Most of my comments can be tackled later I think (the first one should probably be resolved on this PR)

I will start building the time scale logic on top of this to minimize conflicts.

flash1293 · 2020-11-18T11:01:52Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx

+          column &&
+          !column.isBucketed &&
+          !isReferenced(
+            // Fake layer, but works with real columns


What about just passing in the real layer? This seems brittle because the synthetic layer is not consistent (e.g. if isReferenced will start using columnOrder or indexPatternId later, it will break)(

flash1293 · 2020-11-18T11:03:14Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

+    let tempLayer = { ...layer };
+
+    if (previousDefinition.input === 'fullReference') {
+      // @ts-expect-error references are not statically analyzed


Those will go away once we have a first reference-based operation, right? If yes I think we can live with it for now

Yes, once we have the first reference-based operation we will be required to delete the comment. It will fail the type checks if it's not an error.

flash1293 · 2020-11-18T12:25:34Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

+    let tempLayer = { ...layer };
+    const referenceIds = operationDefinition.requiredReferences.map((validation) => {
+      const validOperations = Object.values(operationDefinitionMap).filter(({ type }) =>
+        isOperationAllowedAsReference({ validation, operationType: type })


I guess we should pass in the field we get via parameter as well here? Otherwise isOperationAllowedAsReference will never be called with a field

I'll clarify this in a comment in code, but basically I'm not expecting the field to be passed in while constructing a reference-based operation. However I would expect that the operationSupportMatrix would be used here to look for cases where there's only one possible field or operation.

flash1293 · 2020-11-18T12:32:24Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

+    const metadata = operationDefinition.getPossibleOperation();
+    hasValidMetadata = Boolean(metadata) && validation.validateMetadata(metadata!);
+  } else {
+    // TODO: How can we validate the metadata without a specific field?


That's a very good question and we have to answer it in some way for the UI so we can provide a list of sub-functions. One option would be to go through all fields of the current index pattern and return true if one of them matches.

Exactly, this can't be done purely in the code here.

flash1293 · 2020-11-18T12:40:07Z

x-pack/plugins/lens/public/indexpattern_datasource/to_expression.ts

+    columnEntries.forEach(([colId, col]) => {
+      const def = operationDefinitionMap[col.operationType];
+      if (def.input === 'fullReference') {
+        expressions.push(...def.toExpression(layer, colId, indexPattern));


Can we be sure the columns are in order so all calculations col depends on will be earlier in the list than col itself? Otherwise it can happen an expression function tries to act on a column with doesn't exist yet.

We either have to make sure column order is always right or we add a check here and walk the graph of references in direction of the pointers. It's not relevant for single level calculations, but I guess we will run into this in the future.

It could probably be part of the getColumnOrder as well

Fixed by updating the getColumnOrder function and calling it in a few more places.

kibanamachine · 2020-11-18T22:43:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 104595f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	436	438	+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`	925.2KB	934.2KB	+9.0KB

History

💚 Build #88469 succeeded eb4b573

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

dej611 · 2020-11-19T09:06:01Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts

@@ -134,7 +134,7 @@ export const validateDatasourceAndVisualization = (
    ? currentVisualization?.getErrorMessages(currentVisualizationState, frameAPI)
    : undefined;

-  if (datasourceValidationErrors || visualizationValidationErrors) {
+  if (datasourceValidationErrors?.length || visualizationValidationErrors?.length) {


I wonder if it makes sense to get rid of the undefined here completely, and just work with array. I cannot foresee particular differences between the no current visualization/datasource available case and the no error one in this context.

I considered making this change in this PR, but decided not to because it actually affects the signature of all of the visualizations.

flash1293

I think this PR is fine for now, didn't test again. I think we need to re-revisit the column ordering later but we don't have to solve that problem now.

flash1293 · 2020-11-19T12:18:38Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

  );
+  // If a reference has another reference as input, put it last in sort order


I'm not sure this catches all possible cases for longer chains of references (e.g. what if b doesn't reference a directly, but it references c which in turn references a?), but as we won't have them in the first version, I think it's fine for now if we leave a comment to revisit once we get more complex here.

One way to solve it for all possible cases is to build an inverted index of reference targets (Record<string, string[]> - for each column, there's a list of other columns pointing to it), then pushing all columns without references to the expression, removing them from the inverted index, pushing all columns without references and so on until all of them are gone.

Edit: I originally wrote something incorrect.

I think this is catching all possible cases, including the one you mentioned. If the order is b -> c -> a like you said, then the sorting logic will detect that b references c, so c comes first and also that c references a, so a comes first. This is basically topological sorting.

mbondyra

Code LGTM

kibanamachine · 2020-11-24T18:44:39Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 83603 or prevent reminders by adding the backport:skip label.

* [Lens] Implement types for reference-based operations * Update from review feedback

[Lens] Implement types for reference-based operations

eb4b573

wylieconlon added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.11.0 labels Nov 18, 2020

wylieconlon requested review from dej611, flash1293, mbondyra and a team November 18, 2020 00:16

flash1293 reviewed Nov 18, 2020

View reviewed changes

wylieconlon added 3 commits November 18, 2020 14:52

Merge remote-tracking branch 'origin/master' into lens/reference-types

b4fb6ea

Merge remote-tracking branch 'origin/master' into lens/reference-types

6024a31

Update from review feedback

104595f

dej611 reviewed Nov 19, 2020

View reviewed changes

flash1293 approved these changes Nov 19, 2020

View reviewed changes

flash1293 mentioned this pull request Nov 19, 2020

[Lens] Calculation operations #83789

Merged

mbondyra approved these changes Nov 19, 2020

View reviewed changes

flash1293 mentioned this pull request Nov 20, 2020

[Lens] Normalize values by time unit #83904

Merged

wylieconlon merged commit b50e7ba into elastic:master Nov 20, 2020

wylieconlon deleted the lens/reference-types branch November 20, 2020 18:42

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 24, 2020

wylieconlon mentioned this pull request Nov 24, 2020

[7.x] [Lens] Implement types for reference-based operations (#83603) #84290

Merged

wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Nov 24, 2020

[Lens] Implement types for reference-based operations (elastic#83603)

f630835

* [Lens] Implement types for reference-based operations * Update from review feedback

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 25, 2020

wylieconlon pushed a commit that referenced this pull request Nov 25, 2020

[Lens] Implement types for reference-based operations (#83603) (#84290)

6c2963f

* [Lens] Implement types for reference-based operations * Update from review feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Implement types for reference-based operations #83603

[Lens] Implement types for reference-based operations #83603

wylieconlon commented Nov 18, 2020 •

edited

Loading

elasticmachine commented Nov 18, 2020

flash1293 left a comment •

edited

Loading

flash1293 Nov 18, 2020

wylieconlon Nov 18, 2020

flash1293 Nov 18, 2020

wylieconlon Nov 18, 2020

flash1293 Nov 18, 2020

wylieconlon Nov 18, 2020

flash1293 Nov 18, 2020

wylieconlon Nov 18, 2020

flash1293 Nov 18, 2020

wylieconlon Nov 18, 2020

kibanamachine commented Nov 18, 2020

dej611 Nov 19, 2020

wylieconlon Nov 20, 2020

flash1293 left a comment •

edited

Loading

flash1293 Nov 19, 2020 •

edited

Loading

wylieconlon Nov 20, 2020 •

edited

Loading

mbondyra left a comment

kibanamachine commented Nov 24, 2020

		);
		// If a reference has another reference as input, put it last in sort order

[Lens] Implement types for reference-based operations #83603

[Lens] Implement types for reference-based operations #83603

Conversation

wylieconlon commented Nov 18, 2020 • edited Loading

Checklist

elasticmachine commented Nov 18, 2020

flash1293 left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Nov 18, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

wylieconlon Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

mbondyra left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 24, 2020

wylieconlon commented Nov 18, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

flash1293 left a comment •

edited

Loading

flash1293 Nov 19, 2020 •

edited

Loading

wylieconlon Nov 20, 2020 •

edited

Loading