From d07d9fc3b2ba52cd2e4a790fd58ef3c81d300f37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 8 Apr 2019 12:01:34 +0200 Subject: [PATCH 1/3] Fix range query edge cases Currently we throw an error when a range querys minimum value exceeds the maximum value due to the fact that they are neighbouring values and both upper and lower value are excluded from the interval. Since this is a condition that the user usually doesn't specify conciously (at least in the case of float and double values its difficult to see which values are adjacent) we should ignore those "wrong" intervals and create a MatchNoDocsQuery in those cases. We should still throw errors with an actionable message if the user specifies the query interval in a way that min value > max value. This PR adds those checks and tests for those cases. --- .../index/mapper/RangeFieldMapper.java | 160 +++++++++++++----- .../index/mapper/RangeFieldTypeTests.java | 122 +++++++++++++ 2 files changed, 237 insertions(+), 45 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index e5ba55de7bfd0..1c0c4d967a53d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -35,6 +35,7 @@ import org.apache.lucene.search.BoostQuery; import org.apache.lucene.search.DocValuesFieldExistsQuery; import org.apache.lucene.search.IndexOrDocValuesQuery; +import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; import org.apache.lucene.search.TermQuery; import org.apache.lucene.store.ByteArrayDataOutput; @@ -63,6 +64,7 @@ import java.time.ZoneId; import java.time.ZoneOffset; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -70,6 +72,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.BiFunction; import static org.elasticsearch.index.query.RangeQueryBuilder.GTE_FIELD; import static org.elasticsearch.index.query.RangeQueryBuilder.GT_FIELD; @@ -516,25 +519,38 @@ public Query dvRangeQuery(String field, QueryType queryType, Object from, Object } @Override - public Query withinQuery(String field, Object from, Object to, boolean includeLower, boolean includeUpper) { - InetAddress lower = (InetAddress)from; - InetAddress upper = (InetAddress)to; - return InetAddressRange.newWithinQuery(field, - includeLower ? lower : nextUp(lower), includeUpper ? upper : nextDown(upper)); + public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> InetAddressRange.newWithinQuery(field, f, t)); } @Override - public Query containsQuery(String field, Object from, Object to, boolean includeLower, boolean includeUpper) { - InetAddress lower = (InetAddress)from; - InetAddress upper = (InetAddress)to; - return InetAddressRange.newContainsQuery(field, - includeLower ? lower : nextUp(lower), includeUpper ? upper : nextDown(upper)); + public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> InetAddressRange.newContainsQuery(field, f, t )); } @Override - public Query intersectsQuery(String field, Object from, Object to, boolean includeLower, boolean includeUpper) { - InetAddress lower = (InetAddress)from; - InetAddress upper = (InetAddress)to; - return InetAddressRange.newIntersectsQuery(field, - includeLower ? lower : nextUp(lower), includeUpper ? upper : nextDown(upper)); + public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> InetAddressRange.newIntersectsQuery(field, f ,t )); + } + + private Query createQuery(String field, Object lower, Object upper, boolean includeLower, boolean includeUpper, + BiFunction querySupplier) { + byte[] lowerBytes = InetAddressPoint.encode((InetAddress) lower); + byte[] upperBytes = InetAddressPoint.encode((InetAddress) upper); + if (Arrays.compareUnsigned(lowerBytes, 0, lowerBytes.length, upperBytes, 0, upperBytes.length) > 0) { + throw new IllegalArgumentException( + "Range query `from` value (" + lower + ") is greater than `to` value (" + upper + ")"); + } + InetAddress correctedFrom = includeLower ? (InetAddress) lower : nextUp(lower); + InetAddress correctedTo = includeUpper ? (InetAddress) upper : nextDown(upper);; + lowerBytes = InetAddressPoint.encode(correctedFrom); + upperBytes = InetAddressPoint.encode(correctedTo); + if (Arrays.compareUnsigned(lowerBytes, 0, lowerBytes.length, upperBytes, 0, upperBytes.length) > 0) { + return new MatchNoDocsQuery("float range didn't intersect anything"); + } else { + return querySupplier.apply(correctedFrom, correctedTo); + } } }, DATE("date_range", NumberType.LONG) { @@ -662,21 +678,33 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return FloatRange.newWithinQuery(field, - new float[] {includeFrom ? (Float)from : Math.nextUp((Float)from)}, - new float[] {includeTo ? (Float)to : Math.nextDown((Float)to)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> FloatRange.newWithinQuery(field, new float[] { f }, new float[] { t })); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return FloatRange.newContainsQuery(field, - new float[] {includeFrom ? (Float)from : Math.nextUp((Float)from)}, - new float[] {includeTo ? (Float)to : Math.nextDown((Float)to)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> FloatRange.newContainsQuery(field, new float[] { f }, new float[] { t })); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return FloatRange.newIntersectsQuery(field, - new float[] {includeFrom ? (Float)from : Math.nextUp((Float)from)}, - new float[] {includeTo ? (Float)to : Math.nextDown((Float)to)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> FloatRange.newIntersectsQuery(field, new float[] { f }, new float[] { t })); + } + + private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, + BiFunction querySupplier) { + if ((Float) from > (Float) to) { + // wrong argument order, this is an error the user should fix + throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); + } + Float correctedFrom = (Float) from + (includeFrom ? 0 : 1); + Float correctedTo = (Float) to - (includeTo ? 0 : 1); + if (correctedFrom > correctedTo) { + return new MatchNoDocsQuery("float range didn't intersect anything"); + } else { + return querySupplier.apply(correctedFrom, correctedTo); + } } }, DOUBLE("double_range", NumberType.DOUBLE) { @@ -724,21 +752,33 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return DoubleRange.newWithinQuery(field, - new double[] {includeFrom ? (Double)from : Math.nextUp((Double)from)}, - new double[] {includeTo ? (Double)to : Math.nextDown((Double)to)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> DoubleRange.newWithinQuery(field, new double[] { f }, new double[] { t })); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return DoubleRange.newContainsQuery(field, - new double[] {includeFrom ? (Double)from : Math.nextUp((Double)from)}, - new double[] {includeTo ? (Double)to : Math.nextDown((Double)to)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> DoubleRange.newContainsQuery(field, new double[] { f }, new double[] { t })); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return DoubleRange.newIntersectsQuery(field, - new double[] {includeFrom ? (Double)from : Math.nextUp((Double)from)}, - new double[] {includeTo ? (Double)to : Math.nextDown((Double)to)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> DoubleRange.newIntersectsQuery(field, new double[] { f }, new double[] { t })); + } + + private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, + BiFunction querySupplier) { + if ((Double) from > (Double) to) { + // wrong argument order, this is an error the user should fix + throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); + } + Double correctedFrom = (Double) from + (includeFrom ? 0 : 1); + Double correctedTo = (Double) to - (includeTo ? 0 : 1); + if (correctedFrom > correctedTo) { + return new MatchNoDocsQuery("double range didn't intersect anything"); + } else { + return querySupplier.apply(correctedFrom, correctedTo); + } } }, // todo add BYTE support @@ -777,18 +817,33 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return IntRange.newWithinQuery(field, new int[] {(Integer)from + (includeFrom ? 0 : 1)}, - new int[] {(Integer)to - (includeTo ? 0 : 1)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> IntRange.newWithinQuery(field, new int[] { f }, new int[] { t })); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return IntRange.newContainsQuery(field, new int[] {(Integer)from + (includeFrom ? 0 : 1)}, - new int[] {(Integer)to - (includeTo ? 0 : 1)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> IntRange.newContainsQuery(field, new int[] { f }, new int[] { t })); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return IntRange.newIntersectsQuery(field, new int[] {(Integer)from + (includeFrom ? 0 : 1)}, - new int[] {(Integer)to - (includeTo ? 0 : 1)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> IntRange.newIntersectsQuery(field, new int[] { f }, new int[] { t })); + } + + private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, + BiFunction querySupplier) { + if ((Integer) from > (Integer) to) { + // wrong argument order, this is an error the user should fix + throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); + } + Integer correctedFrom = (Integer) from + (includeFrom ? 0 : 1); + Integer correctedTo = (Integer) to - (includeTo ? 0 : 1); + if (correctedFrom > correctedTo) { + return new MatchNoDocsQuery("integer range didn't intersect anything"); + } else { + return querySupplier.apply(correctedFrom, correctedTo); + } } }, LONG("long_range", NumberType.LONG) { @@ -837,18 +892,33 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return LongRange.newWithinQuery(field, new long[] {(Long)from + (includeFrom ? 0 : 1)}, - new long[] {(Long)to - (includeTo ? 0 : 1)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> LongRange.newWithinQuery(field, new long[] { f }, new long[] { t })); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return LongRange.newContainsQuery(field, new long[] {(Long)from + (includeFrom ? 0 : 1)}, - new long[] {(Long)to - (includeTo ? 0 : 1)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> LongRange.newContainsQuery(field, new long[] { f }, new long[] { t })); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return LongRange.newIntersectsQuery(field, new long[] {(Long)from + (includeFrom ? 0 : 1)}, - new long[] {(Long)to - (includeTo ? 0 : 1)}); + return createQuery(field, from, to, includeFrom, includeTo, + (f, t) -> LongRange.newIntersectsQuery(field, new long[] { f }, new long[] { t })); + } + + private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, + BiFunction querySupplier) { + if ((Long) from > (Long) to) { + // wrong argument order, this is an error the user should fix + throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); + } + Long correctedFrom = (Long) from + (includeFrom ? 0 : 1); + Long correctedTo = (Long) to - (includeTo ? 0 : 1); + if (correctedFrom > correctedTo) { + return new MatchNoDocsQuery("long range didn't intersect anything"); + } else { + return querySupplier.apply(correctedFrom, correctedTo); + } } }; diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java index 6ca98fb4db6d2..0f60b02ec4fd3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java @@ -28,6 +28,7 @@ import org.apache.lucene.index.IndexOptions; import org.apache.lucene.queries.BinaryDocValuesRangeQuery; import org.apache.lucene.search.IndexOrDocValuesQuery; +import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; import org.apache.lucene.util.BytesRef; import org.elasticsearch.ElasticsearchParseException; @@ -49,6 +50,7 @@ import java.util.Locale; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; public class RangeFieldTypeTests extends FieldTypeTestCase { RangeType type; @@ -97,6 +99,126 @@ public void testRangeQuery() throws Exception { ft.rangeQuery(from, to, includeLower, includeUpper, relation, null, null, context)); } + /** + * test the intersection queries are correct if from/to are adjacent with different includeUpper/Lower settings + */ + public void testRangeQueryIntersectsAdjacentValues() throws Exception { + QueryShardContext context = createContext(); + ShapeRelation relation = randomFrom(ShapeRelation.values()); + RangeFieldType ft = new RangeFieldType(type); + ft.setName(FIELDNAME); + ft.setIndexOptions(IndexOptions.DOCS); + + Object from = null; + Object to = null; + switch (type) { + case LONG: { + long fromValue = randomLong(); + from = fromValue; + to = fromValue + 1; + break; + } + case DATE: { + long fromValue = randomInt(); + from = new DateTime(fromValue); + to = new DateTime(fromValue + 1); + break; + } + case INTEGER: { + int fromValue = randomInt(); + from = fromValue; + to = fromValue + 1; + break; + } + case DOUBLE: { + double fromValue = randomDoubleBetween(0, 100, true); + from = fromValue; + to = Math.nextUp(fromValue); + break; + } + case FLOAT: { + float fromValue = randomFloat(); + from = fromValue; + to = Math.nextUp(fromValue); + break; + } + case IP: { + byte[] ipv4 = new byte[4]; + random().nextBytes(ipv4); + InetAddress fromValue = InetAddress.getByAddress(ipv4); + from = fromValue; + to = InetAddressPoint.nextUp(fromValue); + break; + } + default: + from = nextFrom(); + to = nextTo(from); + } + Query rangeQuery = ft.rangeQuery(from, to, false, false, relation, null, null, context); + assertThat(rangeQuery, instanceOf(IndexOrDocValuesQuery.class)); + assertThat(((IndexOrDocValuesQuery) rangeQuery).getIndexQuery(), instanceOf(MatchNoDocsQuery.class)); + } + + /** + * check that we catch cases where the user specifies larger "from" than "to" value, not counting the include upper/lower settings + */ + public void testFromLargerToErrors() throws Exception { + QueryShardContext context = createContext(); + RangeFieldType ft = new RangeFieldType(type); + ft.setName(FIELDNAME); + ft.setIndexOptions(IndexOptions.DOCS); + + final Object from; + final Object to; + switch (type) { + case LONG: { + long fromValue = randomLong(); + from = fromValue; + to = fromValue - 1L; + break; + } + case DATE: { + long fromValue = randomInt(); + from = new DateTime(fromValue); + to = new DateTime(fromValue - 1); + break; + } + case INTEGER: { + int fromValue = randomInt(); + from = fromValue; + to = fromValue - 1; + break; + } + case DOUBLE: { + double fromValue = randomDoubleBetween(0, 100, true); + from = fromValue; + to = fromValue - 1.0d; + break; + } + case FLOAT: { + float fromValue = randomFloat(); + from = fromValue; + to = fromValue - 1.0f; + break; + } + case IP: { + byte[] ipv4 = new byte[4]; + random().nextBytes(ipv4); + InetAddress fromValue = InetAddress.getByAddress(ipv4); + from = fromValue; + to = InetAddressPoint.nextDown(fromValue); + break; + } + default: + // quit test for other range types + return; + } + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, + () -> ft.rangeQuery(from, to, true, true, ShapeRelation.INTERSECTS, null, null, context)); + assertTrue(ex.getMessage().contains("Range query `from` value")); + assertTrue(ex.getMessage().contains("is greater than `to` value")); + } + private QueryShardContext createContext() { Settings indexSettings = Settings.builder() .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT).build(); From f15431ddca0584ef196378c459a42a56e052c241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 15 Apr 2019 10:40:29 +0200 Subject: [PATCH 2/3] adressing comments --- .../index/mapper/RangeFieldMapper.java | 125 ++++++------------ .../index/mapper/RangeFieldTypeTests.java | 11 +- 2 files changed, 50 insertions(+), 86 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index 1c0c4d967a53d..a5441245c4ff5 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -678,33 +678,18 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> FloatRange.newWithinQuery(field, new float[] { f }, new float[] { t })); + return createQuery(field, (Float) from, (Float) to, includeFrom, includeTo, + (f, t) -> FloatRange.newWithinQuery(field, new float[] { f }, new float[] { t }), RangeType.FLOAT); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> FloatRange.newContainsQuery(field, new float[] { f }, new float[] { t })); + return createQuery(field, (Float) from, (Float) to, includeFrom, includeTo, + (f, t) -> FloatRange.newContainsQuery(field, new float[] { f }, new float[] { t }), RangeType.FLOAT); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> FloatRange.newIntersectsQuery(field, new float[] { f }, new float[] { t })); - } - - private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, - BiFunction querySupplier) { - if ((Float) from > (Float) to) { - // wrong argument order, this is an error the user should fix - throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); - } - Float correctedFrom = (Float) from + (includeFrom ? 0 : 1); - Float correctedTo = (Float) to - (includeTo ? 0 : 1); - if (correctedFrom > correctedTo) { - return new MatchNoDocsQuery("float range didn't intersect anything"); - } else { - return querySupplier.apply(correctedFrom, correctedTo); - } + return createQuery(field, (Float) from, (Float) to, includeFrom, includeTo, + (f, t) -> FloatRange.newIntersectsQuery(field, new float[] { f }, new float[] { t }), RangeType.FLOAT); } }, DOUBLE("double_range", NumberType.DOUBLE) { @@ -752,34 +737,20 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> DoubleRange.newWithinQuery(field, new double[] { f }, new double[] { t })); + return createQuery(field, (Double) from, (Double) to, includeFrom, includeTo, + (f, t) -> DoubleRange.newWithinQuery(field, new double[] { f }, new double[] { t }), RangeType.DOUBLE); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> DoubleRange.newContainsQuery(field, new double[] { f }, new double[] { t })); + return createQuery(field, (Double) from, (Double) to, includeFrom, includeTo, + (f, t) -> DoubleRange.newContainsQuery(field, new double[] { f }, new double[] { t }), RangeType.DOUBLE); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> DoubleRange.newIntersectsQuery(field, new double[] { f }, new double[] { t })); + return createQuery(field, (Double) from, (Double) to, includeFrom, includeTo, + (f, t) -> DoubleRange.newIntersectsQuery(field, new double[] { f }, new double[] { t }), RangeType.DOUBLE); } - private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, - BiFunction querySupplier) { - if ((Double) from > (Double) to) { - // wrong argument order, this is an error the user should fix - throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); - } - Double correctedFrom = (Double) from + (includeFrom ? 0 : 1); - Double correctedTo = (Double) to - (includeTo ? 0 : 1); - if (correctedFrom > correctedTo) { - return new MatchNoDocsQuery("double range didn't intersect anything"); - } else { - return querySupplier.apply(correctedFrom, correctedTo); - } - } }, // todo add BYTE support // todo add SHORT support @@ -817,33 +788,18 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> IntRange.newWithinQuery(field, new int[] { f }, new int[] { t })); + return createQuery(field, (Integer) from, (Integer) to, includeFrom, includeTo, + (f, t) -> IntRange.newWithinQuery(field, new int[] { f }, new int[] { t }), RangeType.INTEGER); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> IntRange.newContainsQuery(field, new int[] { f }, new int[] { t })); + return createQuery(field, (Integer) from, (Integer) to, includeFrom, includeTo, + (f, t) -> IntRange.newContainsQuery(field, new int[] { f }, new int[] { t }), RangeType.INTEGER); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> IntRange.newIntersectsQuery(field, new int[] { f }, new int[] { t })); - } - - private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, - BiFunction querySupplier) { - if ((Integer) from > (Integer) to) { - // wrong argument order, this is an error the user should fix - throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); - } - Integer correctedFrom = (Integer) from + (includeFrom ? 0 : 1); - Integer correctedTo = (Integer) to - (includeTo ? 0 : 1); - if (correctedFrom > correctedTo) { - return new MatchNoDocsQuery("integer range didn't intersect anything"); - } else { - return querySupplier.apply(correctedFrom, correctedTo); - } + return createQuery(field, (Integer) from, (Integer) to, includeFrom, includeTo, + (f, t) -> IntRange.newIntersectsQuery(field, new int[] { f }, new int[] { t }), RangeType.INTEGER); } }, LONG("long_range", NumberType.LONG) { @@ -892,33 +848,18 @@ public Field getRangeField(String name, Range r) { } @Override public Query withinQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> LongRange.newWithinQuery(field, new long[] { f }, new long[] { t })); + return createQuery(field, (Long) from, (Long) to, includeFrom, includeTo, + (f, t) -> LongRange.newWithinQuery(field, new long[] { f }, new long[] { t }), RangeType.LONG); } @Override public Query containsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> LongRange.newContainsQuery(field, new long[] { f }, new long[] { t })); + return createQuery(field, (Long) from, (Long) to, includeFrom, includeTo, + (f, t) -> LongRange.newContainsQuery(field, new long[] { f }, new long[] { t }), RangeType.LONG); } @Override public Query intersectsQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo) { - return createQuery(field, from, to, includeFrom, includeTo, - (f, t) -> LongRange.newIntersectsQuery(field, new long[] { f }, new long[] { t })); - } - - private Query createQuery(String field, Object from, Object to, boolean includeFrom, boolean includeTo, - BiFunction querySupplier) { - if ((Long) from > (Long) to) { - // wrong argument order, this is an error the user should fix - throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); - } - Long correctedFrom = (Long) from + (includeFrom ? 0 : 1); - Long correctedTo = (Long) to - (includeTo ? 0 : 1); - if (correctedFrom > correctedTo) { - return new MatchNoDocsQuery("long range didn't intersect anything"); - } else { - return querySupplier.apply(correctedFrom, correctedTo); - } + return createQuery(field, (Long) from, (Long) to, includeFrom, includeTo, + (f, t) -> LongRange.newIntersectsQuery(field, new long[] { f }, new long[] { t }), RangeType.LONG); } }; @@ -936,6 +877,24 @@ private Query createQuery(String field, Object from, Object to, boolean includeF public final String typeName() { return name; } + + private static > Query createQuery(String field, T from, T to, boolean includeFrom, boolean includeTo, + BiFunction querySupplier, RangeType rangeType) { + if (from.compareTo(to) > 0) { + // wrong argument order, this is an error the user should fix + throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); + } + + @SuppressWarnings("unchecked") + T correctedFrom = includeFrom ? from : (T) rangeType.nextUp(from); + @SuppressWarnings("unchecked") + T correctedTo = includeTo ? to : (T) rangeType.nextDown(to); + if (correctedFrom.compareTo(correctedTo) > 0) { + return new MatchNoDocsQuery("range didn't intersect anything"); + } else { + return querySupplier.apply(correctedFrom, correctedTo); + } + } public abstract Field getRangeField(String name, Range range); public List createFields(ParseContext context, String name, Range range, boolean indexed, diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java index 0f60b02ec4fd3..a26999fa3a6f5 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RangeFieldTypeTests.java @@ -94,13 +94,17 @@ public void testRangeQuery() throws Exception { boolean includeUpper = randomBoolean(); Object from = nextFrom(); Object to = nextTo(from); + if (includeLower == false && includeUpper == false) { + // need to increase once more, otherwise interval is empty because edge values are exclusive + to = nextTo(to); + } assertEquals(getExpectedRangeQuery(relation, from, to, includeLower, includeUpper), ft.rangeQuery(from, to, includeLower, includeUpper, relation, null, null, context)); } /** - * test the intersection queries are correct if from/to are adjacent with different includeUpper/Lower settings + * test the queries are correct if from/to are adjacent and the range is exclusive of those values */ public void testRangeQueryIntersectsAdjacentValues() throws Exception { QueryShardContext context = createContext(); @@ -213,8 +217,9 @@ public void testFromLargerToErrors() throws Exception { // quit test for other range types return; } + ShapeRelation relation = randomFrom(ShapeRelation.values()); IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> ft.rangeQuery(from, to, true, true, ShapeRelation.INTERSECTS, null, null, context)); + () -> ft.rangeQuery(from, to, true, true, relation, null, null, context)); assertTrue(ex.getMessage().contains("Range query `from` value")); assertTrue(ex.getMessage().contains("is greater than `to` value")); } @@ -226,7 +231,7 @@ private QueryShardContext createContext() { return new QueryShardContext(0, idxSettings, null, null, null, null, null, xContentRegistry(), writableRegistry(), null, null, () -> nowInMillis, null); } - + public void testDateRangeQueryUsingMappingFormat() { QueryShardContext context = createContext(); RangeFieldType fieldType = new RangeFieldType(RangeType.DATE); From d021a7e5396d54d2f53e72cf7c03ed821ee216ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 15 Apr 2019 15:43:26 +0200 Subject: [PATCH 3/3] add comment --- .../index/mapper/RangeFieldMapper.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index a5441245c4ff5..095f84e61563d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -533,7 +533,7 @@ public Query intersectsQuery(String field, Object from, Object to, boolean inclu return createQuery(field, from, to, includeFrom, includeTo, (f, t) -> InetAddressRange.newIntersectsQuery(field, f ,t )); } - + private Query createQuery(String field, Object lower, Object upper, boolean includeLower, boolean includeUpper, BiFunction querySupplier) { byte[] lowerBytes = InetAddressPoint.encode((InetAddress) lower); @@ -750,7 +750,7 @@ public Query intersectsQuery(String field, Object from, Object to, boolean inclu return createQuery(field, (Double) from, (Double) to, includeFrom, includeTo, (f, t) -> DoubleRange.newIntersectsQuery(field, new double[] { f }, new double[] { t }), RangeType.DOUBLE); } - + }, // todo add BYTE support // todo add SHORT support @@ -877,14 +877,21 @@ public Query intersectsQuery(String field, Object from, Object to, boolean inclu public final String typeName() { return name; } - + + /** + * Internal helper to create the actual {@link Query} using the provided supplier function. Before creating the query we check if + * the intervals min > max, in which case an {@link IllegalArgumentException} is raised. The method adapts the interval bounds + * based on whether the edges should be included or excluded. In case where after this correction the interval would be empty + * because min > max, we simply return a {@link MatchNoDocsQuery}. + * This helper handles all {@link Number} cases and dates, the IP range type uses its own logic. + */ private static > Query createQuery(String field, T from, T to, boolean includeFrom, boolean includeTo, BiFunction querySupplier, RangeType rangeType) { if (from.compareTo(to) > 0) { // wrong argument order, this is an error the user should fix throw new IllegalArgumentException("Range query `from` value (" + from + ") is greater than `to` value (" + to + ")"); } - + @SuppressWarnings("unchecked") T correctedFrom = includeFrom ? from : (T) rangeType.nextUp(from); @SuppressWarnings("unchecked")