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

[data.search] Clean up arguments to esaggs. #84973

Merged
merged 17 commits into from
Dec 10, 2020
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Export function type mapping & adjust logic in toExpresionAst.
  • Loading branch information
lukeelmers committed Dec 9, 2020
commit 09a92960a29cc93694ee530c58387a08b718bea8
126 changes: 84 additions & 42 deletions src/plugins/data/common/search/aggs/agg_config.test.ts
Original file line number Diff line number Diff line change
@@ -518,40 +518,45 @@ describe('AggConfig', () => {
const aggConfig = ac.createAggConfig(configStates);
expect(aggConfig.toExpressionAst()).toMatchInlineSnapshot(`
Object {
"arguments": Object {
"enabled": Array [
true,
],
"field": Array [
"machine.os.keyword",
],
"id": Array [
"1",
],
"missingBucket": Array [
false,
],
"missingBucketLabel": Array [
"Missing",
],
"order": Array [
"asc",
],
"otherBucket": Array [
false,
],
"otherBucketLabel": Array [
"Other",
],
"schema": Array [
"segment",
],
"size": Array [
5,
],
},
"function": "aggTerms",
"type": "function",
"chain": Array [
Object {
"arguments": Object {
"enabled": Array [
true,
],
"field": Array [
"machine.os.keyword",
],
"id": Array [
"1",
],
"missingBucket": Array [
false,
],
"missingBucketLabel": Array [
"Missing",
],
"order": Array [
"asc",
],
"otherBucket": Array [
false,
],
"otherBucketLabel": Array [
"Other",
],
"schema": Array [
"segment",
],
"size": Array [
5,
],
},
"function": "aggTerms",
"type": "function",
},
],
"type": "expression",
}
`);
});
@@ -575,7 +580,7 @@ describe('AggConfig', () => {
},
};
const aggConfig = ac.createAggConfig(configStates);
const aggArg = aggConfig.toExpressionAst()?.arguments.orderAgg;
const aggArg = aggConfig.toExpressionAst()?.chain[0].arguments.orderAgg;
expect(aggArg).toMatchInlineSnapshot(`
Array [
Object {
@@ -629,11 +634,16 @@ describe('AggConfig', () => {
range.expressionName = 'aggRange';
const rangesParam = range.params.find((p) => p.name === 'ranges');
rangesParam!.toExpressionAst = (val: any) => ({
type: 'function',
function: 'aggRanges',
arguments: {
ranges: ['oh hi there!'],
},
type: 'expression',
chain: [
{
type: 'function',
function: 'aggRanges',
arguments: {
ranges: ['oh hi there!'],
},
},
],
});

const ac = new AggConfigs(indexPattern, [], { typesRegistry });
@@ -645,7 +655,7 @@ describe('AggConfig', () => {
};

const aggConfig = ac.createAggConfig(configStates);
const ranges = aggConfig.toExpressionAst()!.arguments.ranges;
const ranges = aggConfig.toExpressionAst()!.chain[0].arguments.ranges;
expect(ranges).toMatchInlineSnapshot(`
Array [
Object {
@@ -677,9 +687,41 @@ describe('AggConfig', () => {
},
};
const aggConfig = ac.createAggConfig(configStates);
const json = aggConfig.toExpressionAst()?.arguments.json;
const json = aggConfig.toExpressionAst()?.chain[0].arguments.json;
expect(json).toEqual([JSON.stringify(configStates.params.json)]);
});

it('stringifies arrays only if they are objects', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
type: 'range',
params: {
field: 'bytes',
ranges: [
{ from: 0, to: 1000 },
{ from: 1001, to: 2000 },
{ from: 2001, to: 3000 },
],
},
};
const aggConfig = ac.createAggConfig(configStates);
const ranges = aggConfig.toExpressionAst()?.chain[0].arguments.ranges;
expect(ranges).toEqual([JSON.stringify(configStates.params.ranges)]);
});

it('does not stringify arrays which are not objects', () => {
const ac = new AggConfigs(indexPattern, [], { typesRegistry });
const configStates = {
type: 'percentiles',
params: {
field: 'bytes',
percents: [1, 25, 50, 75, 99],
},
};
const aggConfig = ac.createAggConfig(configStates);
const percents = aggConfig.toExpressionAst()?.chain[0].arguments.percents;
expect(percents).toEqual([1, 25, 50, 75, 99]);
});
Comment on lines +712 to +724
Copy link
Member Author

Choose a reason for hiding this comment

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

Uncovered a bug in toExpressionAst where the percents arg was incorrectly stringified when we should have preserved it, so I updated the logic to handle this case where a param is already an array, but not an object array.

});

describe('#makeLabel', () => {
44 changes: 26 additions & 18 deletions src/plugins/data/common/search/aggs/agg_config.ts
Original file line number Diff line number Diff line change
@@ -23,7 +23,7 @@ import { Assign, Ensure } from '@kbn/utility-types';

import { ISearchOptions, ISearchSource } from 'src/plugins/data/public';
import {
ExpressionAstFunction,
ExpressionAstExpression,
ExpressionAstArgument,
SerializedFieldFormat,
} from 'src/plugins/expressions/common';
@@ -316,9 +316,9 @@ export class AggConfig {
}

/**
* @returns Returns an ExpressionAst representing the function for this agg type.
* @returns Returns an ExpressionAst representing the this agg type.
*/
toExpressionAst(): ExpressionAstFunction | undefined {
toExpressionAst(): ExpressionAstExpression | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

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

Discovered that returning an ExpressionAstFunction was not very useful since using this as a subexpression requires it to be ExpressionAstExpression, so I updated accordingly.

const functionName = this.type && this.type.expressionName;
const { type, ...rest } = this.serialize();
if (!functionName || !rest.params) {
@@ -334,13 +334,16 @@ export class AggConfig {
// If the param provides `toExpressionAst`, we call it with the value
const paramExpressionAst = deserializedParam.toExpressionAst(this.getParam(key));
if (paramExpressionAst) {
acc[key] = [
{
type: 'expression',
chain: [paramExpressionAst],
},
];
acc[key] = [paramExpressionAst];
}
} else if (value && Array.isArray(value)) {
// For array params which don't provide `toExpressionAst`, we stringify
// if it's an array of objects, otherwise we keep it as-is
const definedValues = value.filter(
(v) => typeof v !== 'undefined' && v !== null
) as ExpressionAstArgument[];
acc[key] =
typeof definedValues[0] === 'object' ? [JSON.stringify(definedValues)] : definedValues;
} else if (typeof value === 'object') {
// For object params which don't provide `toExpressionAst`, we stringify
acc[key] = [JSON.stringify(value)];
@@ -353,15 +356,20 @@ export class AggConfig {
}, {} as Record<string, ExpressionAstArgument[]>);

return {
type: 'function',
function: functionName,
arguments: {
...params,
// Expression args which are provided to all functions
id: [this.id],
enabled: [this.enabled],
...(this.schema ? { schema: [this.schema] } : {}), // schema may be undefined
},
type: 'expression',
chain: [
{
type: 'function',
function: functionName,
arguments: {
...params,
// Expression args which are provided to all functions
id: [this.id],
enabled: [this.enabled],
...(this.schema ? { schema: [this.schema] } : {}), // schema may be undefined
},
},
],
};
}

Loading