Skip to content
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

Fix range query edge cases #41160

Merged
merged 5 commits into from
Apr 16, 2019
Merged

Fix range query edge cases #41160

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d07d9fc
Fix range query edge cases
Apr 8, 2019
a0d63fa
Merge branch 'master' into fix-40937
Apr 15, 2019
f15431d
adressing comments
Apr 15, 2019
d021a7e
add comment
Apr 15, 2019
201aa56
Merge branch 'master' into fix-40937
Apr 15, 2019
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
160 changes: 115 additions & 45 deletions server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -63,13 +64,15 @@
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;
import java.util.Locale;
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;
Expand Down Expand Up @@ -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<InetAddress, InetAddress, Query> 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) {
Expand Down Expand Up @@ -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<Float, Float, Query> 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);
Copy link
Member

Choose a reason for hiding this comment

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

See below comment for doubles ranges; floats should also use nextUp() instead of just increasing by 1.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll try creating only one createQuery function...

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) {
Expand Down Expand Up @@ -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<Double, Double, Query> 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);
Copy link
Member

Choose a reason for hiding this comment

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

I think increasing by 1 here is wrong; that's too big a jump for doubles. We should use RangeType.DOUBLE.nextUp() here, I think. E.g., if a user specifies the exclusive range (0, 1), we would expect it to intersect the range (.3, .7), but this logic will return MatchNoDocsQuery.

In fact, I wonder if we should just have one createQuery function that's written in terms ofnextUp(), possibly accepting a comparator too, if necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I had this in an earlier version of this PR, must have slipped, will update.

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
Expand Down Expand Up @@ -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<Integer, Integer, Query> 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) {
Expand Down Expand Up @@ -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<Long, Long, Query> 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);
}
}
};

Expand Down
Loading