Fix the MapperFieldType.rangeQuery API. (elastic#26552)

RangeQueryBuilder needs to perform too many `instanceof` checks in order to
check for `date` or `range` fields in order to know what it should do with the
shape relation, time zone and date format.

This commit adds those 3 parameters to the `rangeQuery` factory method so that
those instanceof checks are not necessary anymore.

core/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java

@@ -33,6 +33,7 @@
 import org.elasticsearch.Version;
 import org.elasticsearch.common.Explicit;
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.geo.ShapeRelation;
 import org.elasticsearch.common.joda.DateMathParser;
 import org.elasticsearch.common.joda.FormatDateTimeFormatter;
 import org.elasticsearch.common.joda.Joda;
@@ -246,28 +247,21 @@ long parse(String value) {
 
         @Override
         public Query termQuery(Object value, @Nullable QueryShardContext context) {
-            Query query = innerRangeQuery(value, value, true, true, null, null, context);
+            Query query = rangeQuery(value, value, true, true, ShapeRelation.INTERSECTS, null, null, context);
             if (boost() != 1f) {
                 query = new BoostQuery(query, boost());
             }
             return query;
         }
 
         @Override
-        public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, QueryShardContext context) {
-            failIfNotIndexed();
-            return rangeQuery(lowerTerm, upperTerm, includeLower, includeUpper, null, null, context);
-        }
-
-        public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper,
-                @Nullable DateTimeZone timeZone, @Nullable DateMathParser forcedDateParser, QueryShardContext context) {
-            failIfNotIndexed();
-            return innerRangeQuery(lowerTerm, upperTerm, includeLower, includeUpper, timeZone, forcedDateParser, context);
-        }
-
-        Query innerRangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper,
+        public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, ShapeRelation relation,
                 @Nullable DateTimeZone timeZone, @Nullable DateMathParser forcedDateParser, QueryShardContext context) {
             failIfNotIndexed();
+            if (relation == ShapeRelation.DISJOINT) {
+                throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() +
+                        "] does not support DISJOINT ranges");
+            }
             DateMathParser parser = forcedDateParser == null
                     ? dateMathParser
                     : forcedDateParser;

core/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java

@@ -120,7 +120,7 @@ public Mapper.Builder<?,?> parse(String name, Map<String, Object> node, ParserCo
         }
     }
 
