Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Add conditional operations in Formula #142325

Merged
merged 38 commits into from
Oct 12, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
e560bef
:sparkles: Introduce new comparison functions
dej611 Sep 30, 2022
6a78199
:sparkles: Introduce new comparison symbols into grammar
dej611 Sep 30, 2022
96e27cc
:wrench: Introduce new tinymath functions
dej611 Sep 30, 2022
c74cf67
:sparkles: Add comparison fn validation to formula
dej611 Sep 30, 2022
5784a15
:recycle: Some type refactoring
dej611 Sep 30, 2022
738076a
Merge remote-tracking branch 'upstream/main' into feature/94603
dej611 Sep 30, 2022
3944ae2
:pencil2: Fix wrong error message
dej611 Sep 30, 2022
5d4df23
:white_check_mark: Add more formula unit tests
dej611 Sep 30, 2022
66289e6
:white_check_mark: Add more tests
dej611 Sep 30, 2022
12dcfc2
:white_check_mark: Fix tsvb test
dej611 Oct 3, 2022
ed96fbe
Merge remote-tracking branch 'upstream/main' into feature/94603
dej611 Oct 3, 2022
9cee512
:bug: Fix issue with divide by 0
dej611 Oct 3, 2022
11442e4
:pencil2: Update testing command
dej611 Oct 3, 2022
5dc906c
:pencil2: Add some more testing info
dej611 Oct 3, 2022
c22a3d5
:sparkles: Improved grammar to handle edge cases
dej611 Oct 3, 2022
6f0874c
:white_check_mark: Improve comparison code + unit tests
dej611 Oct 3, 2022
00bfae4
:white_check_mark: Fix test
dej611 Oct 3, 2022
ab1f5ad
:pencil2: Update documentation with latest functions
dej611 Oct 3, 2022
40d9da8
Merge remote-tracking branch 'upstream/main' into feature/94603
dej611 Oct 3, 2022
4747225
Merge remote-tracking branch 'upstream/main' into feature/94603
dej611 Oct 5, 2022
f4f48f2
:ok_hand: Integrate feedback
dej611 Oct 5, 2022
0bb5e4d
:ok_hand: Integrate more feedback
dej611 Oct 5, 2022
125ecdd
:ok_hand: Update doc
dej611 Oct 5, 2022
f26e9b7
:bug: Fix bug with function return type check
dej611 Oct 5, 2022
3abc4f4
:fire: remove duplicate test
dej611 Oct 5, 2022
4a7e738
[CI] Auto-commit changed files from 'node scripts/build_plugin_list_d…
kibanamachine Oct 5, 2022
4cf78d5
Merge branch 'main' into feature/94603
dej611 Oct 5, 2022
0efef8b
Merge branch 'main' into feature/94603
flash1293 Oct 6, 2022
441db03
Merge branch 'main' into feature/94603
dej611 Oct 10, 2022
3777811
Merge remote-tracking branch 'upstream/main' into feature/94603
dej611 Oct 11, 2022
6777d31
Merge branch 'feature/94603' of github.com:dej611/kibana into feature…
dej611 Oct 11, 2022
0a95ab4
Update x-pack/plugins/lens/public/indexpattern_datasource/operations/…
dej611 Oct 11, 2022
67a6e1a
:pencil2: Fixes formula
dej611 Oct 11, 2022
79eb184
Merge branch 'main' into feature/94603
dej611 Oct 11, 2022
3eb20bc
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Oct 11, 2022
098a745
Merge branch 'main' into feature/94603
dej611 Oct 11, 2022
38547c9
Merge remote-tracking branch 'upstream/main' into feature/94603
dej611 Oct 12, 2022
c81742c
Merge branch 'feature/94603' of github.com:dej611/kibana into feature…
dej611 Oct 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/kbn-tinymath/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ parse('1 + random()')

This package is rebuilt when running `yarn kbn bootstrap`, but can also be build directly
using `yarn build` from the `packages/kbn-tinymath` directory.

### Running tests

To test `@kbn/tinymath` from Kibana, run `yarn run jest --watch packages/kbn-tinymath` from
To test `@kbn/tinymath` from Kibana, run `node scripts/jest --config packages/kbn-tinymath/jest.config.js` from
the top level of Kibana.

To test grammar changes it is required to run a build task before the test suite.
80 changes: 58 additions & 22 deletions packages/kbn-tinymath/grammar/grammar.peggy
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,32 @@
max: location.end.offset
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 "=" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ==.

Copy link
Contributor Author

@dej611 dej611 Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline and decided to keep the == symbol.

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

'<=': 'lte', '>=': 'gte',
}

// Shared function for AST operations
function parseSymbol(left, rest){
const topLevel = rest.reduce((acc, [name, right]) => ({
type: 'function',
name: symbolsToFn[name],
args: [acc, right],
}), left);
if (typeof topLevel === 'object') {
topLevel.location = simpleLocation(location());
topLevel.text = text();
}
return topLevel;
}

