From dbe419acbafada4be8ffc129ffd702305133257b Mon Sep 17 00:00:00 2001 From: Catherine Liu Date: Tue, 13 Feb 2018 15:06:04 -0700 Subject: [PATCH] Replaced MathJS with Tinymath (#321) * Replaced mathjs with tinymath in math.js. Wrote unit tests for math.js. Upgraded tinymath * Added string and bool columns to testTable for math.js tests. Added unit tests for math.js * Added columns to testTable in math.js test file * Upgraded tinymath to version 0.1.8 * Created function to convert datable to math context for math.js * Replaced pivotObjectArray with datableToMathContext * Wrote unit tests for math.js * Fixed unit test for math.js. Removed references to mathjs package in math.js * Uncommented line to run server tests * Refactored math.js test file * Replaced every instance of Object.value with value from lodash * Create test file for pointseries. Wrote unit tests for pointseries. * Added unit tests for pointseries * Moved 'server/functions/pointseries.js' to 'server/functions/pointseries/index.js'. Updated unit tests. Created lib folder for pointseries helper functions * Removed map function from lodash * Added unit test for pointseries.js * Refactored pointseries and replaced mathJS functions with tinymath functions * Removed common/lib/math.js. Replaced reference to math.js with tinymath in handlebars.js. * Replaced mathjs with tinymath in datacolumn.js * Refactored pointseries.js. Replaced mathjs with tinymath. Removed unused lodash functions * Removed mathjs package and generated docs * Fixed unit tests. Fixed getType function in math.js * Moved getFormObject function to a separate file. Imported getFormObject into datacolumn.js. Rewrote getFormObject to work with parse from tinymath * Rewrote getFormObject to work with parse from tinymath. Wrote unit tests for getFormObject * Added unit test to test runtime of pointseries using demodata * Updated demodata test * Removed console logs * Updated runtime test for pointseries * Removed runtime test for pointseries * Added error handling for empty datatables in math.js. Updated unit tests * Upgraded tinymath to 0.1.9 * Changed error message thrown in getFormObject, updated unit tests * Created test datatables file * Cleaned up tests for math.js. * Wrote tests for datatableToMathContext * Cleaned up unit tests for pointseries * wrote tests for isColumnReference * Renamed getType in pointseries to getExpressionType * Pulled out functions out of pointseries into lib functions and added imports to pointseries * Wrote unit tests for getFieldType * Wrote unit tests for getFieldNames * Wrote unit tests for getExpressionType. Refactored pointseries. Updated unit tests for pointseries helper functions. * Removed mode from dropdown menu in SimpleMathFunction * Cleaned up unit tests for pointseries and helper functions --- .../__tests__/fixtures/test_tables.js | 98 +++++++++++ common/functions/__tests__/math.js | 78 ++++++++- common/functions/math.js | 38 ++-- .../__tests__/datatable_to_math_context.js | 14 ++ common/lib/__tests__/get_field_type.js | 17 ++ common/lib/datatable_to_math_context.js | 7 + common/lib/get_field_type.js | 7 + common/lib/handlebars.js | 4 +- common/lib/math.js | 8 - package.json | 4 +- .../datacolumn/__tests__/get_form_object.js | 29 +++ .../arg_types/datacolumn/datacolumn.js | 32 +--- .../arg_types/datacolumn/get_form_object.js | 35 ++++ .../datacolumn/simple_math_function.js | 1 - .../__tests__/get_expression_type.js | 29 +++ server/functions/__tests__/get_field_names.js | 15 ++ .../__tests__/is_column_reference.js | 13 ++ server/functions/__tests__/pointseries.js | 165 ++++++++++++++++++ .../{pointseries.js => pointseries/index.js} | 117 ++++++------- .../pointseries/lib/get_expression_type.js | 30 ++++ .../pointseries/lib/get_field_names.js | 11 ++ .../pointseries/lib/is_column_reference.js | 7 + 22 files changed, 629 insertions(+), 130 deletions(-) create mode 100644 common/functions/__tests__/fixtures/test_tables.js create mode 100644 common/lib/__tests__/datatable_to_math_context.js create mode 100644 common/lib/__tests__/get_field_type.js create mode 100644 common/lib/datatable_to_math_context.js create mode 100644 common/lib/get_field_type.js delete mode 100644 common/lib/math.js create mode 100644 public/expression_types/arg_types/datacolumn/__tests__/get_form_object.js create mode 100644 public/expression_types/arg_types/datacolumn/get_form_object.js create mode 100644 server/functions/__tests__/get_expression_type.js create mode 100644 server/functions/__tests__/get_field_names.js create mode 100644 server/functions/__tests__/is_column_reference.js create mode 100644 server/functions/__tests__/pointseries.js rename server/functions/{pointseries.js => pointseries/index.js} (55%) create mode 100644 server/functions/pointseries/lib/get_expression_type.js create mode 100644 server/functions/pointseries/lib/get_field_names.js create mode 100644 server/functions/pointseries/lib/is_column_reference.js diff --git a/common/functions/__tests__/fixtures/test_tables.js b/common/functions/__tests__/fixtures/test_tables.js new file mode 100644 index 0000000000000..43ca2d27e9bec --- /dev/null +++ b/common/functions/__tests__/fixtures/test_tables.js @@ -0,0 +1,98 @@ +const emptyTable = { + type: 'datatable', + columns: [], + rows: [], +}; + +const testTable = { + type: 'datatable', + columns: [ + { + name: 'name', + type: 'string', + }, + { + name: 'time', + type: 'date', + }, + { + name: 'price', + type: 'number', + }, + { + name: 'quantity', + type: 'number', + }, + { + name: 'in_stock', + type: 'boolean', + }, + ], + rows: [ + { + name: 'product1', + time: 1517842800950, //05 Feb 2018 15:00:00 GMT + price: 605, + quantity: 100, + in_stock: true, + }, + { + name: 'product1', + time: 1517929200950, //06 Feb 2018 15:00:00 GMT + price: 583, + quantity: 200, + in_stock: true, + }, + { + name: 'product1', + time: 1518015600950, //07 Feb 2018 15:00:00 GMT + price: 420, + quantity: 300, + in_stock: true, + }, + { + name: 'product2', + time: 1517842800950, //05 Feb 2018 15:00:00 GMT + price: 216, + quantity: 350, + in_stock: false, + }, + { + name: 'product2', + time: 1517929200950, //06 Feb 2018 15:00:00 GMT + price: 200, + quantity: 256, + in_stock: false, + }, + { + name: 'product2', + time: 1518015600950, //07 Feb 2018 15:00:00 GMT + price: 190, + quantity: 231, + in_stock: false, + }, + { + name: 'product3', + time: 1517842800950, //05 Feb 2018 15:00:00 GMT + price: 67, + quantity: 240, + in_stock: true, + }, + { + name: 'product4', + time: 1517842800950, //05 Feb 2018 15:00:00 GMT + price: 311, + quantity: 447, + in_stock: false, + }, + { + name: 'product5', + time: 1517842800950, //05 Feb 2018 15:00:00 GMT + price: 288, + quantity: 384, + in_stock: true, + }, + ], +}; + +export { emptyTable, testTable }; diff --git a/common/functions/__tests__/math.js b/common/functions/__tests__/math.js index 13235305dd42c..5b539b8cc45ab 100644 --- a/common/functions/__tests__/math.js +++ b/common/functions/__tests__/math.js @@ -1,7 +1,9 @@ import expect from 'expect.js'; import { math } from '../math.js'; +import { emptyTable, testTable } from './fixtures/test_tables'; describe('math', () => { + const fn = math().fn; describe('spec', () => { it('is a function', () => { expect(math).to.be.a('function'); @@ -9,13 +11,79 @@ describe('math', () => { }); describe('function', () => { - let fn; - beforeEach(() => { - fn = math().fn; - }); - it('is a function', () => { expect(fn).to.be.a('function'); }); + + it('math expressions, no context', () => { + expect(fn(null, { _: '10' })).to.be.equal(10); + expect(fn(null, { _: '10.5345' })).to.be.equal(10.5345); + expect(fn(null, { _: '123 + 456' })).to.be.equal(579); + expect(fn(null, { _: '100 - 46' })).to.be.equal(54); + expect(fn(1, { _: '100 / 5' })).to.be.equal(20); + expect(fn('foo', { _: '100 / 5' })).to.be.equal(20); + expect(fn(true, { _: '100 / 5' })).to.be.equal(20); + expect(fn(testTable, { _: '100 * 5' })).to.be.equal(500); + expect(fn(emptyTable, { _: '100 * 5' })).to.be.equal(500); + }); + + it('math expressions, context as number', () => { + expect(fn(23.23, { _: 'floor(value)' })).to.be.equal(23); + expect(fn(-103, { _: 'abs(value)' })).to.be.equal(103); + }); + + it('math expressions, context as datatable', () => { + expect(fn(testTable, { _: 'count(price)' })).to.be.equal(9); + expect(fn(testTable, { _: 'sum(quantity)' })).to.be.equal(2508); + expect(fn(testTable, { _: 'mean(price)' })).to.be.equal(320); + expect(fn(testTable, { _: 'min(price)' })).to.be.equal(67); + expect(fn(testTable, { _: 'median(quantity)' })).to.be.equal(256); + expect(fn(testTable, { _: 'max(price)' })).to.be.equal(605); + }); + }); + + describe('invalid expression', () => { + it('throws when expression evaluates to an array', () => { + expect(fn) + .withArgs(testTable, { _: 'multiply(price, 2)' }) + .to.throwException(e => { + expect(e.message).to.be( + 'Expressions must return a single number. Try wrapping your expression in mean() or sum()' + ); + }); + }); + it('throws when using an unknown context variable', () => { + expect(fn) + .withArgs(testTable, { _: 'sum(foo)' }) + .to.throwException(e => { + expect(e.message).to.be('Unknown variable: foo'); + }); + }); + it('throws when using non-numeric data', () => { + expect(fn) + .withArgs(testTable, { _: 'mean(name)' }) + .to.throwException(e => { + expect(e.message).to.be('Unknown variable: name'); + }); + expect(fn) + .withArgs(testTable, { _: 'max(in_stock)' }) + .to.throwException(e => { + expect(e.message).to.be('Unknown variable: in_stock'); + }); + }); + it('throws when missing expression', () => { + expect(fn) + .withArgs(testTable, { _: '' }) + .to.throwException(e => { + expect(e.message).to.be('Empty expression'); + }); + }); + it('throws when passing a context variable from an empty datatable', () => { + expect(fn) + .withArgs(emptyTable, { _: 'mean(foo)' }) + .to.throwException(e => { + expect(e.message).to.be('Empty datatable'); + }); + }); }); }); diff --git a/common/functions/math.js b/common/functions/math.js index c7bd70c978981..0a74f8d2afa47 100644 --- a/common/functions/math.js +++ b/common/functions/math.js @@ -1,12 +1,11 @@ -import { map } from 'lodash'; -import mathjs from 'mathjs'; -import { pivotObjectArray } from '../lib/pivot_object_array.js'; +import { evaluate } from 'tinymath'; +import { datatableToMathContext } from '../lib/datatable_to_math_context.js'; export const math = () => ({ name: 'math', type: 'number', help: - 'Interpret a mathJS expression, with a number or datatable as context. Datatable columns are available by their column name. ' + + 'Interpret a math expression, with a number or datatable as context. Datatable columns are available by their column name. ' + 'If you pass in a number it is available as "value" (without the quotes)', context: { types: ['number', 'datatable'], @@ -14,18 +13,31 @@ export const math = () => ({ args: { _: { types: ['string'], - help: - 'An evaluated MathJS expression. (See http://mathjs.org/docs/expressions/parsing.html#eval)', + help: 'An evaluated tinymath expression. (See https://github.com/elastic/tinymath)', }, }, fn: (context, args) => { + if (args._.trim() === '') { + throw new Error('Empty expression'); + } const isDatatable = context && context.type === 'datatable'; - const mathContext = isDatatable - ? pivotObjectArray(context.rows, map(context.columns, 'name')) - : { value: context }; - const result = mathjs.eval(args._, mathContext); - if (typeof result !== 'number') - throw new Error('Failed to execute math expression. Check your column names'); - return result; + const mathContext = isDatatable ? datatableToMathContext(context) : { value: context }; + try { + const result = evaluate(args._, mathContext); + if (Array.isArray(result)) { + throw new Error( + 'Expressions must return a single number. Try wrapping your expression in mean() or sum()' + ); + } + if (typeof result !== 'number') + throw new Error('Failed to execute math expression. Check your column names'); + return result; + } catch (e) { + if (context.rows.length === 0) { + throw new Error('Empty datatable'); + } else { + throw e; + } + } }, }); diff --git a/common/lib/__tests__/datatable_to_math_context.js b/common/lib/__tests__/datatable_to_math_context.js new file mode 100644 index 0000000000000..66a0b857413d6 --- /dev/null +++ b/common/lib/__tests__/datatable_to_math_context.js @@ -0,0 +1,14 @@ +import expect from 'expect.js'; +import { datatableToMathContext } from '../datatable_to_math_context'; +import { emptyTable, testTable } from '../../functions/__tests__/fixtures/test_tables'; +describe('datatableToMathContext', () => { + it('empty table', () => { + expect(datatableToMathContext(emptyTable)).to.be.eql({}); + }); + it('filters out non-numeric columns and pivots datatable', () => { + expect(datatableToMathContext(testTable)).to.be.eql({ + price: [605, 583, 420, 216, 200, 190, 67, 311, 288], + quantity: [100, 200, 300, 350, 256, 231, 240, 447, 384], + }); + }); +}); diff --git a/common/lib/__tests__/get_field_type.js b/common/lib/__tests__/get_field_type.js new file mode 100644 index 0000000000000..a7a660eb7f8b8 --- /dev/null +++ b/common/lib/__tests__/get_field_type.js @@ -0,0 +1,17 @@ +import expect from 'expect.js'; +import { getFieldType } from '../get_field_type'; +import { emptyTable, testTable } from '../../functions/__tests__/fixtures/test_tables'; + +describe('getFieldType', () => { + it('returns type of a field in a datatable', () => { + expect(getFieldType(testTable.columns, 'name')).to.be('string'); + expect(getFieldType(testTable.columns, 'time')).to.be('date'); + expect(getFieldType(testTable.columns, 'price')).to.be('number'); + expect(getFieldType(testTable.columns, 'quantity')).to.be('number'); + expect(getFieldType(testTable.columns, 'in_stock')).to.be('boolean'); + }); + it(`returns 'null' if field does not exist in datatable`, () => { + expect(getFieldType(testTable.columns, 'foo')).to.be('null'); + expect(getFieldType(emptyTable.columns, 'foo')).to.be('null'); + }); +}); diff --git a/common/lib/datatable_to_math_context.js b/common/lib/datatable_to_math_context.js new file mode 100644 index 0000000000000..c288515dda2f7 --- /dev/null +++ b/common/lib/datatable_to_math_context.js @@ -0,0 +1,7 @@ +import { pivotObjectArray } from '../lib/pivot_object_array.js'; + +// filters columns with type number and passes filtered columns to pivotObjectArray +export function datatableToMathContext(datatable) { + const filteredColumns = datatable.columns.filter(col => col.type === 'number'); + return pivotObjectArray(datatable.rows, filteredColumns.map(col => col.name)); +} diff --git a/common/lib/get_field_type.js b/common/lib/get_field_type.js new file mode 100644 index 0000000000000..4c1c534e7ddcc --- /dev/null +++ b/common/lib/get_field_type.js @@ -0,0 +1,7 @@ +// put in common + +export function getFieldType(columns, field) { + if (!field) return 'null'; + const column = columns.find(column => column.name === field); + return column ? column.type : 'null'; +} diff --git a/common/lib/handlebars.js b/common/lib/handlebars.js index fda5fad0b9641..21a8a0d6f279f 100644 --- a/common/lib/handlebars.js +++ b/common/lib/handlebars.js @@ -1,11 +1,11 @@ import Hbars from 'handlebars/dist/handlebars'; -import { math } from './math.js'; +import { evaluate } from 'tinymath'; import { pivotObjectArray } from './pivot_object_array.js'; // example use: {{math rows 'mean(price - cost)' 2}} Hbars.registerHelper('math', (rows, expression, precision) => { if (!Array.isArray(rows)) return 'MATH ERROR: first argument must be an array'; - const value = math.eval(expression, pivotObjectArray(rows)); + const value = evaluate(expression, pivotObjectArray(rows)); try { return precision ? value.toFixed(precision) : value; } catch (e) { diff --git a/common/lib/math.js b/common/lib/math.js deleted file mode 100644 index b6d5ac540299b..0000000000000 --- a/common/lib/math.js +++ /dev/null @@ -1,8 +0,0 @@ -import mathjs from 'mathjs'; - -const isAlphaOriginal = mathjs.expression.parse.isAlpha; -mathjs.expression.parse.isAlpha = function(c, cPrev, cNext) { - return isAlphaOriginal(c, cPrev, cNext) || c === '@' || c === '.'; -}; - -export const math = mathjs; diff --git a/package.json b/package.json index 714f344f21b5a..26776c18eff73 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,6 @@ "lodash.uniqby": "^4.7.0", "lz-string": "^1.4.4", "markdown-it": "^8.3.2", - "mathjs": "^3.12.3", "metalsmith-snippet": "^2.0.0", "moment": "^2.18.1", "object-path-immutable": "^0.5.1", @@ -77,6 +76,7 @@ "redux-thunks": "^1.0.0", "socket.io": "^1.7.3", "style-it": "^1.6.12", + "tinymath": "0.1.9", "uuid": "^3.0.1" }, "devDependencies": { @@ -131,4 +131,4 @@ "sinon": "^2.3.2", "through2": "^2.0.3" } -} \ No newline at end of file +} diff --git a/public/expression_types/arg_types/datacolumn/__tests__/get_form_object.js b/public/expression_types/arg_types/datacolumn/__tests__/get_form_object.js new file mode 100644 index 0000000000000..049febd6aee27 --- /dev/null +++ b/public/expression_types/arg_types/datacolumn/__tests__/get_form_object.js @@ -0,0 +1,29 @@ +import expect from 'expect.js'; +import { getFormObject } from '../get_form_object'; + +describe('getFormObject', () => { + describe('valid input', () => { + it('string', () => { + expect(getFormObject('field')).to.be.eql({ fn: '', column: 'field' }); + }); + it('simple expression', () => { + expect(getFormObject('mean(field)')).to.be.eql({ fn: 'mean', column: 'field' }); + }); + }); + describe('invalid input', () => { + it('number', () => { + expect(getFormObject) + .withArgs('2') + .to.throwException(e => { + expect(e.message).to.be('Cannot render scalar values or complex math expressions'); + }); + }); + it('complex expression', () => { + expect(getFormObject) + .withArgs('mean(field * 3)') + .to.throwException(e => { + expect(e.message).to.be('Cannot render scalar values or complex math expressions'); + }); + }); + }); +}); diff --git a/public/expression_types/arg_types/datacolumn/datacolumn.js b/public/expression_types/arg_types/datacolumn/datacolumn.js index 4fb56c3a78675..ce0578918f17c 100644 --- a/public/expression_types/arg_types/datacolumn/datacolumn.js +++ b/public/expression_types/arg_types/datacolumn/datacolumn.js @@ -3,43 +3,13 @@ import { compose, withPropsOnChange, withHandlers } from 'recompose'; import PropTypes from 'prop-types'; import { FormControl } from 'react-bootstrap'; import { sortBy } from 'lodash'; -import { parse } from 'mathjs'; import { createStatefulPropHoc } from '../../../components/enhance/stateful_prop'; import { getType } from '../../../../common/lib/get_type'; import { SimpleMathFunction } from './simple_math_function'; +import { getFormObject } from './get_form_object'; import './datacolumn.less'; // TODO: Garbage, we could make a much nicer math form that can handle way more. -function getFormObject(argValue) { - if (argValue === '') { - return { - fn: '', - column: '', - }; - } - - // check if the value is a math expression, and set its type if it is - const mathObj = parse(argValue); - - // A symbol node is a plain string, so we guess that they're looking for a column. - if (mathObj.type === 'SymbolNode') { - return { - fn: '', - column: argValue, - }; - - // Check if its a simple function, eg a function wrapping a symbol node - } else if (mathObj.type === 'FunctionNode' && mathObj.args[0].type === 'SymbolNode') { - return { - fn: mathObj.name, - column: mathObj.args[0].name, - }; - - // Screw it, textarea for you my fancy. - } else { - throw new Error(`Invalid math object type: ${mathObj.type}`); - } -} const DatacolumnArgInput = ({ onValueChange, diff --git a/public/expression_types/arg_types/datacolumn/get_form_object.js b/public/expression_types/arg_types/datacolumn/get_form_object.js new file mode 100644 index 0000000000000..65e5cece2422a --- /dev/null +++ b/public/expression_types/arg_types/datacolumn/get_form_object.js @@ -0,0 +1,35 @@ +import { parse } from 'tinymath'; +// break out into separate function, write unit tests first +export function getFormObject(argValue) { + if (argValue === '') { + return { + fn: '', + column: '', + }; + } + + // check if the value is a math expression, and set its type if it is + const mathObj = parse(argValue); + // A symbol node is a plain string, so we guess that they're looking for a column. + if (typeof mathObj === 'string') { + return { + fn: '', + column: argValue, + }; + } + + // Check if its a simple function, eg a function wrapping a symbol node, check for only one arg of type string + if ( + typeof mathObj === 'object' && + mathObj.args.length === 1 && + typeof mathObj.args[0] === 'string' + ) { + return { + fn: mathObj.name, + column: mathObj.args[0], + }; + } + + // Screw it, textarea for you my fancy. + throw new Error(`Cannot render scalar values or complex math expressions`); +} diff --git a/public/expression_types/arg_types/datacolumn/simple_math_function.js b/public/expression_types/arg_types/datacolumn/simple_math_function.js index 0338e4ab49747..f4d97e80c800a 100644 --- a/public/expression_types/arg_types/datacolumn/simple_math_function.js +++ b/public/expression_types/arg_types/datacolumn/simple_math_function.js @@ -11,7 +11,6 @@ export const SimpleMathFunction = ({ onChange, value, inputRef }) => { { label: 'Max', value: 'max' }, { label: 'Min', value: 'min' }, { label: 'Median', value: 'median' }, - { label: 'Mode', value: 'mode' }, ]; const onSelect = ev => onChange(ev.target.value); diff --git a/server/functions/__tests__/get_expression_type.js b/server/functions/__tests__/get_expression_type.js new file mode 100644 index 0000000000000..7a70e79d4465b --- /dev/null +++ b/server/functions/__tests__/get_expression_type.js @@ -0,0 +1,29 @@ +import expect from 'expect.js'; +import { getExpressionType } from '../pointseries/lib/get_expression_type'; +import { emptyTable, testTable } from '../../../common/functions/__tests__/fixtures/test_tables'; + +describe('getExpressionType', () => { + it('returns the result type of an evaluated math expression', () => { + expect(getExpressionType(testTable.columns, '2')).to.be.equal('number'); + expect(getExpressionType(testTable.colunns, '2 + 3')).to.be.equal('number'); + expect(getExpressionType(testTable.columns, 'name')).to.be.equal('string'); + expect(getExpressionType(testTable.columns, 'time')).to.be.equal('date'); + expect(getExpressionType(testTable.columns, 'price')).to.be.equal('number'); + expect(getExpressionType(testTable.columns, 'quantity')).to.be.equal('number'); + expect(getExpressionType(testTable.columns, 'in_stock')).to.be.equal('boolean'); + expect(getExpressionType(testTable.columns, 'mean(price)')).to.be.equal('number'); + expect(getExpressionType(testTable.columns, 'count(name)')).to.be.equal('string'); + expect(getExpressionType(testTable.columns, 'random()')).to.be.equal('number'); + expect(getExpressionType(testTable.columns, 'mean(multiply(price,quantity))')).to.be.eql( + 'number' + ); + }); + it('returns date instead of number when referencing date column', () => { + expect(getExpressionType(testTable.columns, 'mean(time)')).to.be.equal('date'); + }); + it(`returns 'null' if referenced field does not exist in datatable`, () => { + expect(getExpressionType(testTable.columns, 'foo')).to.be.equal('null'); + expect(getExpressionType(emptyTable.columns, 'foo')).to.be.equal('null'); + expect(getExpressionType(emptyTable.columns, 'mean(foo)')).to.be.equal('string'); + }); +}); diff --git a/server/functions/__tests__/get_field_names.js b/server/functions/__tests__/get_field_names.js new file mode 100644 index 0000000000000..479602e5bd4f5 --- /dev/null +++ b/server/functions/__tests__/get_field_names.js @@ -0,0 +1,15 @@ +import expect from 'expect.js'; +import { parse } from 'tinymath'; +import { getFieldNames } from '../pointseries/lib/get_field_names'; + +describe('getFieldNames', () => { + it('returns array of field names referenced in a parsed math object', () => { + expect(getFieldNames([], parse('2+3'))).to.be.eql([]); + expect(getFieldNames([], parse('mean(foo)'))).to.be.eql(['foo']); + expect(getFieldNames([], parse('max(foo + bar)'))).to.be.eql(['foo', 'bar']); + expect(getFieldNames([], parse('count(foo) + count(bar)'))).to.be.eql(['foo', 'bar']); + expect( + getFieldNames([], parse('sum(count(foo),count(bar),count(fizz),count(buzz),2,3,4)')) + ).to.be.eql(['foo', 'bar', 'fizz', 'buzz']); + }); +}); diff --git a/server/functions/__tests__/is_column_reference.js b/server/functions/__tests__/is_column_reference.js new file mode 100644 index 0000000000000..bc2e4cbd8ea6a --- /dev/null +++ b/server/functions/__tests__/is_column_reference.js @@ -0,0 +1,13 @@ +import expect from 'expect.js'; +import { isColumnReference } from '../pointseries/lib/is_column_reference'; + +describe('isColumnReference', () => { + it('get a string result after parsing math expression', () => { + expect(isColumnReference('field')).to.be(true); + }); + it('non-string', () => { + expect(isColumnReference('2')).to.be(false); + expect(isColumnReference('mean(field)')).to.be(false); + expect(isColumnReference('field * 3')).to.be(false); + }); +}); diff --git a/server/functions/__tests__/pointseries.js b/server/functions/__tests__/pointseries.js new file mode 100644 index 0000000000000..c13dfc14478d9 --- /dev/null +++ b/server/functions/__tests__/pointseries.js @@ -0,0 +1,165 @@ +import expect from 'expect.js'; +import { pointseries } from '../pointseries'; +import { emptyTable, testTable } from '../../../common/functions/__tests__/fixtures/test_tables'; + +describe('pointseries', () => { + const fn = pointseries().fn; + + describe('function', () => { + it('empty datatable, null args', () => { + expect(fn(emptyTable, { x: null, y: null })).to.be.eql({ + type: 'pointseries', + columns: {}, + rows: [], + }); + }); + it('empty datatable, invalid args', () => { + expect(fn(emptyTable, { x: 'name', y: 'price' })).to.be.eql({ + type: 'pointseries', + columns: { + x: { type: 'null', role: 'dimension', expression: 'name' }, + y: { + type: 'null', + role: 'dimension', + expression: 'price', + }, + }, + rows: [], + }); + }); + it('args with constants only', () => { + expect(fn(testTable, { x: '1', y: '2' })).to.be.eql({ + type: 'pointseries', + columns: { + x: { type: 'number', role: 'measure', expression: '1' }, + y: { type: 'number', role: 'measure', expression: '2' }, + }, + rows: [{ x: 1, y: 2 }], + }); + }); + it('invalid args', () => { + expect(fn(testTable, { x: 'name', y: 'quantity * 3' })).to.be.eql({ + type: 'pointseries', + columns: { + x: { + type: 'string', + role: 'dimension', + expression: 'name', + }, + y: { + type: 'number', + role: 'measure', + expression: 'quantity * 3', + }, + }, + rows: [ + { x: 'product1', y: null }, + { x: 'product2', y: null }, + { x: 'product3', y: null }, + { x: 'product4', y: null }, + { x: 'product5', y: null }, + ], + }); + }); + it('args with dimensions only', () => { + expect(fn(testTable, { x: 'name', y: 'price', size: 'quantity' })).to.be.eql({ + type: 'pointseries', + columns: { + x: { + type: 'string', + role: 'dimension', + expression: 'name', + }, + y: { + type: 'number', + role: 'dimension', + expression: 'price', + }, + size: { + type: 'number', + role: 'dimension', + expression: 'quantity', + }, + }, + rows: [ + { x: 'product1', y: 605, size: 100 }, + { x: 'product1', y: 583, size: 200 }, + { x: 'product1', y: 420, size: 300 }, + { x: 'product2', y: 216, size: 350 }, + { x: 'product2', y: 200, size: 256 }, + { x: 'product2', y: 190, size: 231 }, + { x: 'product3', y: 67, size: 240 }, + { x: 'product4', y: 311, size: 447 }, + { x: 'product5', y: 288, size: 384 }, + ], + }); + }); + it('args including measures', () => { + expect(fn(testTable, { x: 'name', y: 'mean(price)', size: 'mean(quantity)' })).to.be.eql({ + type: 'pointseries', + columns: { + x: { + type: 'string', + role: 'dimension', + expression: 'name', + }, + y: { + type: 'number', + role: 'measure', + expression: 'mean(price)', + }, + size: { + type: 'number', + role: 'measure', + expression: 'mean(quantity)', + }, + }, + rows: [ + { x: 'product1', y: 536, size: 200 }, + { x: 'product2', y: 202, size: 279 }, + { x: 'product3', y: 67, size: 240 }, + { x: 'product4', y: 311, size: 447 }, + { x: 'product5', y: 288, size: 384 }, + ], + }); + expect(fn(testTable, { x: 'name', y: 'max(price * quantity + 2)' })).to.be.eql({ + type: 'pointseries', + columns: { + x: { + type: 'string', + role: 'dimension', + expression: 'name', + }, + y: { + type: 'number', + role: 'measure', + expression: 'max(price * quantity + 2)', + }, + }, + rows: [ + { x: 'product1', y: 126002 }, + { x: 'product2', y: 75602 }, + { x: 'product3', y: 16082 }, + { x: 'product4', y: 139019 }, + { x: 'product5', y: 110594 }, + ], + }); + }); + it('args including random()', () => { + const randomPointseries = fn(testTable, { x: 'time', y: 'random()' }); + expect(randomPointseries).to.have.property('type', 'pointseries'); + expect(randomPointseries.columns).to.be.eql({ + x: { type: 'date', role: 'dimension', expression: 'time' }, + y: { + type: 'number', + role: 'measure', + expression: 'random()', + }, + }); + randomPointseries.rows.map((row, i) => { + expect(row.x).to.be(testTable.rows[i].time); + expect(row.y).to.be.within(0, 1); + }); + }); + }); +}); diff --git a/server/functions/pointseries.js b/server/functions/pointseries/index.js similarity index 55% rename from server/functions/pointseries.js rename to server/functions/pointseries/index.js index ead30303748a1..cf6f2ac15a24a 100644 --- a/server/functions/pointseries.js +++ b/server/functions/pointseries/index.js @@ -1,62 +1,21 @@ -import { groupBy, zipObject, omit, uniq, map, mapValues } from 'lodash'; +import { groupBy, zipObject, omit, values } from 'lodash'; import uniqBy from 'lodash.uniqby'; -import pickBy from 'lodash.pickby'; import moment from 'moment'; -import mathjs from 'mathjs'; -import { findInObject } from '../../common/lib/find_in_object'; -import { pivotObjectArray } from '../../common/lib/pivot_object_array.js'; +import { evaluate } from 'tinymath'; +import { pivotObjectArray } from '../../../common/lib/pivot_object_array.js'; +import { datatableToMathContext } from '../../../common/lib/datatable_to_math_context.js'; +import { isColumnReference } from './lib/is_column_reference.js'; +import { getExpressionType } from './lib/get_expression_type'; // TODO: pointseries performs poorly, that's why we run it on the server. -function isColumnReference(mathExpression) { - const parsedMath = mathjs.parse(mathExpression); - if (parsedMath.type === 'SymbolNode') return true; -} - -function isMeasure(mathScope, mathExpression) { - if (isColumnReference(mathExpression)) return false; - - const parsedMath = mathjs.parse(mathExpression); - - if (parsedMath.type !== 'FunctionNode' && parsedMath.type !== 'ConstantNode') { - throw new Error('Expressions must be wrapped in a function such as sum()'); - } - - if (parsedMath.type !== 'ConstantNode') return true; - - // This will throw if the field isn't found on scope. - // Must be a function node! - const evaluated = mathjs.eval(mathExpression, mathScope); - if (typeof evaluated !== 'number') return false; - - return true; -} - -function getFieldType(columns, field) { - if (!field) return 'null'; - const column = columns.find(column => column.name === field); - return column ? column.type : 'null'; -} - -function getType(columns, mathExpression) { - if (isColumnReference(mathExpression)) return getFieldType(columns, mathExpression); - - const parsedMath = mathjs.parse(mathExpression); - const symbolNames = map( - findInObject(parsedMath, (val, name) => val.type === 'SymbolNode' && name !== 'fn'), - 'name' - ); - const symbolTypes = uniq(symbolNames.map(field => getFieldType(columns, field))); - return symbolTypes.length === 1 ? symbolTypes[0] : 'string'; -} - export const pointseries = () => ({ name: 'pointseries', type: 'pointseries', help: - 'Turn a datatable into a point series model. Currently we differentiate measure from dimensions by looking for a MathJS function.' + - 'If you are using MathJS, we treat that argument as a measure, otherwise it is a dimension. Dimensions are combined to create unique ' + - 'keys. Measures are then deduplicated by those keys using the specified MathJS function', + 'Turn a datatable into a point series model. Currently we differentiate measure from dimensions by looking for a tinymath function.' + + 'If you are using tinymath, we treat that argument as a measure, otherwise it is a dimension. Dimensions are combined to create unique ' + + 'keys. Measures are then deduplicated by those keys using the specified tinymath function', context: { types: ['datatable'], }, @@ -85,26 +44,43 @@ export const pointseries = () => ({ // The way the function below is written you can add as many arbitrary named args as you want. }, fn: (context, args) => { + // Note: can't replace pivotObjectArray with datatableToMathContext, lose name of non-numeric columns const mathScope = pivotObjectArray(context.rows, context.columns.map(col => col.name)); - const dimensionNames = Object.keys(pickBy(args, val => !isMeasure(mathScope, val))).filter( - arg => args[arg] != null - ); - const measureNames = Object.keys(pickBy(args, val => isMeasure(mathScope, val))); - const columns = mapValues(args, arg => { - if (!arg) return; - // TODO: We're setting the measure/dimension break down here, but it should probably come from the datatable right? - return { - type: getType(context.columns, arg), - role: isMeasure(mathScope, arg) ? 'measure' : 'dimension', - expression: arg, - }; + + const measureNames = []; + const dimensionNames = []; + const columns = {}; + + // Separates args into dimensions and measures arrays by checking if arg is a column reference (dimension) + Object.keys(args).forEach(arg => { + const mathExp = args[arg]; + + if (mathExp != null) { + const col = { + type: '', + role: '', + expression: mathExp, + }; + + if (isColumnReference(mathExp)) { + dimensionNames.push(arg); + col.type = getExpressionType(context.columns, mathExp); + col.role = 'dimension'; + } else { + measureNames.push(arg); + col.type = 'number'; + col.role = 'measure'; + } + + columns[arg] = col; + } }); const PRIMARY_KEY = '%%CANVAS_POINTSERIES_PRIMARY_KEY%%'; const rows = context.rows.map((row, i) => ({ ...row, [PRIMARY_KEY]: i })); function normalizeValue(expression, value) { - switch (getType(context.columns, expression)) { + switch (getExpressionType(context.columns, expression)) { case 'string': return String(value); case 'number': @@ -125,7 +101,7 @@ export const pointseries = () => ({ const colName = args[dimension]; try { acc[dimension] = colName - ? normalizeValue(colName, mathjs.eval(colName, mathScope)[i]) + ? normalizeValue(colName, evaluate(colName, mathScope)[i]) : '_all'; } catch (e) { // TODO: handle invalid column names... @@ -150,13 +126,18 @@ export const pointseries = () => ({ }); // Then compute that 1 value for each measure - Object.values(measureKeys).forEach(rows => { + values(measureKeys).forEach(rows => { const subtable = { type: 'datatable', columns: context.columns, rows: rows }; - const subScope = pivotObjectArray(subtable.rows, subtable.columns.map(col => col.name)); + const subScope = datatableToMathContext(subtable); const measureValues = measureNames.map(measure => { try { - return mathjs.eval(args[measure], subScope); + const ev = evaluate(args[measure], subScope); + if (Array.isArray(ev)) { + throw new Error('Expressions must be wrapped in a function such as sum()'); + } + return evaluate(args[measure], subScope); } catch (e) { + // TODO: don't catch if eval to Array return null; } }); @@ -168,7 +149,7 @@ export const pointseries = () => ({ // It only makes sense to uniq the rows in a point series as 2 values can not exist in the exact same place at the same time. const resultingRows = uniqBy( - Object.values(results).map(row => omit(row, PRIMARY_KEY)), + values(results).map(row => omit(row, PRIMARY_KEY)), JSON.stringify ); diff --git a/server/functions/pointseries/lib/get_expression_type.js b/server/functions/pointseries/lib/get_expression_type.js new file mode 100644 index 0000000000000..581ee3be4ae21 --- /dev/null +++ b/server/functions/pointseries/lib/get_expression_type.js @@ -0,0 +1,30 @@ +import { parse } from 'tinymath'; +import { getFieldType } from '../../../../common/lib/get_field_type'; +import { isColumnReference } from './is_column_reference'; +import { getFieldNames } from './get_field_names'; + +export function getExpressionType(columns, mathExpression) { + // if isColumnReference returns true, then mathExpression is just a string referencing a column in a datatable + if (isColumnReference(mathExpression)) return getFieldType(columns, mathExpression); + + const parsedMath = parse(mathExpression); + + if (parsedMath.args) { + const fieldNames = parsedMath.args.reduce(getFieldNames, []); + + if (fieldNames.length > 0) { + const fieldTypes = fieldNames.reduce((types, name) => { + const type = getFieldType(columns, name); + console.log(type); + if (type !== 'null' && types.indexOf(type) === -1) { + return types.concat(type); + } + return types; + }, []); + + return fieldTypes.length === 1 ? fieldTypes[0] : 'string'; + } + return 'number'; + } + return typeof parsedMath; +} diff --git a/server/functions/pointseries/lib/get_field_names.js b/server/functions/pointseries/lib/get_field_names.js new file mode 100644 index 0000000000000..afd1b9f7b37e8 --- /dev/null +++ b/server/functions/pointseries/lib/get_field_names.js @@ -0,0 +1,11 @@ +export function getFieldNames(names, arg) { + if (arg.args != null) { + return names.concat(arg.args.reduce(getFieldNames, [])); + } + + if (typeof arg === 'string') { + return names.concat(arg); + } + + return names; +} diff --git a/server/functions/pointseries/lib/is_column_reference.js b/server/functions/pointseries/lib/is_column_reference.js new file mode 100644 index 0000000000000..ee9358dbaf015 --- /dev/null +++ b/server/functions/pointseries/lib/is_column_reference.js @@ -0,0 +1,7 @@ +import { parse } from 'tinymath'; + +export function isColumnReference(mathExpression) { + if (mathExpression == null) mathExpression = 'null'; + const parsedMath = parse(mathExpression); + return typeof parsedMath === 'string'; +}