[Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn #90481

dej611 · 2021-02-05T18:17:11Z

Summary

This PR moves the mapColumn and math expressions to the shared expressions plugin in order to enable Lens to use them for the "Formula" features (see issue for more details).

Move mapColumn
- Add new id argument (feature + help + tests)
- Add new copyMetaFrom argument (feature + help + tests)
Move math
- Add new onError argument (feature + help + tests)
- Migrate to new returning types (feature + help + tests)
cleanup

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
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

dej611 · 2021-02-09T09:30:03Z

@elasticmachine merge upstream

dej611 · 2021-02-11T14:05:57Z

@elasticmachine merge upstream

dej611 · 2021-02-15T09:09:00Z

@elasticmachine merge upstream

elasticmachine · 2021-02-16T13:29:21Z

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

dej611 · 2021-02-16T13:49:11Z

docs/canvas/canvas-function-reference.asciidoc

 |===

-*Returns:* `number`
+*Returns:* `number` | `boolean` | `null`


It needs to extends the return type to cover the onError returning values

ppisljar

code LGTM

dej611 · 2021-02-17T09:24:16Z

Renamed PR for changelog

flash1293 · 2021-02-17T16:09:15Z

src/plugins/expressions/common/expression_functions/specs/math.ts

+      types: ['string'],
+      help: i18n.translate('expressions.functions.math.args.onErrorHelpText', {
+        defaultMessage:
+          "In case the {TINYMATH} evaluation fails or returns NaN, the return value is specified by onError. When `'throw'`, it will throw an exception, terminating expression execution (default).",


This should specify valid values instead of just being of type string (to allow auto-complete)

I tried that as first option, but could not find a way to map a string union to a specific type for expressions. All the examples I check in the documentation have the string type assigned and documenting the valid values on the documentation side.

flash1293 · 2021-02-17T16:10:40Z

src/plugins/expressions/common/expression_functions/specs/math.ts

+      }
+      return result;
+    } catch (e) {
+      if (isDatatable(input) && input.rows.length === 0) {


The onError logic has to happen in the catch handler as well to catch things like divide by zero

dej611 · 2021-02-18T14:10:17Z

@elasticmachine merge upstream

dej611 · 2021-02-19T13:18:00Z

@elasticmachine merge upstream

flash1293 · 2021-02-22T09:18:50Z

@elasticmachine merge upstream

flash1293

Looks almost good to me, just found one bug

flash1293 · 2021-02-22T09:36:10Z

src/plugins/expressions/common/expression_functions/specs/map_column.ts

+
+    return Promise.all(rowPromises).then((rows) => {
+      const existingColumnIndex = columns.findIndex(({ name }) => name === args.name);
+      const type = rows.length ? getType(rows[0][args.name]) : 'null';


If id and name are both used, this lookup won't work (same issue with the row above). It should use columnId as well (probably worth a functional test)

Fixed the issue. I did misinterpret it initially. There's a test already to cover this specific case.

…nto expression/mapColumn-ext

flash1293

Small nit, otherwise looks good

flash1293 · 2021-02-22T12:25:57Z

src/plugins/expressions/common/expression_functions/specs/map_column.ts

+      types: ['string', 'null'],
+      help: i18n.translate('expressions.functions.mapColumn.args.copyMetaFromHelpText', {
+        defaultMessage:
+          "If set, the meta object from the specified column id is copied over to the specified target column. Throws an exception if the column doesn't exist",


It says the meta object is copied by specified column id, but it is actually using the name. I think we should use the id in line 107 as it's more stable)

It also says "Throws an exception if the column doesn't exist", but it will actually just silently not copy over meta. I think this behavior is OK, but we should adjust documentation.

wylieconlon

This is looking good, except I agree with Joe's comment that metadata should be copied by ID instead of by name.

wylieconlon · 2021-02-22T22:20:11Z

src/plugins/expressions/common/expression_functions/specs/map_column.ts

+      aliases: ['exp', 'fn', 'function'],
+      help: i18n.translate('expressions.functions.mapColumn.args.expressionHelpText', {
+        defaultMessage:
+          'A {CANVAS} expression that is passed to each row as a single row {DATATABLE}.',


I know that you didn't write this text, but maybe if you're making changes you can correct it. Two things to correct:

The row is passed into the expression, but the text has it backwards

Canvas expression vs expression

Suggested change

'A {CANVAS} expression that is passed to each row as a single row {DATATABLE}.',

'An expression that is executed on every row, provided with a single-row {DATATABLE} context and returning the cell value.',

dej611 · 2021-02-23T10:35:04Z

@elasticmachine merge upstream

flash1293

LGTM, left two nits about extending the unit tests a little

flash1293 · 2021-02-24T08:53:16Z

src/plugins/expressions/common/expression_functions/specs/tests/map_column.test.ts

+          {
+            id: 'myId',
+            name: 'myName',
+            meta: { type: 'date' },


Could we extends this test a little with more meta information?

flash1293 · 2021-02-24T08:54:59Z

src/plugins/expressions/common/expression_functions/specs/tests/map_column.test.ts

+        expect(result.columns).toHaveLength(1);
+        expect(result.columns[0]).toHaveProperty('name', 'name');
+        expect(result.columns[0]).toHaveProperty('id', 'name');
+        expect(result.columns[0].meta).toHaveProperty('type', 'null');


Can we add a test here that still infers the type correctly from the first row if the meta information transfer fails?

dej611 · 2021-02-24T15:16:04Z

@elasticmachine merge upstream

kibanamachine · 2021-02-24T17:13:43Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`canvas`	1149	1145	-4
`expressions`	105	149	+44
total			+40

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`canvas`	1.4MB	1.4MB	+38.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`canvas`	592.1KB	548.7KB	-43.4KB
`expressions`	165.8KB	210.8KB	+45.0KB
total			+1.5KB

Unknown metric groups

async chunk count

id	before	after	diff
`canvas`	6	7	+1

History

💔 Build #109054 failed 0f601c0
💔 Build #109016 failed 7cdf96f
💔 Build #108997 failed a6cb2d0
💚 Build #108837 succeeded 6bfa823
💔 Build #108810 failed a5c8bdf

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

cqliu1

LGTM 👍 Thanks for updating the function reference docs

…d configurable onError argument to math fn (elastic#90481) Co-authored-by: Kibana Machine <[email protected]>

…d configurable onError argument to math fn (#90481) (#92810) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

* master: (38 commits) Fixes Cypress flake by adding pipe, click, and should (elastic#92762) [Discover] Fix filtering selected sidebar fields (elastic#91828) [ML] Fixes positions of calendar arrow buttons in start datafeed modal (elastic#92625) [dev/build_ts_refs] check that commit in outDirs matches mergeBase (elastic#92513) add dep on `@kbn/config` so it is built first [Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn (elastic#90481) [Lens] Fix Workspace hidden when using Safari (elastic#92616) [Lens] Fixes vertical alignment validation messages (elastic#91878) forbid x-elastic-product-origin header in elasticsearch configuration (elastic#92359) [Security Solution][Detections] Set default indicator path to reduce friction with new filebeat modules (elastic#92081) [ILM][Accessibility] Added A11y test for ILM new policy form. (elastic#92570) [Security Solution][Exceptions] - Fixes exceptions builder UI where invalid values can cause overwrites of other values (elastic#90634) Automatically generated Api documentation (elastic#86232) Increase index pattern select limit to 1000 (elastic#92093) [core.logging] Add RewriteAppender for filtering LogMeta. (elastic#91492) [Security Solution][Detection Rules] Update prebuilt rule threats to match schema (elastic#92281) [Security Solutions][Detection Engine] Fixes bug with not being able to duplicate indicator matches (elastic#92565) [Dashboard] Export appropriate references from byValue panels (elastic#91567) [Upgrade Assistant] Align code between branches (elastic#91862) [Security Solution][Case] Fix alerts push (elastic#91638) ...

🚚 Move mapColumn to expressions plugin

2fc8e67

dej611 added v8.0.0 Feature:Lens Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Feb 5, 2021

Merge branch 'master' into expression/mapColumn-ext

2f1e324

Merge branch 'master' into expression/mapColumn-ext

744f39a

kibanamachine and others added 6 commits February 15, 2021 04:09

Merge branch 'master' into expression/mapColumn-ext

b00b9dc

🌐 Fix i18n for mapColumns

96c3aae

🚚 Move over the math function to shared expressions plugin

241aa03

✨ Add new onError argument

b89934f

🌐 Fix i18n for math

4c8fd8c

📝 Update expressions doc with new args

75fecc8

dej611 added v7.13.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:enhancement labels Feb 16, 2021

dej611 marked this pull request as ready for review February 16, 2021 13:29

dej611 requested review from a team as code owners February 16, 2021 13:29

dej611 commented Feb 16, 2021

View reviewed changes

ppisljar approved these changes Feb 17, 2021

View reviewed changes

dej611 changed the title ~~[Expressions] [Lens] Move mapColumn and Math from canvas to expressions plugin~~ [Expressions] [Lens] Add id and copyMetaFrom to mapColumn fn + add configurable onError argument to math fn Feb 17, 2021

dej611 changed the title ~~[Expressions] [Lens] Add id and copyMetaFrom to mapColumn fn + add configurable onError argument to math fn~~ [Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn Feb 17, 2021

flash1293 requested changes Feb 17, 2021

View reviewed changes

Merge branch 'master' into expression/mapColumn-ext

abbdbbc

Merge branch 'master' into expression/mapColumn-ext

88a7d36

dej611 requested a review from flash1293 February 19, 2021 13:18

Merge branch 'master' into expression/mapColumn-ext

0ba2ce9

flash1293 requested changes Feb 22, 2021

View reviewed changes

dej611 added 2 commits February 22, 2021 12:55

🐛 Fix bug wth type picker

0ac1f27

Merge branch 'expression/mapColumn-ext' of github.com:dej611/kibana i…

eb841bc

…nto expression/mapColumn-ext

flash1293 reviewed Feb 22, 2021

View reviewed changes

wylieconlon approved these changes Feb 22, 2021

View reviewed changes

kibanamachine and others added 3 commits February 23, 2021 05:35

Merge branch 'master' into expression/mapColumn-ext

2579d0e

👌 Integrated feedback

a5c8bdf

🐛 Fix unused value

6bfa823

flash1293 approved these changes Feb 24, 2021

View reviewed changes

dej611 added 3 commits February 24, 2021 10:06

✅ Add more tests

a6cb2d0

🐛 Fix type issue

7cdf96f

✅ Fix unit test

0f601c0

Merge branch 'master' into expression/mapColumn-ext

b8aff19

cqliu1 approved these changes Feb 24, 2021

View reviewed changes

dej611 merged commit 0194739 into elastic:master Feb 25, 2021

dej611 deleted the expression/mapColumn-ext branch February 25, 2021 08:43

dej611 mentioned this pull request Feb 25, 2021

[7.x] [Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn (#90481) #92810

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Feb 25, 2021

[Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + ad…

f01d30b

…d configurable onError argument to math fn (elastic#90481) Co-authored-by: Kibana Machine <[email protected]>

poffdeluxe mentioned this pull request Apr 6, 2021

Handle essql results with zero rows #28688

Closed

poffdeluxe mentioned this pull request Aug 4, 2021

[Canvas] Add onError fallback for getCell function #107699

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn #90481

[Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn #90481

dej611 commented Feb 5, 2021 •

edited

Loading

dej611 commented Feb 9, 2021

dej611 commented Feb 11, 2021

dej611 commented Feb 15, 2021

elasticmachine commented Feb 16, 2021

dej611 Feb 16, 2021

ppisljar left a comment

dej611 commented Feb 17, 2021

flash1293 Feb 17, 2021

dej611 Feb 18, 2021

flash1293 Feb 17, 2021

dej611 commented Feb 18, 2021

dej611 commented Feb 19, 2021

flash1293 commented Feb 22, 2021

flash1293 left a comment

flash1293 Feb 22, 2021 •

edited

Loading

dej611 Feb 22, 2021

flash1293 left a comment

flash1293 Feb 22, 2021

wylieconlon left a comment

wylieconlon Feb 22, 2021

dej611 commented Feb 23, 2021

flash1293 left a comment

flash1293 Feb 24, 2021

flash1293 Feb 24, 2021

dej611 commented Feb 24, 2021

kibanamachine commented Feb 24, 2021

async chunk count

cqliu1 left a comment

	'A {CANVAS} expression that is passed to each row as a single row {DATATABLE}.',
	'An expression that is executed on every row, provided with a single-row {DATATABLE} context and returning the cell value.',

[Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn #90481

[Expressions] [Lens] Add id and copyMetaFrom arg to mapColumn fn + add configurable onError argument to math fn #90481

Conversation

dej611 commented Feb 5, 2021 • edited Loading

Summary

Checklist

dej611 commented Feb 9, 2021

dej611 commented Feb 11, 2021

dej611 commented Feb 15, 2021

elasticmachine commented Feb 16, 2021

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

dej611 commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Feb 18, 2021

dej611 commented Feb 19, 2021

flash1293 commented Feb 22, 2021

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Feb 23, 2021

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Feb 24, 2021

kibanamachine commented Feb 24, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

cqliu1 left a comment

Choose a reason for hiding this comment

dej611 commented Feb 5, 2021 •

edited

Loading

flash1293 Feb 22, 2021 •

edited

Loading