Skip to content

Commit

Permalink
[data] change KQL node builder to not generate recursive and/or claus…
Browse files Browse the repository at this point in the history 
…es (elastic#89345) (elastic#89890)

resolves elastic#88367

Prior to this PR, the KQL node_builder code was using recursion to generate
"and" & "or" expressions.  Eg, `and(foo1=bar1, foo2=bar2, foo3=bar3)` would
be generated as if was specified as `and(foo1=bar1, and(foo2=bar2, foo3=bar3))`.

Calls to the builder with long lists of expressions would generate nested JSON
as deep as the lists are long.  This is problematic, as Elasticsearch is
changing the default limit on nested bools to 20 levels, and alerting already
generates nested bools greater than that limit.
See: elastic/elasticsearch#55303

This PR changes the generated shape of above, so that all the nodes are at the
same level, instead of the previous "recursive" treatment.
  • Loading branch information
@pmuellr
pmuellr authored Feb 1, 2021
1 parent f845e19 commit 7c9028a
Show file tree
Hide file tree
Showing 8 changed files with 1,086 additions and 35 deletions.
2 changes: 1 addition & 1 deletion packages/kbn-es/src/cluster.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ exports.Cluster = class Cluster {
this._log.info(chalk.bold('Starting'));
this._log.indent(4);

const esArgs = ['indices.query.bool.max_nested_depth=100'].concat(options.esArgs || []);
const esArgs = options.esArgs || [];

// Add to esArgs if ssl is enabled
if (this._ssl) {
Expand Down
8 changes: 2 additions & 6 deletions packages/kbn-es/src/integration_tests/cluster.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,7 @@ describe('#start(installPath)', () => {
expect(extractConfigFiles.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
Array [
"indices.query.bool.max_nested_depth=100",
],
Array [],
undefined,
Object {
"log": <ToolingLog>,
Expand Down Expand Up @@ -342,9 +340,7 @@ describe('#run()', () => {
expect(extractConfigFiles.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
Array [
"indices.query.bool.max_nested_depth=100",
],
Array [],
undefined,
Object {
"log": <ToolingLog>,
Expand Down
280 changes: 280 additions & 0 deletions src/plugins/data/common/es_query/kuery/node_types/node_builder.test.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,280 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* and the Server Side Public License, v 1; you may not use this file except in
* compliance with, at your election, the Elastic License or the Server Side
* Public License, v 1.
*/

import { nodeBuilder } from './node_builder';
import { toElasticsearchQuery } from '../index';

describe('nodeBuilder', () => {
describe('is method', () => {
test('string value', () => {
const nodes = nodeBuilder.is('foo', 'bar');
const query = toElasticsearchQuery(nodes);
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo": "bar",
},
},
],
},
}
`);
});

test('KueryNode value', () => {
const literalValue = {
type: 'literal' as 'literal',
value: 'bar',
};
const nodes = nodeBuilder.is('foo', literalValue);
const query = toElasticsearchQuery(nodes);
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo": "bar",
},
},
],
},
}
`);
});
});

describe('and method', () => {
test('single clause', () => {
const nodes = [nodeBuilder.is('foo', 'bar')];
const query = toElasticsearchQuery(nodeBuilder.and(nodes));
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo": "bar",
},
},
],
},
}
`);
});

test('two clauses', () => {
const nodes = [nodeBuilder.is('foo1', 'bar1'), nodeBuilder.is('foo2', 'bar2')];
const query = toElasticsearchQuery(nodeBuilder.and(nodes));
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"filter": Array [
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo1": "bar1",
},
},
],
},
},
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo2": "bar2",
},
},
],
},
},
],
},
}
`);
});

test('three clauses', () => {
const nodes = [
nodeBuilder.is('foo1', 'bar1'),
nodeBuilder.is('foo2', 'bar2'),
nodeBuilder.is('foo3', 'bar3'),
];
const query = toElasticsearchQuery(nodeBuilder.and(nodes));
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"filter": Array [
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo1": "bar1",
},
},
],
},
},
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo2": "bar2",
},
},
],
},
},
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo3": "bar3",
},
},
],
},
},
],
},
}
`);
});
});

describe('or method', () => {
test('single clause', () => {
const nodes = [nodeBuilder.is('foo', 'bar')];
const query = toElasticsearchQuery(nodeBuilder.or(nodes));
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo": "bar",
},
},
],
},
}
`);
});

test('two clauses', () => {
const nodes = [nodeBuilder.is('foo1', 'bar1'), nodeBuilder.is('foo2', 'bar2')];
const query = toElasticsearchQuery(nodeBuilder.or(nodes));
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo1": "bar1",
},
},
],
},
},
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo2": "bar2",
},
},
],
},
},
],
},
}
`);
});

test('three clauses', () => {
const nodes = [
nodeBuilder.is('foo1', 'bar1'),
nodeBuilder.is('foo2', 'bar2'),
nodeBuilder.is('foo3', 'bar3'),
];
const query = toElasticsearchQuery(nodeBuilder.or(nodes));
expect(query).toMatchInlineSnapshot(`
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo1": "bar1",
},
},
],
},
},
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo2": "bar2",
},
},
],
},
},
Object {
"bool": Object {
"minimum_should_match": 1,
"should": Array [
Object {
"match": Object {
"foo3": "bar3",
},
},
],
},
},
],
},
}
`);
});
});
});
10 changes: 4 additions & 6 deletions src/plugins/data/common/es_query/kuery/node_types/node_builder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@ export const nodeBuilder = {
nodeTypes.literal.buildNode(false),
]);
},
or: ([first, ...args]: KueryNode[]): KueryNode => {
return args.length ? nodeTypes.function.buildNode('or', [first, nodeBuilder.or(args)]) : first;
or: (nodes: KueryNode[]): KueryNode => {
return nodes.length > 1 ? nodeTypes.function.buildNode('or', nodes) : nodes[0];
},
and: ([first, ...args]: KueryNode[]): KueryNode => {
return args.length
? nodeTypes.function.buildNode('and', [first, nodeBuilder.and(args)])
: first;
and: (nodes: KueryNode[]): KueryNode => {
return nodes.length > 1 ? nodeTypes.function.buildNode('and', nodes) : nodes[0];
},
};
Loading

0 comments on commit 7c9028a

Please sign in to comment.