[Expressions] Use table column ID instead of name when set (#99724) (#…

…101110) * [Expressions] Use table column ID instead of name when set * Update ID matching to match by name sometimes * Add an extra case to prevent insertion of duplicate column * Simplify logic and add test for output ID * Respond to review comments Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
elastic · Jun 1, 2021 · 1a2e46b · 1a2e46b
1 parent d252c4e
commit 1a2e46b
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 31 deletions.
diff --git a/src/plugins/expressions/common/expression_functions/specs/map_column.ts b/src/plugins/expressions/common/expression_functions/specs/map_column.ts
@@ -44,7 +44,7 @@ export const mapColumn: ExpressionFunctionDefinition<
       types: ['string', 'null'],
       help: i18n.translate('expressions.functions.mapColumn.args.idHelpText', {
         defaultMessage:
-          'An optional id of the resulting column. When `null` the name/column argument is used as id.',
+          'An optional id of the resulting column. When no id is provided, the id will be looked up from the existing column by the provided name argument. If no column with this name exists yet, a new column with this name and an identical id will be added to the table.',
       }),
       required: false,
       default: null,
@@ -53,7 +53,7 @@ export const mapColumn: ExpressionFunctionDefinition<
       types: ['string'],
       aliases: ['_', 'column'],
       help: i18n.translate('expressions.functions.mapColumn.args.nameHelpText', {
-        defaultMessage: 'The name of the resulting column.',
+        defaultMessage: 'The name of the resulting column. Names are not required to be unique.',
       }),
       required: true,
     },
@@ -86,9 +86,17 @@ export const mapColumn: ExpressionFunctionDefinition<
         .expression?.(...params)
         .pipe(take(1))
         .toPromise() ?? Promise.resolve(null);
-    const columnId = args.id != null ? args.id : args.name;
 
     const columns = [...input.columns];