// op is always defined, while eq can be null for gt and lt cases
function getComparisonSymbol([op, eq]){
return symbolsToFn[op+(eq || '')];
}
}

start
Expand Down Expand Up @@ -70,45 +96,55 @@ Variable

// expressions

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screenshot 2022-10-04 at 17 58 09

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.

Copy link
Contributor Author

@dej611 dej611 Oct 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest commit fixed the validation bug:

Screenshot 2022-10-05 at 15 22 41

// * a MathOperation, which can contain other MathOperations, but not Comparison types
// * an ExpressionGroup, which is a generic Grouping that contains also Comparison operations (i.e. ( 5 > 1))
Expression
= Comparison
/ MathOperation
/ ExpressionGroup

Comparison
= _ left:MathOperation op:(('>' / '<')('=')? / '=''=') right:MathOperation _ {
return {
type: 'function',
name: getComparisonSymbol(op),
args: [left, right],
location: simpleLocation(location()),
text: text()
};
}

MathOperation
= AddSubtract
/ MultiplyDivide
/ Factor

AddSubtract
= _ left:MultiplyDivide rest:(('+' / '-') MultiplyDivide)+ _ {
const topLevel = rest.reduce((acc, curr) => ({
type: 'function',
name: curr[0] === '+' ? 'add' : 'subtract',
args: [acc, curr[1]],
}), left);
if (typeof topLevel === 'object') {
topLevel.location = simpleLocation(location());
topLevel.text = text();
}
return topLevel;
return parseSymbol(left, rest, {'+': 'add', '-': 'subtract'});
}
/ MultiplyDivide

MultiplyDivide
= _ left:Factor rest:(('*' / '/') Factor)* _ {
const topLevel = rest.reduce((acc, curr) => ({
type: 'function',
name: curr[0] === '*' ? 'multiply' : 'divide',
args: [acc, curr[1]],
}), left);
if (typeof topLevel === 'object') {
topLevel.location = simpleLocation(location());
topLevel.text = text();
}
return topLevel;
return parseSymbol(left, rest, {'*': 'multiply', '/': 'divide'});
}
/ Factor

Factor
= Group
/ Function
/ Literal

// Because of the new Comparison syntax it is required a new Group type
// the previous Group has been renamed into ExpressionGroup while
// a new Group type has been defined to exclude the Comparison type from it
Group
= _ '(' _ expr:MathOperation _ ')' _ {
return expr
}

ExpressionGroup
= _ '(' _ expr:Expression _ ')' _ {
return expr
}
Expand Down
40 changes: 40 additions & 0 deletions packages/kbn-tinymath/src/functions/comparison/eq.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/**
* Performs an equality comparison between two values.
* @param {number|number[]} a a number or an array of numbers
* @param {number|number[]} b a number or an array of numbers
* @return {boolean} Returns true if `a` and `b` are equal, false otherwise. Returns an array with the equality comparison of each element if `a` is an array.
* @throws `'Missing b value'` if `b` is not provided
* @throws `'Array length mismatch'` if `args` contains arrays of different lengths
* @example
* eq(1, 1) // returns true
* eq(1, 2) // returns false
* eq([1, 2], 1) // returns [true, false]
* eq([1, 2], [1, 2]) // returns [true, true]
*/

module.exports = { eq };

function eq(a, b) {
if (b == null) {
throw new Error('Missing b value');
}
if (Array.isArray(a)) {
if (!Array.isArray(b)) {
return a.every((v) => v === b);
}
if (a.length !== b.length) {
throw new Error('Array length mismatch');
}
return a.every((v, i) => v === b[i]);
}

return a === b;
}
40 changes: 40 additions & 0 deletions packages/kbn-tinymath/src/functions/comparison/gt.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/**
* Performs a greater than comparison between two values.
* @param {number|number[]} a a number or an array of numbers
* @param {number|number[]} b a number or an array of numbers
* @return {boolean} Returns true if `a` is greater than `b`, false otherwise. Returns an array with the greater than comparison of each element if `a` is an array.
* @throws `'Missing b value'` if `b` is not provided
* @throws `'Array length mismatch'` if `args` contains arrays of different lengths
* @example
* gt(1, 1) // returns false
* gt(2, 1) // returns true
* gt([1, 2], 1) // returns [true, false]
* gt([1, 2], [2, 1]) // returns [false, true]
*/

module.exports = { gt };

function gt(a, b) {
if (b == null) {
throw new Error('Missing b value');
}
if (Array.isArray(a)) {
if (!Array.isArray(b)) {
return a.every((v) => v > b);
}
if (a.length !== b.length) {
throw new Error('Array length mismatch');
}
return a.every((v, i) => v > b[i]);
}

return a > b;
}
29 changes: 29 additions & 0 deletions packages/kbn-tinymath/src/functions/comparison/gte.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

