From 05c3d05cff9149c140a21da10cb096f39e0fb8b8 Mon Sep 17 00:00:00 2001 From: markharwood Date: Thu, 16 Apr 2015 16:02:53 +0100 Subject: [PATCH] Query enhancement: single value numeric queries shouldn't be handled by NumericRangeQuery and should use a TermQuery wrapped in a ConstantScoreQuery instead. Equally, single value filters should use TermFilters rather than NumericRangeFilters Closes #10646 --- .../index/mapper/core/ByteFieldMapper.java | 17 +----- .../index/mapper/core/DateFieldMapper.java | 14 ----- .../index/mapper/core/DoubleFieldMapper.java | 14 ----- .../index/mapper/core/FloatFieldMapper.java | 14 ----- .../index/mapper/core/IntegerFieldMapper.java | 14 ----- .../index/mapper/core/LongFieldMapper.java | 14 ----- .../index/mapper/core/NumberFieldMapper.java | 23 ++++---- .../index/mapper/core/ShortFieldMapper.java | 14 ----- .../query/SimpleIndexQueryParserTests.java | 55 ++++++++++--------- .../percolator/PercolatorTests.java | 8 +-- .../validate/SimpleValidateQueryTests.java | 6 +- 11 files changed, 48 insertions(+), 145 deletions(-) diff --git a/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java index d841b9a01ec..66d87a77aea 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/ByteFieldMapper.java @@ -200,13 +200,6 @@ public class ByteFieldMapper extends NumberFieldMapper { true, true); } - @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - int iValue = parseValue(value); - return NumericRangeQuery.newIntRange(names.indexName(), precisionStep, - iValue, iValue, true, true); - } - @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return NumericRangeQuery.newIntRange(names.indexName(), precisionStep, @@ -216,14 +209,8 @@ public class ByteFieldMapper extends NumberFieldMapper { } @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - int iValue = parseValueAsInt(value); - return Queries.wrap(NumericRangeQuery.newIntRange(names.indexName(), precisionStep, - iValue, iValue, true, true)); - } - - @Override - public Filter rangeFilter(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { + public Filter rangeFilter(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, + @Nullable QueryParseContext context) { return Queries.wrap(NumericRangeQuery.newIntRange(names.indexName(), precisionStep, lowerTerm == null ? null : parseValueAsInt(lowerTerm), upperTerm == null ? null : parseValueAsInt(upperTerm), diff --git a/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java index d5cc31606d7..8e5c88a9636 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/DateFieldMapper.java @@ -297,13 +297,6 @@ public class DateFieldMapper extends NumberFieldMapper { true, true); } - @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - long lValue = parseToMilliseconds(value); - return NumericRangeQuery.newLongRange(names.indexName(), precisionStep, - lValue, lValue, true, true); - } - public long parseToMilliseconds(Object value) { return parseToMilliseconds(value, false, null, dateMathParser); } @@ -323,13 +316,6 @@ public class DateFieldMapper extends NumberFieldMapper { return dateParser.parse(value, now(), inclusive, zone); } - @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - final long lValue = parseToMilliseconds(value); - return Queries.wrap(NumericRangeQuery.newLongRange(names.indexName(), precisionStep, - lValue, lValue, true, true)); - } - @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return rangeQuery(lowerTerm, upperTerm, includeLower, includeUpper, null, null, context); diff --git a/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java index 96c75f98153..6f6058439bf 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/DoubleFieldMapper.java @@ -191,13 +191,6 @@ public class DoubleFieldMapper extends NumberFieldMapper { true, true); } - @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - double dValue = parseDoubleValue(value); - return NumericRangeQuery.newDoubleRange(names.indexName(), precisionStep, - dValue, dValue, true, true); - } - @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return NumericRangeQuery.newDoubleRange(names.indexName(), precisionStep, @@ -206,13 +199,6 @@ public class DoubleFieldMapper extends NumberFieldMapper { includeLower, includeUpper); } - @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - double dValue = parseDoubleValue(value); - return Queries.wrap(NumericRangeQuery.newDoubleRange(names.indexName(), precisionStep, - dValue, dValue, true, true)); - } - @Override public Filter rangeFilter(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return Queries.wrap(NumericRangeQuery.newDoubleRange(names.indexName(), precisionStep, diff --git a/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java index 4bb38974103..ab1391e9698 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/FloatFieldMapper.java @@ -201,13 +201,6 @@ public class FloatFieldMapper extends NumberFieldMapper { true, true); } - @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - float fValue = parseValue(value); - return NumericRangeQuery.newFloatRange(names.indexName(), precisionStep, - fValue, fValue, true, true); - } - @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return NumericRangeQuery.newFloatRange(names.indexName(), precisionStep, @@ -216,13 +209,6 @@ public class FloatFieldMapper extends NumberFieldMapper { includeLower, includeUpper); } - @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - float fValue = parseValue(value); - return Queries.wrap(NumericRangeQuery.newFloatRange(names.indexName(), precisionStep, - fValue, fValue, true, true)); - } - @Override public Filter rangeFilter(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return Queries.wrap(NumericRangeQuery.newFloatRange(names.indexName(), precisionStep, diff --git a/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java index 4989d3e6856..eec2d84d0b9 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/IntegerFieldMapper.java @@ -195,20 +195,6 @@ public class IntegerFieldMapper extends NumberFieldMapper { true, true); } - @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - int iValue = parseValue(value); - return NumericRangeQuery.newIntRange(names.indexName(), precisionStep, - iValue, iValue, true, true); - } - - @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - int iValue = parseValue(value); - return Queries.wrap(NumericRangeQuery.newIntRange(names.indexName(), precisionStep, - iValue, iValue, true, true)); - } - @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return NumericRangeQuery.newIntRange(names.indexName(), precisionStep, diff --git a/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java index a11d89a000d..c10fdf79af6 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/LongFieldMapper.java @@ -185,20 +185,6 @@ public class LongFieldMapper extends NumberFieldMapper { true, true); } - @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - long iValue = parseLongValue(value); - return NumericRangeQuery.newLongRange(names.indexName(), precisionStep, - iValue, iValue, true, true); - } - - @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - long iValue = parseLongValue(value); - return Queries.wrap(NumericRangeQuery.newLongRange(names.indexName(), precisionStep, - iValue, iValue, true, true)); - } - @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return NumericRangeQuery.newLongRange(names.indexName(), precisionStep, diff --git a/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java index 2614e60f98e..8cccf0d6770 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/NumberFieldMapper.java @@ -33,8 +33,11 @@ import org.apache.lucene.index.DocValuesType; import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.IndexableField; import org.apache.lucene.index.IndexableFieldType; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.ConstantScoreQuery; import org.apache.lucene.search.Filter; import org.apache.lucene.search.Query; +import org.apache.lucene.search.TermQuery; import org.apache.lucene.store.ByteArrayDataOutput; import org.apache.lucene.util.BytesRef; import org.elasticsearch.ElasticsearchIllegalArgumentException; @@ -271,22 +274,18 @@ public abstract class NumberFieldMapper extends AbstractFieldM return true; } - /** - * Numeric field level query are basically range queries with same value and included. That's the recommended - * way to execute it. - */ @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - return rangeQuery(value, value, true, true, context); + public final Query termQuery(Object value, @Nullable QueryParseContext context) { + TermQuery scoringQuery = new TermQuery(new Term(names.indexName(), indexedValueForSearch(value))); + return new ConstantScoreQuery(scoringQuery); } - /** - * Numeric field level filter are basically range queries with same value and included. That's the recommended - * way to execute it. - */ @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - return rangeFilter(value, value, true, true, context); + public final Filter termFilter(Object value, @Nullable QueryParseContext context) { + // Made this method final because previously many subclasses duplicated + // the same code, returning a NumericRangeFilter, which should be less + // efficient than super's default impl of a single TermFilter. + return super.termFilter(value, context); } @Override diff --git a/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java index 59e9fd44869..b16518769d1 100644 --- a/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/core/ShortFieldMapper.java @@ -201,13 +201,6 @@ public class ShortFieldMapper extends NumberFieldMapper { true, true); } - @Override - public Query termQuery(Object value, @Nullable QueryParseContext context) { - int iValue = parseValueAsInt(value); - return NumericRangeQuery.newIntRange(names.indexName(), precisionStep, - iValue, iValue, true, true); - } - @Override public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return NumericRangeQuery.newIntRange(names.indexName(), precisionStep, @@ -216,13 +209,6 @@ public class ShortFieldMapper extends NumberFieldMapper { includeLower, includeUpper); } - @Override - public Filter termFilter(Object value, @Nullable QueryParseContext context) { - int iValue = parseValueAsInt(value); - return Queries.wrap(NumericRangeQuery.newIntRange(names.indexName(), precisionStep, - iValue, iValue, true, true)); - } - @Override public Filter rangeFilter(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, @Nullable QueryParseContext context) { return Queries.wrap(NumericRangeQuery.newIntRange(names.indexName(), precisionStep, diff --git a/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java b/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java index 6fc4a341d7b..6e928ca4f81 100644 --- a/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java +++ b/src/test/java/org/elasticsearch/index/query/SimpleIndexQueryParserTests.java @@ -464,25 +464,25 @@ public class SimpleIndexQueryParserTests extends ElasticsearchSingleNodeTest { public void testTermQueryBuilder() throws IOException { IndexQueryParserService queryParser = queryParser(); Query parsedQuery = queryParser.parse(termQuery("age", 34).buildAsBytes()).query(); - assertThat(parsedQuery, instanceOf(NumericRangeQuery.class)); - NumericRangeQuery fieldQuery = (NumericRangeQuery) parsedQuery; - assertThat(fieldQuery.getMin().intValue(), equalTo(34)); - assertThat(fieldQuery.getMax().intValue(), equalTo(34)); - assertThat(fieldQuery.includesMax(), equalTo(true)); - assertThat(fieldQuery.includesMin(), equalTo(true)); + TermQuery fieldQuery = unwrapTermQuery(parsedQuery, true); + assertThat(fieldQuery.getTerm().bytes(), equalTo(indexedValueForSearch(34l))); } @Test public void testTermQuery() throws IOException { IndexQueryParserService queryParser = queryParser(); String query = copyToStringFromClasspath("/org/elasticsearch/index/query/term.json"); - Query parsedQuery = queryParser.parse(query).query(); - assertThat(parsedQuery, instanceOf(NumericRangeQuery.class)); - NumericRangeQuery fieldQuery = (NumericRangeQuery) parsedQuery; - assertThat(fieldQuery.getMin().intValue(), equalTo(34)); - assertThat(fieldQuery.getMax().intValue(), equalTo(34)); - assertThat(fieldQuery.includesMax(), equalTo(true)); - assertThat(fieldQuery.includesMin(), equalTo(true)); + TermQuery fieldQuery = unwrapTermQuery(queryParser.parse(query).query(), true); + assertThat(fieldQuery.getTerm().bytes(), equalTo(indexedValueForSearch(34l))); + } + + private static TermQuery unwrapTermQuery(Query q, boolean expectConstantWrapper) { + if (expectConstantWrapper) { + assertThat(q, instanceOf(ConstantScoreQuery.class)); + q = ((ConstantScoreQuery) q).getQuery(); + } + assertThat(q, instanceOf(TermQuery.class)); + return (TermQuery) q; } @Test @@ -543,14 +543,19 @@ public class SimpleIndexQueryParserTests extends ElasticsearchSingleNodeTest { @Test public void testTermWithBoostQueryBuilder() throws IOException { IndexQueryParserService queryParser = queryParser(); + Query parsedQuery = queryParser.parse(termQuery("age", 34).boost(2.0f)).query(); - assertThat(parsedQuery, instanceOf(NumericRangeQuery.class)); - NumericRangeQuery fieldQuery = (NumericRangeQuery) parsedQuery; - assertThat(fieldQuery.getMin().intValue(), equalTo(34)); - assertThat(fieldQuery.getMax().intValue(), equalTo(34)); - assertThat(fieldQuery.includesMax(), equalTo(true)); - assertThat(fieldQuery.includesMin(), equalTo(true)); - assertThat((double) fieldQuery.getBoost(), closeTo(2.0, 0.01)); + TermQuery fieldQuery = unwrapTermQuery(parsedQuery, true); + assertThat(fieldQuery.getTerm().bytes(), equalTo(indexedValueForSearch(34l))); + assertThat((double) parsedQuery.getBoost(), closeTo(2.0, 0.01)); + } + + private BytesRef indexedValueForSearch(long value) { + BytesRefBuilder bytesRef = new BytesRefBuilder(); + NumericUtils.longToPrefixCoded(value, 0, bytesRef); // 0 because of + // exact + // match + return bytesRef.get(); } @Test @@ -558,13 +563,9 @@ public class SimpleIndexQueryParserTests extends ElasticsearchSingleNodeTest { IndexQueryParserService queryParser = queryParser(); String query = copyToStringFromClasspath("/org/elasticsearch/index/query/term-with-boost.json"); Query parsedQuery = queryParser.parse(query).query(); - assertThat(parsedQuery, instanceOf(NumericRangeQuery.class)); - NumericRangeQuery fieldQuery = (NumericRangeQuery) parsedQuery; - assertThat(fieldQuery.getMin().intValue(), equalTo(34)); - assertThat(fieldQuery.getMax().intValue(), equalTo(34)); - assertThat(fieldQuery.includesMax(), equalTo(true)); - assertThat(fieldQuery.includesMin(), equalTo(true)); - assertThat((double) fieldQuery.getBoost(), closeTo(2.0, 0.01)); + TermQuery fieldQuery = unwrapTermQuery(parsedQuery, true); + assertThat(fieldQuery.getTerm().bytes(), equalTo(indexedValueForSearch(34l))); + assertThat((double) parsedQuery.getBoost(), closeTo(2.0, 0.01)); } @Test diff --git a/src/test/java/org/elasticsearch/percolator/PercolatorTests.java b/src/test/java/org/elasticsearch/percolator/PercolatorTests.java index 01c0eab4dee..91f5afa7b0b 100644 --- a/src/test/java/org/elasticsearch/percolator/PercolatorTests.java +++ b/src/test/java/org/elasticsearch/percolator/PercolatorTests.java @@ -18,8 +18,6 @@ */ package org.elasticsearch.percolator; -import com.google.common.base.Predicate; - import org.elasticsearch.action.ShardOperationFailedException; import org.elasticsearch.action.admin.cluster.node.stats.NodeStats; import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsResponse; @@ -32,9 +30,9 @@ import org.elasticsearch.action.percolate.PercolateResponse; import org.elasticsearch.action.percolate.PercolateSourceBuilder; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.support.IndicesOptions; -import org.elasticsearch.client.Client; import org.elasticsearch.client.Requests; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.lucene.search.function.CombineFunction; import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.common.settings.ImmutableSettings.Builder; import org.elasticsearch.common.unit.TimeValue; @@ -1329,7 +1327,9 @@ public class PercolatorTests extends ElasticsearchIntegrationTest { .setSortByScore(true) .setSize(size) .setPercolateDoc(docBuilder().setDoc("field", "value")) - .setPercolateQuery(QueryBuilders.functionScoreQuery(matchQuery("field1", value), scriptFunction("doc['level'].value"))) + .setPercolateQuery( + QueryBuilders.functionScoreQuery(matchQuery("field1", value), scriptFunction("doc['level'].value")).boostMode( + CombineFunction.REPLACE)) .execute().actionGet(); assertMatchCount(response, levels.size()); diff --git a/src/test/java/org/elasticsearch/validate/SimpleValidateQueryTests.java b/src/test/java/org/elasticsearch/validate/SimpleValidateQueryTests.java index fa5f2f58973..b1cc5938c41 100644 --- a/src/test/java/org/elasticsearch/validate/SimpleValidateQueryTests.java +++ b/src/test/java/org/elasticsearch/validate/SimpleValidateQueryTests.java @@ -25,8 +25,6 @@ import org.elasticsearch.action.admin.indices.validate.query.ValidateQueryRespon import org.elasticsearch.client.Client; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.settings.ImmutableSettings; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.unit.Fuzziness; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.query.FilterBuilders; @@ -48,7 +46,9 @@ import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_NUMBER_OF import static org.elasticsearch.index.query.QueryBuilders.queryStringQuery; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures; -import static org.hamcrest.Matchers.*; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; /** *