Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 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
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,22 +338,35 @@ 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) {
final Double result;

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

@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 @@ -385,6 +428,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
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@

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 java.io.IOException;
import java.util.List;
import java.util.Arrays;
import java.util.HashSet;

Expand Down Expand Up @@ -314,4 +316,76 @@ public void testEmptyName() throws IOException {
assertThat(e.getMessage(), containsString("name cannot be empty string"));
}
}

public void testOutOfRangeValues() throws IOException {
final List<Triple<String, Object, String>> inputs = Arrays.asList(
Triple.of("byte", "128", "is out of range for a byte"),
Triple.of("short", "32768", "is out of range for a short"),
Triple.of("integer", "2147483648", "For input string"),
Triple.of("long", "92233720368547758080", "For input string"),

Triple.of("half_float", "65504.1", "[half_float] supports only finite values"),
Triple.of("float", "3.4028235E39", "[float] supports only finite values"),
Triple.of("double", "1.7976931348623157E309", "[double] supports only finite values"),

Triple.of("half_float", Float.NaN, "[half_float] supports only finite values"),
Triple.of("float", Float.NaN, "[float] supports only finite values"),
Triple.of("double", Double.NaN, "[double] supports only finite values"),

Triple.of("half_float", Float.POSITIVE_INFINITY, "[half_float] supports only finite values"),
Triple.of("float", Float.POSITIVE_INFINITY, "[float] supports only finite values"),
Triple.of("double", Double.POSITIVE_INFINITY, "[double] supports only finite values")
);

for(Triple<String, Object, String> 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 static class Triple<K,V,M> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this being a generic class with a generic name can we call this OutOfRangeSpec, remove the generic arguments and instead use K -> String, V -> Number, M -> String. Also I think it would be ok for you to declare this class here and then reuse it in the NumberFieldTypeTests below instead of re-defining it

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 only keep generic argument for value because I use both strings and numbers (BigInteger and BigDecimal) as input values.


final K type;
final V value;
final M message;

static <K,V,M> Triple<K,V,M> of(K t, V v, M m) {
return new Triple<>(t, v, m);
}

Triple(K t, V v, M m) {
type = t;
value = v;
message = m;
}
}

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

private DocumentMapper createDocumentMapper(String type) throws IOException {
String mapping = XContentFactory.jsonBuilder()
.startObject()
.startObject("type")
.startObject("properties")
.startObject("field")
.field("type", type)
.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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@
import org.junit.Before;

import java.io.IOException;
import java.math.BigDecimal;
import java.math.BigInteger;
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 @@ -266,8 +271,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 @@ -348,4 +353,61 @@ public void doTestDocValueRangeQueries(NumberType type, Supplier<Number> valueSu
reader.close();
dir.close();
}

public void testParseOutOfRangeValues() throws IOException {
final List<Triple<NumberType, Object, String>> inputs = Arrays.asList(
Triple.of(NumberType.BYTE, "128", "Value out of range"),
Triple.of(NumberType.SHORT, "32768", "Value out of range"),
Triple.of(NumberType.INTEGER, "2147483648", "For input string"),
Triple.of(NumberType.LONG, "9223372036854775808", "For input string"),

Triple.of(NumberType.BYTE, 128, "is out of range for a byte"),
Triple.of(NumberType.SHORT, 32768, "is out of range for a short"),
Triple.of(NumberType.INTEGER, 2147483648L, "is out of range for an integer"),
Triple.of(NumberType.LONG, new BigInteger("92233720368547758080"), " is out of range for a long"),

Triple.of(NumberType.HALF_FLOAT, "65504.1", "[half_float] supports only finite values"),
Triple.of(NumberType.FLOAT, "3.4028235E39", "[float] supports only finite values"),
Triple.of(NumberType.DOUBLE, "1.7976931348623157E309", "[double] supports only finite values"),

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

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

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

for (Triple<NumberType, Object, String> 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

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

}
}

private static class Triple<K,V,M> {

final K type;
final V value;
final M message;

static <K,V,M> Triple<K,V,M> of(K t, V v, M m) {
return new Triple<>(t, v, m);
}

Triple(K t, V v, M m) {
type = t;
value = v;
message = m;
}
}
}