Skip to content

Reject out of range numbers for float, double and half_float #25826

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 20 commits into from
Aug 9, 2017
Merged

Reject out of range numbers for float, double and half_float #25826

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2d0e5c1
validate half float values
fred84 Jul 12, 2017
c0fff6f
Merge branch 'master' into 25534_reject_out_of_range_numbers
fred84 Jul 13, 2017
6e0a6ea
test upper bound for numeric mapper
fred84 Jul 17, 2017
7d4f315
merge master
fred84 Jul 21, 2017
d1ebd6f
test for upper bound for float, double and half_float
fred84 Jul 21, 2017
1358fed
Merge branch 'master' into 25533_reject_out_of_range_numbers
fred84 Jul 22, 2017
44983d8
more tests on NaN and Infinity for NumberFieldMapper
fred84 Jul 23, 2017
d072e69
Merge branch 'master' into 25534_reject_out_of_range_numbers
fred84 Jul 23, 2017
ecf3424
fix checkstyle errors
fred84 Jul 23, 2017
b5d231d
minor renaming
fred84 Jul 23, 2017
187982a
resolve merge conflict and cleanup NumberFieldMapper out of range tests
fred84 Jul 27, 2017
d236158
Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…
fred84 Jul 27, 2017
7423c4d
comments for disabled test
fred84 Jul 27, 2017
8e88c9d
Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…
fred84 Jul 30, 2017
11ce7c8
tests for byte/short/integer/long removed and will be added in separa…
fred84 Jul 30, 2017
3902911
remove unused import
fred84 Jul 30, 2017
656cf63
Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…
fred84 Aug 4, 2017
cd38455
Fix scaledfloat out of range validation message
fred84 Aug 4, 2017
b578b5a
Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…
fred84 Aug 8, 2017
3c666a9
1) delayed autoboxing in numbertype.parse(...)
fred84 Aug 8, 2017
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
66 changes: 56 additions & 10 deletions core/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,25 @@ public enum NumberType {
HALF_FLOAT("half_float", NumericType.HALF_FLOAT) {
@Override
Float parse(Object value, boolean coerce) {
return (Float) FLOAT.parse(value, false);
final Float result;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's store it as a float in order to delay boxing as much as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, I will fix in half_float, float and double.


if (value instanceof Number) {
result = ((Number) value).floatValue();
} else {
if (value instanceof BytesRef) {
value = ((BytesRef) value).utf8ToString();
}
result = Float.parseFloat(value.toString());
}
validateParsed(result);
return result;
}

@Override
Float parse(XContentParser parser, boolean coerce) throws IOException {
return parser.floatValue(coerce);
Float parsed = parser.floatValue(coerce);
validateParsed(parsed);
return parsed;
}

@Override
Expand Down Expand Up @@ -231,22 +244,39 @@ public List<Field> createFields(String name, Number value,
}
return fields;
}

private void validateParsed(Float value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make it take a float so that we do not have to worry about null values

if (
value.isNaN() || value.isInfinite()
|| value > 65504
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably be Math.abs(value) >= 65520 rather than 65504. 65504 is indeed the maximum value but values up to 65520 excluded would be rounded to 65504

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with Math.abs, but do not understand about 65520. As I understand all finite floats greater than 65504 will be rounded to 65504 inside HalfFloatPoint.halfFloatToShortBits. Am I right?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Floats that are between 65504 and 65520 will be rounded to 65504 however floats that are equal or greater than 65520 will be converted to +Infinity.

|| !Float.isFinite(HalfFloatPoint.sortableShortToHalfFloat(HalfFloatPoint.halfFloatToSortableShort(value)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Those last checks are redundant. We should do only one of them.

) {
throw new IllegalArgumentException("[half_float] supports only finite values, but got [" + value + "]");
}
}
},
FLOAT("float", NumericType.FLOAT) {
@Override
Float parse(Object value, boolean coerce) {
final Float result;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's store it as a float in order to delay boxing as much as possible?


if (value instanceof Number) {
return ((Number) value).floatValue();
}
if (value instanceof BytesRef) {
value = ((BytesRef) value).utf8ToString();
result = ((Number) value).floatValue();
} else {
if (value instanceof BytesRef) {
value = ((BytesRef) value).utf8ToString();
}
result = Float.parseFloat(value.toString());
}
return Float.parseFloat(value.toString());
validateParsed(result);
return result;
}

@Override
Float parse(XContentParser parser, boolean coerce) throws IOException {
return parser.floatValue(coerce);
Float parsed = parser.floatValue(coerce);
validateParsed(parsed);
return parsed;
}

@Override
Expand Down Expand Up @@ -308,16 +338,26 @@ public List<Field> createFields(String name, Number value,
}
return fields;
}

private void validateParsed(Float value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please make it a float rather than Float and use Float.isFinite(value) == false below

if (value.isInfinite() || value.isNaN()) {
throw new IllegalArgumentException("[float] supports only finite values, but got [" + value + "]");
}
}
},
DOUBLE("double", NumericType.DOUBLE) {
@Override
Double parse(Object value, boolean coerce) {
return objectToDouble(value);
Double parsed = objectToDouble(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's store it as a double in order to delay boxing as much as possible?

validateParsed(parsed);
return parsed;
}

@Override
Double parse(XContentParser parser, boolean coerce) throws IOException {
return parser.doubleValue(coerce);
Double parsed = parser.doubleValue(coerce);
validateParsed(parsed);
return parsed;
}

@Override
Expand Down Expand Up @@ -379,6 +419,12 @@ public List<Field> createFields(String name, Number value,
}
return fields;
}

private void validateParsed(Double value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please make it a double rather than Double and use Double.isFinite(value) == false below

if (value.isInfinite() || value.isNaN()) {
throw new IllegalArgumentException("[double] supports only finite values, but got [" + value + "]");
}
}
},
BYTE("byte", NumericType.BYTE) {
@Override
Expand Down
42 changes: 34 additions & 8 deletions core/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.lucene.search.BoostQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.SortField;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.settings.Setting;
Expand Down Expand Up @@ -144,7 +145,7 @@ public Mapper.Builder<?,?> parse(String name, Map<String, Object> node,
if (propNode == null) {
throw new MapperParsingException("Property [null_value] cannot be null.");
}
builder.nullValue(NumberFieldMapper.NumberType.DOUBLE.parse(propNode, false));
builder.nullValue(ScaledFloatFieldMapper.parse(propNode));
iterator.remove();
} else if (propName.equals("ignore_malformed")) {
builder.ignoreMalformed(TypeParsers.nodeBooleanValue(name, "ignore_malformed", propNode, parserContext));
Expand All @@ -153,7 +154,7 @@ public Mapper.Builder<?,?> parse(String name, Map<String, Object> node,
builder.coerce(TypeParsers.nodeBooleanValue(name, "coerce", propNode, parserContext));
iterator.remove();
} else if (propName.equals("scaling_factor")) {
builder.scalingFactor(NumberFieldMapper.NumberType.DOUBLE.parse(propNode, false).doubleValue());
builder.scalingFactor(ScaledFloatFieldMapper.parse(propNode));
iterator.remove();
}
}
Expand Down Expand Up @@ -207,7 +208,7 @@ public void checkCompatibility(MappedFieldType other, List<String> conflicts, bo
@Override
public Query termQuery(Object value, QueryShardContext context) {
failIfNotIndexed();
double queryValue = NumberFieldMapper.NumberType.DOUBLE.parse(value, false).doubleValue();
double queryValue = parse(value);
long scaledValue = Math.round(queryValue * scalingFactor);
Query query = NumberFieldMapper.NumberType.LONG.termQuery(name(), scaledValue);
if (boost() != 1f) {
Expand All @@ -221,7 +222,7 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
failIfNotIndexed();
List<Long> scaledValues = new ArrayList<>(values.size());
for (Object value : values) {
double queryValue = NumberFieldMapper.NumberType.DOUBLE.parse(value, false).doubleValue();
double queryValue = parse(value);
long scaledValue = Math.round(queryValue * scalingFactor);
scaledValues.add(scaledValue);
}
Expand All @@ -237,15 +238,15 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower
failIfNotIndexed();
Long lo = null;
if (lowerTerm != null) {
double dValue = NumberFieldMapper.NumberType.DOUBLE.parse(lowerTerm, false).doubleValue();
double dValue = parse(lowerTerm);
if (includeLower == false) {
dValue = Math.nextUp(dValue);
}
lo = Math.round(Math.ceil(dValue * scalingFactor));
}
Long hi = null;
if (upperTerm != null) {
double dValue = NumberFieldMapper.NumberType.DOUBLE.parse(upperTerm, false).doubleValue();
double dValue = parse(upperTerm);
if (includeUpper == false) {
dValue = Math.nextDown(dValue);
}
Expand Down Expand Up @@ -366,7 +367,7 @@ protected void parseCreateField(ParseContext context, List<IndexableField> field
value = null;
} else {
try {
numericValue = NumberFieldMapper.NumberType.DOUBLE.parse(parser, coerce.value());
numericValue = parse(parser, coerce.value());
} catch (IllegalArgumentException e) {
if (ignoreMalformed.value()) {
return;
Expand All @@ -390,7 +391,7 @@ protected void parseCreateField(ParseContext context, List<IndexableField> field
}

if (numericValue == null) {
numericValue = NumberFieldMapper.NumberType.DOUBLE.parse(value, false);
numericValue = parse(value);
}

if (includeInAll) {
Expand Down Expand Up @@ -451,6 +452,31 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
}
}

static Double parse(Object value) {
return objectToDouble(value);
}

private static Double parse(XContentParser parser, boolean coerce) throws IOException {
return parser.doubleValue(coerce);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this helper method does not look very useful


/**
* Converts an Object to a double by checking it against known types first
*/
private static double objectToDouble(Object value) {
double doubleValue;

if (value instanceof Number) {
doubleValue = ((Number) value).doubleValue();
} else if (value instanceof BytesRef) {
doubleValue = Double.parseDouble(((BytesRef) value).utf8ToString());
} else {
doubleValue = Double.parseDouble(value.toString());
}

return doubleValue;
}

private static class ScaledFloatIndexFieldData implements IndexNumericFieldData {

private final IndexNumericFieldData scaledFieldData;
Expand Down
53 changes: 53 additions & 0 deletions core/src/test/java/org/elasticsearch/index/mapper/NumberFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,15 @@

import org.apache.lucene.index.DocValuesType;
import org.apache.lucene.index.IndexableField;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.mapper.NumberFieldMapper.NumberType;
import org.elasticsearch.index.mapper.NumberFieldTypeTests.OutOfRangeSpec;

import java.io.IOException;
import java.util.List;
import java.util.Arrays;
import java.util.HashSet;

Expand Down Expand Up @@ -339,4 +343,53 @@ public void testEmptyName() throws IOException {
}
}

public void testOutOfRangeValues() throws IOException {
final List<OutOfRangeSpec<Object>> inputs = Arrays.asList(
OutOfRangeSpec.of(NumberType.HALF_FLOAT, "65504.1", "[half_float] supports only finite values"),
OutOfRangeSpec.of(NumberType.FLOAT, "3.4028235E39", "[float] supports only finite values"),
OutOfRangeSpec.of(NumberType.DOUBLE, "1.7976931348623157E309", "[double] supports only finite values"),

OutOfRangeSpec.of(NumberType.HALF_FLOAT, Float.NaN, "[half_float] supports only finite values"),
OutOfRangeSpec.of(NumberType.FLOAT, Float.NaN, "[float] supports only finite values"),
OutOfRangeSpec.of(NumberType.DOUBLE, Double.NaN, "[double] supports only finite values"),

OutOfRangeSpec.of(NumberType.HALF_FLOAT, Float.POSITIVE_INFINITY, "[half_float] supports only finite values"),
OutOfRangeSpec.of(NumberType.FLOAT, Float.POSITIVE_INFINITY, "[float] supports only finite values"),
OutOfRangeSpec.of(NumberType.DOUBLE, Double.POSITIVE_INFINITY, "[double] supports only finite values")
);

for(OutOfRangeSpec<Object> item: inputs) {
try {
parseRequest(item.type, createIndexRequest(item.value));
fail("Mapper parsing exception expected for [" + item.type + "] with value [" + item.value + "]");
} catch (MapperParsingException e) {
assertThat("Incorrect error message for [" + item.type + "] with value [" + item.value + "]",
e.getCause().getMessage(), containsString(item.message));
}
}
}

private void parseRequest(NumberType type, BytesReference content) throws IOException {
createDocumentMapper(type).parse(SourceToParse.source("test", "type", "1", content, XContentType.JSON));
}

private DocumentMapper createDocumentMapper(NumberType type) throws IOException {
String mapping = XContentFactory.jsonBuilder()
.startObject()
.startObject("type")
.startObject("properties")
.startObject("field")
.field("type", type.typeName())
.endObject()
.endObject()
.endObject()
.endObject()
.string();

return parser.parse("type", new CompressedXContent(mapping));
}

private BytesReference createIndexRequest(Object value) throws IOException {
return XContentFactory.jsonBuilder().startObject().field("field", value).endObject().bytes();
}
}
55 changes: 53 additions & 2 deletions core/src/test/java/org/elasticsearch/index/mapper/NumberFieldTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,14 @@
import org.junit.Before;

import java.io.IOException;
import java.math.BigDecimal;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.List;
import java.util.function.Supplier;

import static org.hamcrest.Matchers.containsString;

public class NumberFieldTypeTests extends FieldTypeTestCase {

NumberType type;
Expand Down Expand Up @@ -300,8 +304,8 @@ public void testHalfFloatRange() throws IOException {
IndexSearcher searcher = newSearcher(reader);
final int numQueries = 1000;
for (int i = 0; i < numQueries; ++i) {
float l = (randomFloat() * 2 - 1) * 70000;
float u = (randomFloat() * 2 - 1) * 70000;
float l = (randomFloat() * 2 - 1) * 65504;
float u = (randomFloat() * 2 - 1) * 65504;
boolean includeLower = randomBoolean();
boolean includeUpper = randomBoolean();
Query floatQ = NumberFieldMapper.NumberType.FLOAT.rangeQuery("float", l, u, includeLower, includeUpper, false);
Expand Down Expand Up @@ -382,4 +386,51 @@ public void doTestDocValueRangeQueries(NumberType type, Supplier<Number> valueSu
reader.close();
dir.close();
}

public void testParseOutOfRangeValues() throws IOException {
final List<OutOfRangeSpec<Object>> inputs = Arrays.asList(
OutOfRangeSpec.of(NumberType.HALF_FLOAT, "65504.1", "[half_float] supports only finite values"),
OutOfRangeSpec.of(NumberType.FLOAT, "3.4028235E39", "[float] supports only finite values"),
OutOfRangeSpec.of(NumberType.DOUBLE, "1.7976931348623157E309", "[double] supports only finite values"),

OutOfRangeSpec.of(NumberType.HALF_FLOAT, 65504.1, "[half_float] supports only finite values"),
OutOfRangeSpec.of(NumberType.FLOAT, 3.4028235E39, "[float] supports only finite values"),
OutOfRangeSpec.of(NumberType.DOUBLE, new BigDecimal("1.7976931348623157E309"), "[double] supports only finite values"),
Copy link
Contributor

Choose a reason for hiding this comment

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

can you test negative values too?


OutOfRangeSpec.of(NumberType.HALF_FLOAT, Float.NaN, "[half_float] supports only finite values"),
OutOfRangeSpec.of(NumberType.FLOAT, Float.NaN, "[float] supports only finite values"),
OutOfRangeSpec.of(NumberType.DOUBLE, Double.NaN, "[double] supports only finite values"),

OutOfRangeSpec.of(NumberType.HALF_FLOAT, Float.POSITIVE_INFINITY, "[half_float] supports only finite values"),
OutOfRangeSpec.of(NumberType.FLOAT, Float.POSITIVE_INFINITY, "[float] supports only finite values"),
OutOfRangeSpec.of(NumberType.DOUBLE, Double.POSITIVE_INFINITY, "[double] supports only finite values")
);

for (OutOfRangeSpec<Object> item: inputs) {
try {
item.type.parse(item.value, false);
fail("Parsing exception expected for [" + item.type + "] with value [" + item.value + "]");
} catch (IllegalArgumentException e) {
assertThat("Incorrect error message for [" + item.type + "] with value [" + item.value + "]",
e.getMessage(), containsString(item.message));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could you use expectThrows instead of try/fail/catch/assert?

Copy link
Contributor Author

@fred84 fred84 Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Despite expectThrows is more concise and readable, I think we should keep try/fail/catch/assert because
fail("Mapper parsing exception expected for [" + item.type + "] with value [" + item.value + "]");
shows which numbertype with which value failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

}
}

static class OutOfRangeSpec<V> {

final NumberType type;
final V value;
final String message;

static <V> OutOfRangeSpec<V> of(NumberType t, V v, String m) {
return new OutOfRangeSpec<>(t, v, m);
}

OutOfRangeSpec(NumberType t, V v, String m) {
type = t;
value = v;
message = m;
}
}
}