-    public static final class IpFieldType extends MappedFieldType {
+    public static final class IpFieldType extends SimpleMappedFieldType {
 
         public IpFieldType() {
             super();

core/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -35,6 +35,7 @@
 import org.apache.lucene.search.TermQuery;
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.common.geo.ShapeRelation;
 import org.elasticsearch.common.joda.DateMathParser;
 import org.elasticsearch.common.unit.Fuzziness;
 import org.elasticsearch.index.analysis.NamedAnalyzer;
@@ -347,7 +348,15 @@ public Query termsQuery(List<?> values, @Nullable QueryShardContext context) {
         return new ConstantScoreQuery(builder.build());
     }
 
-    public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper, QueryShardContext context) {
+    /**
+     * Factory method for range queries.
+     * @param relation the relation, nulls should be interpreted like INTERSECTS
+     */
+    public Query rangeQuery(
+            Object lowerTerm, Object upperTerm,
+            boolean includeLower, boolean includeUpper,
+            ShapeRelation relation, DateTimeZone timeZone, DateMathParser parser,
+            QueryShardContext context) {
         throw new IllegalArgumentException("Field [" + name + "] of type [" + typeName() + "] does not support range queries");
     }
 

core/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

@@ -44,7 +44,6 @@
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentParser.Token;
-import org.elasticsearch.common.xcontent.support.AbstractXContentParser;
 import org.elasticsearch.index.fielddata.IndexFieldData;
 import org.elasticsearch.index.fielddata.IndexNumericFieldData.NumericType;
 import org.elasticsearch.index.fielddata.plain.DocValuesIndexFieldData;
@@ -836,7 +835,7 @@ private static double objectToDouble(Object value) {
         }
     }
 
-    public static final class NumberFieldType extends MappedFieldType {
+    public static final class NumberFieldType extends SimpleMappedFieldType {
 
         NumberType type;
 

core/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java

@@ -162,7 +162,7 @@ public Mapper.Builder<?,?> parse(String name, Map<String, Object> node,
         }
     }
 
-    public static final class ScaledFloatFieldType extends MappedFieldType {
+    public static final class ScaledFloatFieldType extends SimpleMappedFieldType {
 
         private double scalingFactor;
 

core/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java

@@ -126,7 +126,7 @@ public MetadataFieldMapper getDefault(MappedFieldType fieldType, ParserContext c
         }
     }
 
-    static final class SeqNoFieldType extends MappedFieldType {
+    static final class SeqNoFieldType extends SimpleMappedFieldType {
 
         SeqNoFieldType() {
         }

core/src/main/java/org/elasticsearch/index/mapper/SimpleMappedFieldType.java

@@ -0,0 +1,63 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.index.mapper;
+
+import org.apache.lucene.search.Query;
+import org.elasticsearch.common.geo.ShapeRelation;
+import org.elasticsearch.common.joda.DateMathParser;
+import org.elasticsearch.index.query.QueryShardContext;
+import org.joda.time.DateTimeZone;
+
+/**
+ * {@link MappedFieldType} base impl for field types that are neither dates nor ranges.
+ */
+public abstract class SimpleMappedFieldType extends MappedFieldType {
+
+    protected SimpleMappedFieldType() {
+        super();
+    }
+
+    protected SimpleMappedFieldType(MappedFieldType ref) {
+        super(ref);
+    }
+
+    @Override
+    public final Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper,
+            ShapeRelation relation, DateTimeZone timeZone, DateMathParser parser, QueryShardContext context) {
+        if (relation == ShapeRelation.DISJOINT) {
+            throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() +
+                    "] does not support DISJOINT ranges");
+        }
+        // We do not fail on non-null time zones and date parsers
+        // The reasoning is that on query parsers, you might want to set a time zone or format for date fields
+        // but then the API has no way to know which fields are dates and which fields are not dates
+        return rangeQuery(lowerTerm, upperTerm, includeLower, includeUpper, context);
+    }
+
+    /**
+     * Same as {@link #rangeQuery(Object, Object, boolean, boolean, ShapeRelation, DateTimeZone, DateMathParser, QueryShardContext)}
+     * but without the trouble of relations or date-specific options.
+     */
+    protected Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower, boolean includeUpper,
+            QueryShardContext context) {
+        throw new IllegalArgumentException("Field [" + name() + "] of type [" + typeName() + "] does not support range queries");
+    }
+
+}

core/src/main/java/org/elasticsearch/index/mapper/TermBasedFieldType.java

@@ -32,7 +32,7 @@
 
 /** Base {@link MappedFieldType} implementation for a field that is indexed
  *  with the inverted index. */
-abstract class TermBasedFieldType extends MappedFieldType {
+abstract class TermBasedFieldType extends SimpleMappedFieldType {
 
     TermBasedFieldType() {}
 

core/src/main/java/org/elasticsearch/index/query/RangeQueryBuilder.java

@@ -36,7 +36,6 @@
 import org.elasticsearch.common.lucene.BytesRefs;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.index.mapper.DateFieldMapper;
 import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.MapperService;
@@ -453,7 +452,7 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC
             // no field means we have no values
             return MappedFieldType.Relation.DISJOINT;
         } else {
-            DateMathParser dateMathParser = format == null ? null : new DateMathParser(format);
+            DateMathParser dateMathParser = getForceDateParser();
             return fieldType.isFieldWithinQuery(shardContext.getIndexReader(), from, to, includeLower,
                     includeUpper, timeZone, dateMathParser, queryRewriteContext);
         }
@@ -503,25 +502,10 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
         Query query = null;
         MappedFieldType mapper = context.fieldMapper(this.fieldName);
         if (mapper != null) {
-            if (mapper instanceof DateFieldMapper.DateFieldType) {
-
-                query = ((DateFieldMapper.DateFieldType) mapper).rangeQuery(from, to, includeLower, includeUpper,
-                        timeZone, getForceDateParser(), context);
-            } else if (mapper instanceof RangeFieldMapper.RangeFieldType) {
-                DateMathParser forcedDateParser = null;
-                if (mapper.typeName() == RangeFieldMapper.RangeType.DATE.name && this.format != null) {
-                    forcedDateParser = new DateMathParser(this.format);
-                }
-                query = ((RangeFieldMapper.RangeFieldType) mapper).rangeQuery(from, to, includeLower, includeUpper,
+            DateMathParser forcedDateParser = getForceDateParser();
+            query = mapper.rangeQuery(
+                    from, to, includeLower, includeUpper,
                     relation, timeZone, forcedDateParser, context);
-            } else {
-                if (timeZone != null) {
-                    throw new QueryShardException(context, "[range] time_zone can not be applied to non date field ["
-                            + fieldName + "]");
-                }
-                //LUCENE 4 UPGRADE Mapper#rangeQuery should use bytesref as well?
-                query = mapper.rangeQuery(from, to, includeLower, includeUpper, context);
-            }
         } else {
             if (timeZone != null) {
                 throw new QueryShardException(context, "[range] time_zone can not be applied to non unmapped field ["
@@ -530,7 +514,9 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
         }
 
         if (query == null) {
-            query = new TermRangeQuery(this.fieldName, BytesRefs.toBytesRef(from), BytesRefs.toBytesRef(to), includeLower, includeUpper);
+            query = new TermRangeQuery(this.fieldName,
+                    BytesRefs.toBytesRef(from), BytesRefs.toBytesRef(to),
+                    includeLower, includeUpper);
         }
         return query;
     }

core/src/main/java/org/elasticsearch/index/search/QueryStringQueryParser.java

@@ -46,11 +46,10 @@
 import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.common.unit.Fuzziness;
-import org.elasticsearch.index.mapper.DateFieldMapper;
 import org.elasticsearch.index.mapper.FieldNamesFieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
-import org.elasticsearch.index.mapper.StringFieldType;
 import org.elasticsearch.index.mapper.MapperService;
+import org.elasticsearch.index.mapper.StringFieldType;
 import org.elasticsearch.index.query.ExistsQueryBuilder;
 import org.elasticsearch.index.query.MultiMatchQueryBuilder;
 import org.elasticsearch.index.query.QueryShardContext;
@@ -394,14 +393,8 @@ private Query getRangeQuerySingle(String field, String part1, String part2,
             Analyzer normalizer = forceAnalyzer == null ? queryBuilder.context.getSearchAnalyzer(currentFieldType) : forceAnalyzer;
             BytesRef part1Binary = part1 == null ? null : normalizer.normalize(field, part1);
             BytesRef part2Binary = part2 == null ? null : normalizer.normalize(field, part2);
-            Query rangeQuery;
-            if (currentFieldType instanceof DateFieldMapper.DateFieldType && timeZone != null) {
-                DateFieldMapper.DateFieldType dateFieldType = (DateFieldMapper.DateFieldType) this.currentFieldType;
-                rangeQuery = dateFieldType.rangeQuery(part1Binary, part2Binary,
-                    startInclusive, endInclusive, timeZone, null, context);
-            } else {
-                rangeQuery = currentFieldType.rangeQuery(part1Binary, part2Binary, startInclusive, endInclusive, context);
-            }
+            Query rangeQuery = currentFieldType.rangeQuery(part1Binary, part2Binary,
+                startInclusive, endInclusive, null, timeZone, null, context);
             return rangeQuery;
         } catch (RuntimeException e) {
             if (lenient) {

core/src/test/java/org/elasticsearch/index/mapper/DateFieldTypeTests.java

@@ -200,11 +200,11 @@ public void testRangeQuery() throws IOException {
                 LongPoint.newRangeQuery("field", instant1, instant2),
                 SortedNumericDocValuesField.newSlowRangeQuery("field", instant1, instant2));
         assertEquals(expected,
-                ft.rangeQuery(date1, date2, true, true, context).rewrite(new MultiReader()));
+                ft.rangeQuery(date1, date2, true, true, null, null, null, context).rewrite(new MultiReader()));
 
         ft.setIndexOptions(IndexOptions.NONE);
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> ft.rangeQuery(date1, date2, true, true, context));
+                () -> ft.rangeQuery(date1, date2, true, true, null, null, null, context));
         assertEquals("Cannot search on field [field] since it is not indexed.", e.getMessage());
     }
 }

core/src/test/java/org/elasticsearch/index/mapper/IdFieldTypeTests.java

@@ -42,7 +42,7 @@ public void testRangeQuery() {
         MappedFieldType ft = createDefaultFieldType();
         ft.setName("_id");
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> ft.rangeQuery(null, null, randomBoolean(), randomBoolean(), null));
+                () -> ft.rangeQuery(null, null, randomBoolean(), randomBoolean(), null, null, null, null));
         assertEquals("Field [_id] of type [_id] does not support range queries", e.getMessage());
     }