+    const existingColumnIndex = columns.findIndex(({ id, name }) => {
+      if (args.id) {
+        return id === args.id;
+      }
+      return name === args.name;
+    });
+    const columnId =
+      existingColumnIndex === -1 ? args.id ?? args.name : columns[existingColumnIndex].id;
+
     const rowPromises = input.rows.map((row) => {
       return expression({
         type: 'datatable',
@@ -101,7 +109,6 @@ export const mapColumn: ExpressionFunctionDefinition<
     });
 
     return Promise.all(rowPromises).then((rows) => {
-      const existingColumnIndex = columns.findIndex(({ name }) => name === args.name);
       const type = rows.length ? getType(rows[0][columnId]) : 'null';
       const newColumn = {
         id: columnId,

diff --git a/src/plugins/expressions/common/expression_functions/specs/math.ts b/src/plugins/expressions/common/expression_functions/specs/math.ts
@@ -130,10 +130,11 @@ export const math: ExpressionFunctionDefinition<
       throw errors.emptyExpression();
     }
 
+    // Use unique ID if available, otherwise fall back to names
     const mathContext = isDatatable(input)
       ? pivotObjectArray(
           input.rows,
-          input.columns.map((col) => col.name)
+          input.columns.map((col) => col.id)
         )
       : { value: input };
 

diff --git a/src/plugins/expressions/common/expression_functions/specs/tests/map_column.test.ts b/src/plugins/expressions/common/expression_functions/specs/tests/map_column.test.ts
@@ -35,16 +35,35 @@ describe('mapColumn', () => {
     expect(result.rows[arbitraryRowIndex]).toHaveProperty('pricePlusTwo');
   });
 
-  it('overwrites existing column with the new column if an existing column name is provided', async () => {
-    const result = await runFn(testTable, { name: 'name', expression: pricePlusTwo });
-    const nameColumnIndex = result.columns.findIndex(({ name }) => name === 'name');
+  it('allows the id arg to be optional, looking up by name instead', async () => {
+    const result = await runFn(testTable, { name: 'name label', expression: pricePlusTwo });
+    const nameColumnIndex = result.columns.findIndex(({ name }) => name === 'name label');
     const arbitraryRowIndex = 4;
 
     expect(result.type).toBe('datatable');
     expect(result.columns).toHaveLength(testTable.columns.length);
-    expect(result.columns[nameColumnIndex]).toHaveProperty('name', 'name');
+    expect(result.columns[nameColumnIndex]).toHaveProperty('id', 'name');
+    expect(result.columns[nameColumnIndex]).toHaveProperty('name', 'name label');
     expect(result.columns[nameColumnIndex].meta).toHaveProperty('type', 'number');
     expect(result.rows[arbitraryRowIndex]).toHaveProperty('name', 202);
+    expect(result.rows[arbitraryRowIndex]).not.toHaveProperty('name label');
+  });
+
+  it('allows a duplicate name when the ids are different', async () => {
+    const result = await runFn(testTable, {
+      id: 'new',
+      name: 'name label',
+      expression: pricePlusTwo,
+    });
+    const nameColumnIndex = result.columns.findIndex(({ id }) => id === 'new');
+    const arbitraryRowIndex = 4;
+
+    expect(result.type).toBe('datatable');
+    expect(result.columns).toHaveLength(testTable.columns.length + 1);
+    expect(result.columns[nameColumnIndex]).toHaveProperty('id', 'new');
+    expect(result.columns[nameColumnIndex]).toHaveProperty('name', 'name label');
+    expect(result.columns[nameColumnIndex].meta).toHaveProperty('type', 'number');
+    expect(result.rows[arbitraryRowIndex]).toHaveProperty('new', 202);
   });
 
   it('adds a column to empty tables', async () => {
@@ -66,18 +85,6 @@ describe('mapColumn', () => {
     expect(result.columns[0].meta).toHaveProperty('type', 'null');
   });
 
-  it('should assign specific id, different from name, when id arg is passed for copied column', async () => {
-    const result = await runFn(testTable, { name: 'name', id: 'myid', expression: pricePlusTwo });
-    const nameColumnIndex = result.columns.findIndex(({ name }) => name === 'name');
-
-    expect(result.type).toBe('datatable');
-    expect(result.columns[nameColumnIndex]).toEqual({
-      id: 'myid',
-      name: 'name',
-      meta: { type: 'number' },
-    });
-  });
-
   it('should copy over the meta information from the specified column', async () => {
     const result = await runFn(
       {

diff --git a/src/plugins/expressions/common/expression_functions/specs/tests/math.test.ts b/src/plugins/expressions/common/expression_functions/specs/tests/math.test.ts
@@ -27,7 +27,7 @@ describe('math', () => {
     expect(fn(-103, { expression: 'abs(value)' })).toBe(103);
   });
 
-  it('evaluates math expressions with references to columns in a datatable', () => {
+  it('evaluates math expressions with references to columns by id in a datatable', () => {
     expect(fn(testTable, { expression: 'unique(in_stock)' })).toBe(2);
     expect(fn(testTable, { expression: 'sum(quantity)' })).toBe(2508);
     expect(fn(testTable, { expression: 'mean(price)' })).toBe(320);
@@ -36,6 +36,21 @@ describe('math', () => {
     expect(fn(testTable, { expression: 'max(price)' })).toBe(605);
   });
 
+  it('does not use the name for math', () => {
+    expect(() => fn(testTable, { expression: 'unique("in_stock label")' })).toThrow(
+      'Unknown variable'
+    );
+    expect(() => fn(testTable, { expression: 'sum("quantity label")' })).toThrow(
+      'Unknown variable'
+    );
+    expect(() => fn(testTable, { expression: 'mean("price label")' })).toThrow('Unknown variable');
+    expect(() => fn(testTable, { expression: 'min("price label")' })).toThrow('Unknown variable');
+    expect(() => fn(testTable, { expression: 'median("quantity label")' })).toThrow(
+      'Unknown variable'
+    );
+    expect(() => fn(testTable, { expression: 'max("price label")' })).toThrow('Unknown variable');
+  });
+
   describe('args', () => {
     describe('expression', () => {
       it('sets the math expression to be evaluted', () => {

diff --git a/src/plugins/expressions/common/expression_functions/specs/tests/utils.ts b/src/plugins/expressions/common/expression_functions/specs/tests/utils.ts
@@ -37,27 +37,27 @@ const testTable: Datatable = {
   columns: [
     {
       id: 'name',
-      name: 'name',
+      name: 'name label',
       meta: { type: 'string' },
     },
     {
       id: 'time',
-      name: 'time',
+      name: 'time label',
       meta: { type: 'date' },
     },
     {
       id: 'price',
-      name: 'price',
+      name: 'price label',
       meta: { type: 'number' },
     },
     {
       id: 'quantity',
-      name: 'quantity',
+      name: 'quantity label',
       meta: { type: 'number' },
     },
     {
       id: 'in_stock',
-      name: 'in_stock',
+      name: 'in_stock label',
       meta: { type: 'boolean' },
     },
   ],

diff --git a/x-pack/plugins/translations/translations/ja-JP.json b/x-pack/plugins/translations/translations/ja-JP.json
@@ -1898,8 +1898,6 @@
     "expressions.functions.fontHelpText": "フォントスタイルを作成します。",
     "expressions.functions.mapColumn.args.copyMetaFromHelpText": "設定されている場合、指定した列IDのメタオブジェクトが指定したターゲット列にコピーされます。列が存在しない場合は失敗し、エラーは表示されません。",
     "expressions.functions.mapColumn.args.expressionHelpText": "すべての行で実行される式。単一行の{DATATABLE}と一緒に指定され、セル値を返します。",
-    "expressions.functions.mapColumn.args.idHelpText": "結果列の任意のID。「null」の場合、name/column引数がIDとして使用されます。",
-    "expressions.functions.mapColumn.args.nameHelpText": "結果の列の名前です。",
     "expressions.functions.mapColumnHelpText": "他の列の結果として計算された列を追加します。引数が指定された場合のみ変更が加えられます。{alterColumnFn}と{staticColumnFn}もご参照ください。",
     "expressions.functions.math.args.expressionHelpText": "評価された {TINYMATH} 表現です。{TINYMATH_URL} をご覧ください。",
     "expressions.functions.math.args.onErrorHelpText": "{TINYMATH}評価が失敗するか、NaNが返される場合、戻り値はonErrorで指定されます。「'throw'」の場合、例外が発生し、式の実行が終了します (デフォルト) 。",

diff --git a/x-pack/plugins/translations/translations/zh-CN.json b/x-pack/plugins/translations/translations/zh-CN.json
@@ -1910,8 +1910,6 @@
     "expressions.functions.fontHelpText": "创建字体样式。",
     "expressions.functions.mapColumn.args.copyMetaFromHelpText": "如果设置，指定列 ID 的元对象将复制到指定目标列。如果列不存在，复制将无提示失败。",
     "expressions.functions.mapColumn.args.expressionHelpText": "在每行上执行的表达式，为其提供了单行 {DATATABLE} 上下文，其将返回单元格值。",
-    "expressions.functions.mapColumn.args.idHelpText": "结果列的可选 ID。如果为 `null`，名称/列参数将用作 ID。",
-    "expressions.functions.mapColumn.args.nameHelpText": "结果列的名称。",
     "expressions.functions.mapColumnHelpText": "添加计算为其他列的结果的列。只有提供参数时，才会执行更改。另请参见 {alterColumnFn} 和 {staticColumnFn}。",
     "expressions.functions.math.args.expressionHelpText": "已计算的 {TINYMATH} 表达式。请参阅 {TINYMATH_URL}。",
     "expressions.functions.math.args.onErrorHelpText": "如果 {TINYMATH} 评估失败或返回 NaN，返回值将由 onError 指定。为 `'throw'` 时，其将引发异常，从而终止表达式执行 (默认) 。",