const { eq } = require('./eq');
const { gt } = require('./gt');

/**
* Performs a greater than or equal comparison between two values.
* @param {number|number[]} a a number or an array of numbers
* @param {number|number[]} b a number or an array of numbers
* @return {boolean} Returns true if `a` is greater than or equal to `b`, false otherwise. Returns an array with the greater than or equal comparison of each element if `a` is an array.
* @throws `'Array length mismatch'` if `args` contains arrays of different lengths
* @example
* gte(1, 1) // returns true
* gte(1, 2) // returns false
* gte([1, 2], 2) // returns [false, true]
* gte([1, 2], [1, 1]) // returns [true, true]
*/

module.exports = { gte };

function gte(a, b) {
return eq(a, b) || gt(a, b);
}
39 changes: 39 additions & 0 deletions packages/kbn-tinymath/src/functions/comparison/ifelse.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/**
* Evaluates the a conditional argument and returns one of the two values based on that.
* @param {(boolean)} cond a boolean value
* @param {(any|any[])} a a value or an array of any values
* @param {(any|any[])} b a value or an array of any values
* @return {(any|any[])} if the value of cond is truthy, return `a`, otherwise return `b`.
* @throws `'Condition clause is of the wrong type'` if the `cond` provided is not of boolean type
* @throws `'Missing a value'` if `a` is not provided
* @throws `'Missing b value'` if `b` is not provided
* @example
* 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]
*/

module.exports = { ifelse };

function ifelse(cond, a, b) {
if (typeof cond !== 'boolean') {
throw Error('Condition clause is of the wrong type');
}
if (a == null) {
throw new Error('Missing a value');
}
if (b == null) {
throw new Error('Missing b value');
}
return cond ? a : b;
}

ifelse.skipNumberValidation = true;
16 changes: 16 additions & 0 deletions packages/kbn-tinymath/src/functions/comparison/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

const { eq } = require('./eq');
const { lt } = require('./lt');
const { gt } = require('./gt');
const { lte } = require('./lte');
const { gte } = require('./gte');
const { ifelse } = require('./ifelse');

module.exports = { eq, lt, gt, lte, gte, ifelse };
40 changes: 40 additions & 0 deletions packages/kbn-tinymath/src/functions/comparison/lt.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

/**
* Performs a lower than comparison between two values.
* @param {number|number[]} a a number or an array of numbers
* @param {number|number[]} b a number or an array of numbers
* @return {boolean} Returns true if `a` is lower than `b`, false otherwise. Returns an array with the lower than comparison of each element if `a` is an array.
* @throws `'Missing b value'` if `b` is not provided
* @throws `'Array length mismatch'` if `args` contains arrays of different lengths
* @example
* lt(1, 1) // returns false
* lt(1, 2) // returns true
* lt([1, 2], 2) // returns [true, false]
* lt([1, 2], [1, 2]) // returns [false, false]
*/

module.exports = { lt };

function lt(a, b) {
if (b == null) {
throw new Error('Missing b value');
}
if (Array.isArray(a)) {
if (!Array.isArray(b)) {
return a.every((v) => v < b);
}
if (a.length !== b.length) {
throw new Error('Array length mismatch');
}
return a.every((v, i) => v < b[i]);
}

return a < b;
}
29 changes: 29 additions & 0 deletions packages/kbn-tinymath/src/functions/comparison/lte.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

const { eq } = require('./eq');
const { lt } = require('./lt');

/**
* Performs a lower than or equal comparison between two values.
* @param {number|number[]} a a number or an array of numbers
* @param {number|number[]} b a number or an array of numbers
* @return {boolean} Returns true if `a` is lower than or equal to `b`, false otherwise. Returns an array with the lower than or equal comparison of each element if `a` is an array.
* @throws `'Array length mismatch'` if `args` contains arrays of different lengths
* @example
* lte(1, 1) // returns true
* lte(1, 2) // returns true
* lte([1, 2], 2) // returns [true, true]
* lte([1, 2], [1, 1]) // returns [true, false]
*/

module.exports = { lte };

function lte(a, b) {
return eq(a, b) || lt(a, b);
}
7 changes: 6 additions & 1 deletion packages/kbn-tinymath/src/functions/divide.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ function divide(a, b) {
return val / b[i];
});
}
if (Array.isArray(b)) return b.map((b) => a / b);
if (Array.isArray(b)) {
return b.map((bi) => {
if (bi === 0) throw new Error('Cannot divide by 0');
return a / bi;
});
}
Comment on lines +31 to +36
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (b === 0) throw new Error('Cannot divide by 0');
if (Array.isArray(a)) return a.map((a) => a / b);
return a / b;
Expand Down
Loading