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.

Sign up for GitHub

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] Create mathColumn function to improve performance #101908

Merged
merged 6 commits into from
Jun 16, 2021
Merged

[Lens] Create mathColumn function to improve performance #101908

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
08121d9
[Lens] Create mathColumn function to improve performance
wylieconlon Jun 10, 2021
517be0e
Merge remote-tracking branch 'origin/master' into lens/formula-perfor…
wylieconlon Jun 14, 2021
22f61e9
Fix empty formula case
wylieconlon Jun 14, 2021
4b65c74
Merge remote-tracking branch 'origin/master' into lens/formula-perfor…
wylieconlon Jun 15, 2021
9d1b40d
Fix tinymath memoization
wylieconlon Jun 15, 2021
c3fe29d
Merge branch 'master' into lens/formula-performance
kibanamachine Jun 15, 2021
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
3 changes: 2 additions & 1 deletion packages/kbn-tinymath/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

const { get } = require('lodash');
const memoizeOne = require('memoize-one');
// eslint-disable-next-line import/no-unresolved
const { parse: parseFn } = require('../grammar');
const { functions: includedFunctions } = require('./functions');
Expand All @@ -23,7 +24,7 @@ function parse(input, options) {
}

try {
return parseFn(input, options);
return memoizeOne(parseFn)(input, options);
Copy link
Contributor

@flash1293 flash1293 Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Didn't test (I can do tomorrow), but is this actually memoizing? Looking at the memoize-one source code, it seems like memoizeOne itself is not memoized on the passed-in function so it would create a new memoization closure on each call without actually ever hitting the cache.

Looks like the memoizeOne call should be moved outside of the parse function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right, the memoizeOne function returns a instance each time!

} catch (e) {
throw new Error(`Failed to parse expression. ${e.message}`);
}
Expand Down
4 changes: 2 additions & 2 deletions ...ck/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/formula.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,11 @@ export const formulaOperation: OperationDefinition<
return [
{
type: 'function',
function: 'mathColumn',
function: currentColumn.references.length ? 'mathColumn' : 'mapColumn',
arguments: {
id: [columnId],
name: [label || defaultLabel],
expression: [currentColumn.references.length ? `"${currentColumn.references[0]}"` : ``],
expression: [currentColumn.references.length ? `"${currentColumn.references[0]}"` : ''],
},
},
];
Expand Down