-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 16 commits
e560bef
6a78199
96e27cc
c74cf67
5784a15
738076a
3944ae2
5d4df23
66289e6
12dcfc2
ed96fbe
9cee512
11442e4
5dc906c
c22a3d5
6f0874c
00bfae4
ab1f5ad
40d9da8
4747225
f4f48f2
0bb5e4d
125ecdd
f26e9b7
3abc4f4
4a7e738
4cf78d5
0efef8b
441db03
3777811
6777d31
0a95ab4
67a6e1a
79eb184
3eb20bc
098a745
38547c9
c81742c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,32 @@ | |
max: location.end.offset | ||
} | ||
} | ||
|
||
const symbolsToFn = { | ||
'+': 'add', '-': 'subtract', | ||
'*': 'multiply', '/': 'divide', | ||
'<': 'lt', '>': 'gt', '==': 'eq', | ||
'<=': '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 | ||
|
@@ -70,45 +96,55 @@ Variable | |
|
||
// expressions | ||
|
||
// An Expression can be of 3 different types: | ||
// * a Comparison operation, which can contain recursive MathOperations inside | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😓 . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// * 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 | ||
} | ||
|
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; | ||
} |
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; | ||
} |
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); | ||
} |
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; |
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 }; |
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; | ||
} |
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); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (b === 0) throw new Error('Cannot divide by 0'); | ||
if (Array.isArray(a)) return a.map((a) => a / b); | ||
return a / b; | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 "=" ?
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!There was a problem hiding this comment.
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:
exec
Canvas
it is possible to use a variable for the comparison:math "ifelse(total_errors = 37, 1, 0)"
which will raise an error about unsupportedNamed parameters
. This is usingexec
underneath. This is probably a non-issue as conditional logic can be performed elsewhereAll 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==
.There was a problem hiding this comment.
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
=
:=
(and magically add another==
).