Skip to content

Remove simple_query_string hack now that multi_match supports * properly #13285

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

Merged
merged 1 commit into from
Aug 4, 2017
Merged

Remove simple_query_string hack now that multi_match supports * properly #13285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions src/ui/public/kuery/functions/__tests__/and.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { nodeTypes } from '../../node_types';
import * as ast from '../../ast';
import StubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import ngMock from 'ng_mock';
import { expectDeepEqual } from '../../../../../test_utils/expect_deep_equal';

let indexPattern;

Expand Down Expand Up @@ -54,11 +55,11 @@ describe('kuery functions', function () {

it('should wrap a literal argument with an "is" function targeting all fields', function () {
const literalFoo = nodeTypes.literal.buildNode('foo');
const expectedChild = ast.toElasticsearchQuery(nodeTypes.function.buildNode('is', '*', 'foo'), indexPattern);
const node = nodeTypes.function.buildNode('and', [literalFoo]);
const result = and.toElasticsearchQuery(node, indexPattern);
const resultChild = result.bool.filter[0];
expect(resultChild).to.have.property('simple_query_string');
expect(resultChild.simple_query_string.all_fields).to.be(true);
expectDeepEqual(resultChild, expectedChild);
});

});
Expand Down
34 changes: 11 additions & 23 deletions src/ui/public/kuery/functions/__tests__/is.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import expect from 'expect.js';
import * as is from '../is';
import { nodeTypes } from '../../node_types';
import _ from 'lodash';
import StubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import ngMock from 'ng_mock';
import { expectDeepEqual } from '../../../../../test_utils/expect_deep_equal';

let indexPattern;

Expand Down Expand Up @@ -54,34 +54,22 @@ describe('kuery functions', function () {

const node = nodeTypes.function.buildNode('is', '*', '*');
const result = is.toElasticsearchQuery(node, indexPattern);
expect(_.isEqual(expected, result)).to.be(true);
expectDeepEqual(result, expected);
});

it('should return an ES simple_query_string query in all fields mode when fieldName is "*"', function () {
it('should return an ES multi_match query when fieldName is "*"', function () {
const expected = {
simple_query_string: {
query: '"200"',
all_fields: true,
multi_match: {
query: 200,
fields: ['*'],
type: 'phrase',
lenient: true,
}
};

const node = nodeTypes.function.buildNode('is', '*', 200);
const result = is.toElasticsearchQuery(node, indexPattern);
expect(_.isEqual(expected, result)).to.be(true);
});

// See discussion about kuery escaping for background:
// https://github.com/elastic/kibana/pull/12624#issuecomment-312650307
it('should ensure the simple_query_string query is wrapped in double quotes to force a phrase search', function () {
const node = nodeTypes.function.buildNode('is', '*', '+response');
const result = is.toElasticsearchQuery(node, indexPattern);
expect(result.simple_query_string.query).to.be('"+response"');
});

it('already double quoted phrases should not get wrapped a second time', function () {
const node = nodeTypes.function.buildNode('is', '*', '"+response"');
const result = is.toElasticsearchQuery(node, indexPattern);
expect(result.simple_query_string.query).to.be('"+response"');
expectDeepEqual(result, expected);
});

it('should return an ES exists query when value is "*"', function () {
Expand All @@ -91,7 +79,7 @@ describe('kuery functions', function () {

const node = nodeTypes.function.buildNode('is', 'response', '*');
const result = is.toElasticsearchQuery(node, indexPattern);
expect(_.isEqual(expected, result)).to.be(true);
expectDeepEqual(result, expected);
});

it('should return an ES match_phrase query when a concrete fieldName and value are provided', function () {
Expand All @@ -103,7 +91,7 @@ describe('kuery functions', function () {

const node = nodeTypes.function.buildNode('is', 'response', 200);
const result = is.toElasticsearchQuery(node, indexPattern);
expect(_.isEqual(expected, result)).to.be(true);
expectDeepEqual(result, expected);
});

it('should support scripted fields', function () {
Expand Down
5 changes: 3 additions & 2 deletions src/ui/public/kuery/functions/__tests__/not.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { nodeTypes } from '../../node_types';
import * as ast from '../../ast';
import StubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import ngMock from 'ng_mock';
import { expectDeepEqual } from '../../../../../test_utils/expect_deep_equal';

let indexPattern;

Expand Down Expand Up @@ -49,11 +50,11 @@ describe('kuery functions', function () {

it('should wrap a literal argument with an "is" function targeting all fields', function () {
const literalFoo = nodeTypes.literal.buildNode('foo');
const expectedChild = ast.toElasticsearchQuery(nodeTypes.function.buildNode('is', '*', 'foo'), indexPattern);
const node = nodeTypes.function.buildNode('not', literalFoo);
const result = not.toElasticsearchQuery(node, indexPattern);
const resultChild = result.bool.must_not;
expect(resultChild).to.have.property('simple_query_string');
expect(resultChild.simple_query_string.all_fields).to.be(true);
expectDeepEqual(resultChild, expectedChild);
});

});
Expand Down
5 changes: 3 additions & 2 deletions src/ui/public/kuery/functions/__tests__/or.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { nodeTypes } from '../../node_types';
import * as ast from '../../ast';
import StubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
import ngMock from 'ng_mock';
import { expectDeepEqual } from '../../../../../test_utils/expect_deep_equal';

let indexPattern;

Expand Down Expand Up @@ -54,11 +55,11 @@ describe('kuery functions', function () {

it('should wrap a literal argument with an "is" function targeting all fields', function () {
const literalFoo = nodeTypes.literal.buildNode('foo');
const expectedChild = ast.toElasticsearchQuery(nodeTypes.function.buildNode('is', '*', 'foo'), indexPattern);
const node = nodeTypes.function.buildNode('or', [literalFoo]);
const result = or.toElasticsearchQuery(node, indexPattern);
const resultChild = result.bool.should[0];
expect(resultChild).to.have.property('simple_query_string');
expect(resultChild.simple_query_string.all_fields).to.be(true);
expectDeepEqual(resultChild, expectedChild);
});

it('should require one of the clauses to match', function () {
Expand Down
16 changes: 5 additions & 11 deletions src/ui/public/kuery/functions/is.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,12 @@ export function toElasticsearchQuery(node, indexPattern) {
return { match_all: {} };
}
else if (fieldName === '*' && value !== '*') {
const userQuery = String(value);
const query = isDoubleQuoted(userQuery) ? userQuery : `"${userQuery}"`;

return {
simple_query_string: {
query,
all_fields: true
multi_match: {
query: value,
fields: ['*'],
Copy link
Contributor

@lukasolson lukasolson Aug 3, 2017

Choose a reason for hiding this comment

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

If we rewrite this as fields: [fieldName], then couldn't we remove this piece of code? Or is there some benefit to using a match_phrase over a multi_match?

Copy link
Contributor Author

@Bargs Bargs Aug 4, 2017

Choose a reason for hiding this comment

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

There are 2 potential reasons I can think of to hold off on that for now:

  • lenient isn't necessary, and is perhaps undesirable when querying against a single field
  • Passing the field name straight through to multi_match would allow people to use leading wildcards and boosting which I don't think we should support just yet

type: 'phrase',
lenient: true,
}
};
}
Expand Down Expand Up @@ -68,8 +67,3 @@ export function toKueryExpression(node) {

return `${fieldName}:${value}`;
}

function isDoubleQuoted(str) {
return str.startsWith('"') && str.endsWith('"');
}