[Lens] Add conditional operations in Formula by dej611 · Pull Request #142325 · elastic/kibana

dej611 · 2022-09-30T11:12:23Z

Summary

Tinymath package
- Add new comparison operation: lt, gt, eq, lte, gte
- Add new functions documentation
  - Generate new functions.md page
- Add new ifelse operation
  - The ifelse name is a placeholder. Possible alternatives are: ifElse, when, ifThen, or make your proposal. :)
- Add new operation documentation
- Add tests for new functions
- Make grammar accept comparison symbols: >, <, ==, >=, <=
  - to test it I recommend copy the grammar.peggy content into this web site to test the resulting AST: https://peggyjs.org/online.html
- Add tests for backward compatibility and new features in grammar
Lens side
- Enable new functions in Formula (comparison + ifelse)
- Validation
  - Make validation work for different returning types
  - Make validation fail for comparison only expressions
  - Make validation fail if non-comparison condition passed to ifelse function
  - Add new validation tests
- Add documentation for new operations
  - New section for comparison?

Extra fixes

🐛 Fixed also a bug in the divide operation with a divide by 0 scenario.
✏️ Improved documentation about testing to avoid the Jest esm error message

New grammar changes examples

// Not ok: math operations cannot be performed with a comparison on any end of the operation
1 + (1 > 1)
(1 > 1) + 1
((1 > 1) + 1)
5 > 1 > 2
(5 > 1) > 2
(5 > 1) + (5 > 1)

// Ok: math operations can be on either side of the comparison, and be considered as grouped
1 > 1
(1 > 1)
((1 > 1))
((1 + 1) > 1)
1 + 1 > 1 * 1 // => becomes (1 + 1) > (1 * 1) => 1 > 1

Tinymath documentation

Command I've used to generate the new functions.md file:

npx -p jsdoc-to-markdown -c 'jsdoc2md --template packages/kbn-tinymath/docs/template/functions.hbs --private --files "packages/kbn-tinymath/src/functions/**/*.js"' > packages/kbn-tinymath/docs/functions.md

I couldn't make it work properly the previous command used when tinymath was in its own repo: the produced json for the documentation had the wrong function name, always set to module.exports.
In the jsdoc-to-markdown documentation it mentions that module.exports had to be defined after the actual function implementation (which is apparently a jsdoc limitation], and after that everything worked properly. That is why some many files changed with only the exports.

Demo

Working example:

Comparison symbols and functions working as condition:

Errors:

Documentation:

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

dej611 · 2022-10-03T11:52:11Z

packages/kbn-tinymath/src/functions/divide.js

+  if (Array.isArray(b)) {
+    return b.map((bi) => {
+      if (bi === 0) throw new Error('Cannot divide by 0');
+      return a / bi;
+    });
+  }


divide(4, [1, 0]) would generate an unhandled error without this fix

elasticmachine · 2022-10-03T14:58:09Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

dej611 · 2022-10-04T06:56:29Z

packages/kbn-tinymath/src/functions/round.js

-module.exports = { round };
-
-function round(a, b = 0) {
+function round(a, b) {


The refactor here was due to the documentation processor, which handles the default parameter assignment in the function generating a different documentation layout in the functions.md file for the round signature.

nit: could do the defaulting once—

function round(a, _b) { b = _b ?? 0 ...

Changed the code here with a cleaner solution in a deeper function.

flash1293 · 2022-10-04T15:54:39Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/util.ts

+Returns a value depending on whether the element of condition is true or false.
+
+Example: Compare two fields average and return the highest
+\`ifelse( average(memory) >= average(bytes), average(memory), average(bytes))\`


Not sure about this example, you can already do it via pick_max. What about the first example from the "Use cases" section here: #94603 ? Seems understandable to me.

flash1293 · 2022-10-04T15:55:49Z

packages/kbn-tinymath/grammar/grammar.peggy

+  const symbolsToFn = {
+    '+': 'add', '-': 'subtract',
+    '*': 'multiply', '/': 'divide',
+    '<': 'lt', '>': 'gt', '==': 'eq',


any reason we aren't going with = for equality? As we don't have assignment, it seems like we can go with the simpler case which is also what excel is doing.

Named parameters already use the = syntax ( i.e. count(kql="...")).
It should work already, but thought that the == operation is something quite familiar as well and different enough to not confuse the two.

it's familiar for developers, I don't think it's super common outside of that group. SQL also doesn't do it. What do you think @ghudgins @ninoslavmiskovic ?

I also believe that "==" is not known among no developers, and if SQL does not support it, then it is also an indicator that it would be the preferable choice since SQL is a broader query language than KQL. Are there any cases where it will be an issue of sticking with "=" ?

Made a quick spike on that to see whether it is possible to co-exists with the named argument.
Unfortunately there are some overlaps between the two which make it harder to solve it, at least at grammar level:

ifelse(a=1, 1, 2) // => 🆘 named argument or comparison? Grammar says it's named argument ifelse(a=a, 1, 2) // => 🆘 named argument or comparison? Grammar says it's named argument ifelse(1=1, 1, 2) // => ✅ comparison ifelse(1=a, 1, 2) // => ✅ comparison ifelse(unsupported-named-argument=1, 1, 2) // => ✅ comparison

It looks like it should parse correctly, but I didn't check

They all parse fine.
As long as there's no future plan to have string comparison in the future it can work.

OK, in that case I think we should go with the single =. Otherwise this looks good to me!

Had some more investigation on the topic and found out some more issues on the usage for the single = symbol for comparison.
Here some examples:

❌ for plain tinymath it is possible to use a variable for the comparison on exec

❌ in Canvas it is possible to use a variable for the comparison: math "ifelse(total_errors = 37, 1, 0)" which will raise an error about unsupported Named parameters. This is using exec underneath. This is probably a non-issue as conditional logic can be performed elsewhere

❌ If in the future an optimization like [Lens] Improve performance for large formulas #141456 is introduced for comparison, this can be a problem also for Lens in the formula rewrite step.

All of them are minor problems, but still to keep in mind if we decide to go down this single = symbol route rather than the existing ==.

Discussed offline and decided to keep the == symbol.

Some follow ups have been proposed while keeping the double =:

add autocomplete on the editor when typing = (and magically add another ==).

add operation optimization as [Lens] Improve performance for large formulas #141456 ?

flash1293 · 2022-10-04T16:00:04Z

packages/kbn-tinymath/grammar/grammar.peggy

 // expressions

+// An Expression can be of 3 different types:
+// * a Comparison operation, which can contain recursive MathOperations inside


It's pretty smart to do this within the parsing step, but I worry it's too clever (hard to maintain later on) and it also doesn't create great error messages:

Maybe it makes more sense to pull the type check logic into a separate step after the parsing? Walking the tree and keeping track of the type while recursing should work fine.

The error is generated at validation time while traversing the AST. That is a validation bug 😓 .
The grammar works fine for the expression ifelse(unique_count(extension, kql='...') == 1, count() > 2) => try to paste the peggy file content in here to see it passing: https://peggyjs.org/online.html

Latest commit fixed the validation bug:

drewdaemon

FWIW, I like ifelse! It doesn't look like we have any camelcase functions to this point, so ifElse would be a style break.

drewdaemon · 2022-10-04T15:50:16Z

packages/kbn-tinymath/docs/functions.md

+ifelse( 5 > 6, 1, 0) // returns 0
+ifelse( 1 == 1, [1, 2, 3], 5) // returns [1, 2, 3]
+ifelse( 1 < 2, [1, 2, 3], [2, 3, 4]) // returns [1, 2, 3]


nit: leading spaces

drewdaemon · 2022-10-04T17:46:19Z

packages/kbn-tinymath/src/functions/round.js

-module.exports = { round };
-
-function round(a, b = 0) {
+function round(a, b) {


nit: could do the defaulting once—

function round(a, _b) { b = _b ?? 0 ...

…ocs'

…/94603

flash1293

Looks mostly good, left two small nits but overall great work!

flash1293 · 2022-10-11T12:12:48Z

src/plugins/vis_types/timeseries/server/lib/vis_data/response_processors/series/math.test.js

    } catch (e) {
      expect(e.message).toEqual(
-        'Failed to parse expression. Expected "*", "+", "-", "/", end of input, or whitespace but "(" found.'
+        'Failed to parse expression. Expected "*", "+", "-", "/", "<", "=", ">", end of input, or whitespace but "(" found.'


nit - should be == now

The message is correct. The grammar picks the first possible valid char at the time.
If the expression becomes divide = params.a, params.b it will error with Expected "=" but " " found as it's the only valid route.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/util.ts

…definitions/formula/util.ts

…-ref HEAD~1..HEAD --fix'

legrego

Formatting changes to x-pack/plugins/security/public/management/roles/edit_role/privileges/kibana/feature_table/feature_table.tsx LGTM

…/94603

kibana-ci · 2022-10-12T09:49:37Z

💚 Build Succeeded

Buildkite Build
Commit: c81742c

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`canvas`	1155	1162	+7
`expressions`	173	180	+7
`lens`	925	932	+7
total			+21

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.0MB	1.0MB	+2.7KB
`expressions`	25.7KB	28.4KB	+2.6KB
`lens`	1.3MB	1.3MB	+8.3KB
total			+13.6KB

Page load bundle

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

id	before	after	diff
`expressions`	97.4KB	97.4KB	+5.0B

History

💚 Build #79447 succeeded 098a745
💔 Build #79401 failed 3eb20bc
💔 Build #79372 failed 67a6e1a
💚 Build #79319 succeeded 6777d31
💚 Build #79027 succeeded 441db03
💚 Build #78783 succeeded 0efef8b

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

dej611 added 7 commits September 30, 2022 09:11

✨ Introduce new comparison functions

e560bef

✨ Introduce new comparison symbols into grammar

6a78199

🔧 Introduce new tinymath functions

96e27cc

✨ Add comparison fn validation to formula

c74cf67

♻️ Some type refactoring

5784a15

Merge remote-tracking branch 'upstream/main' into feature/94603

738076a

✏️ Fix wrong error message

3944ae2

dej611 added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:Lens v8.6.0 labels Sep 30, 2022

dej611 requested a review from cqliu1 September 30, 2022 11:13

dej611 added 9 commits September 30, 2022 18:57

✅ Add more formula unit tests

5d4df23

✅ Add more tests

66289e6

✅ Fix tsvb test

12dcfc2

Merge remote-tracking branch 'upstream/main' into feature/94603

ed96fbe

🐛 Fix issue with divide by 0

9cee512

✏️ Update testing command

11442e4

✏️ Add some more testing info

5dc906c

✨ Improved grammar to handle edge cases

c22a3d5

✅ Improve comparison code + unit tests

6f0874c

dej611 commented Oct 3, 2022

View reviewed changes

dej611 added 3 commits October 3, 2022 13:54

✅ Fix test

00bfae4

✏️ Update documentation with latest functions

ab1f5ad

Merge remote-tracking branch 'upstream/main' into feature/94603

40d9da8

dej611 marked this pull request as ready for review October 3, 2022 14:58

dej611 requested a review from a team as a code owner October 3, 2022 14:58

dej611 added the release_note:enhancement label Oct 3, 2022

dej611 commented Oct 4, 2022

View reviewed changes

flash1293 reviewed Oct 4, 2022

View reviewed changes

drewdaemon reviewed Oct 4, 2022

View reviewed changes

drewdaemon added the ui-copy Review of UI copy with docs team is recommended label Oct 4, 2022

dej611 and others added 12 commits October 5, 2022 12:21

Merge remote-tracking branch 'upstream/main' into feature/94603

4747225

👌 Integrate feedback

f4f48f2

👌 Integrate more feedback

0bb5e4d

👌 Update doc

125ecdd

🐛 Fix bug with function return type check

f26e9b7

🔥 remove duplicate test

3abc4f4

[CI] Auto-commit changed files from 'node scripts/build_plugin_list_d…

4a7e738

…ocs'

Merge branch 'main' into feature/94603

4cf78d5

Merge branch 'main' into feature/94603

0efef8b

Merge branch 'main' into feature/94603

441db03

Merge remote-tracking branch 'upstream/main' into feature/94603

3777811

Merge branch 'feature/94603' of github.com:dej611/kibana into feature…

6777d31

…/94603

flash1293 approved these changes Oct 11, 2022

View reviewed changes

dej611 and others added 4 commits October 11, 2022 14:39

Update x-pack/plugins/lens/public/indexpattern_datasource/operations/…

0a95ab4

…definitions/formula/util.ts

✏️ Fixes formula

67a6e1a

Merge branch 'main' into feature/94603

79eb184

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

3eb20bc

…-ref HEAD~1..HEAD --fix'

kibanamachine requested a review from a team as a code owner October 11, 2022 14:13

Merge branch 'main' into feature/94603

098a745

legrego approved these changes Oct 11, 2022

View reviewed changes

dej611 added 2 commits October 12, 2022 10:30

Merge remote-tracking branch 'upstream/main' into feature/94603

38547c9

Merge branch 'feature/94603' of github.com:dej611/kibana into feature…

c81742c

…/94603

dej611 merged commit 757ab76 into elastic:main Oct 12, 2022

kibanamachine added the backport:skip This PR does not require backporting label Oct 12, 2022

This was referenced Oct 13, 2022

[Lens] Add automatic = insertion on equal comparison typing when in Formula #143286

Merged

[Lens] Add formula optimization for comparison symbols #143288

Merged

rajvi-patel-22 mentioned this pull request Mar 23, 2023

[O11y][HAProxy] Migrate stat data-stream visualizations to lens in kibana 8.4.0 elastic/integrations#5611

Closed

7 tasks

Conversation

dej611 commented Sep 30, 2022 • edited by morashwan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Extra fixes

New grammar changes examples

Tinymath documentation

Demo

Checklist

Risk Matrix

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dej611 Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dej611 Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewdaemon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Oct 12, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Uh oh!

dej611 commented Sep 30, 2022 •

edited by morashwan

Loading

dej611 Oct 11, 2022 •

edited

Loading

dej611 Oct 4, 2022 •

edited

Loading