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

fred84 · 2017-07-21T08:00:54Z

@colings86 Please take a look.

elasticmachine · 2017-07-21T08:00:56Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

colings86

@fred84 Thanks for the PR, I left a few small comments.

colings86 · 2017-07-21T08:19:11Z

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

@@ -370,6 +390,13 @@ Query rangeQuery(String field, Object lowerTerm, Object upperTerm,
            }

            @Override
+            void validateParsed(Number value) {
+                if (!Double.isFinite(value.doubleValue())) {
+                    throw new IllegalArgumentException("[double] supports only finite values, but got [" + value.toString() + "]");


Can we use this message format where we show what we got as an invalid value in the error messages for the other types too?

I updated message format.

colings86 · 2017-07-21T08:20:55Z

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

+         * @throws IllegalArgumentException if value is not finite for this type
+         */
+        void validateParsed(Number value) {
+        }


Since every numeric type should implement this method, can we have this throw an UnsupportedOperationException here so we get an error if we implement a new numeric type and forget to override this method?

Or just make it abstract

I made validation as a part of parse method in the same way as it did in byte/short/int/long. Thanks for suggestion, @rjernst!

colings86 · 2017-07-21T08:21:45Z

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

+        final Map<String, String> outOfRangeValues = new HashMap<>();
+        outOfRangeValues.put("float", new BigDecimal("3.4028235E39").toString());
+        outOfRangeValues.put("double", new BigDecimal("1.7976931348623157E309").toString());
+        outOfRangeValues.put("half_float", new BigDecimal("65504.1").toString());


As well as these values can we add infinite and Nan values to this test?

Tests are ready. Also added out of range tests for byte/short/int/long

colings86 · 2017-07-21T08:23:47Z

jenkins test this

colings86 · 2017-07-21T08:58:44Z

@fred84 the build failed but the failure doesn't seem to be related to your change. It also doesn't reproduce for me locally so I'm going to kick off another build and see if we get the error again.

colings86 · 2017-07-21T08:58:53Z

jenkins retest this

rjernst

Why would the validation just be part of the parse method for each type? Then it would not have to be retrieved back out of Number.

rjernst · 2017-07-21T16:15:25Z

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

+         * @throws IllegalArgumentException if value is not finite for this type
+         */
+        void validateParsed(Number value) {
+        }


Or just make it abstract

fred84 · 2017-07-23T17:30:25Z

@colings86 I updated PR. There some code duplication left, I'm not sure where to put Triple class used in 2 test cases.

colings86

@fred84 Thanks for updating the PR, I left a comment about the Triple class but I think this is getting close.

Could you also rebase your branch on the latest master and resolve the merge conflicts?

colings86 · 2017-07-26T15:10:31Z

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

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


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

Agreed. I only keep generic argument for value because I use both strings and numbers (BigInteger and BigDecimal) as input values.

…of_range_numbers

fred84 · 2017-07-27T12:46:38Z

@colings86 PR is updated. I also found that validation for min/max values in short/integer/long works not as intended. I will provide more details a bit later).

fred84 · 2017-07-28T08:26:17Z

@colings86 I moved proposed changes for byte/short/int/long validation to separate PR: fred84#2

…of_range_numbers

…te PR

colings86 · 2017-08-01T12:11:00Z

@fred84 I'm a little confused, are you saying that fred84#2 replaces this PR? IF so, could you open that PR against this repo instead of against your fork?

fred84 · 2017-08-01T14:06:39Z

@colings86 I think we should solve half_float/float/double validation in this PR and then I will create separate PR for byte/short/int/double.

jpountz

It looks good to me overall but I left some minor comments about the handling of half floats and boxing.

jpountz · 2017-08-07T12:49:13Z

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

+            private void validateParsed(Float value) {
+                if (
+                    value.isNaN() || value.isInfinite()
+                        || value > 65504


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

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?

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.

jpountz · 2017-08-07T12:50:13Z

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

+                if (
+                    value.isNaN() || value.isInfinite()
+                        || value > 65504
+                        || !Float.isFinite(HalfFloatPoint.sortableShortToHalfFloat(HalfFloatPoint.halfFloatToSortableShort(value)))


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

jpountz · 2017-08-07T12:51:07Z

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

@@ -231,22 +244,39 @@ Query rangeQuery(String field, Object lowerTerm, Object upperTerm,
                }
                return fields;
            }
+
+            private void validateParsed(Float value) {


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

jpountz · 2017-08-07T12:51:35Z

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

@@ -162,12 +162,25 @@ public TypeParser(NumberType type) {
        HALF_FLOAT("half_float", NumericType.HALF_FLOAT) {
            @Override
            Float parse(Object value, boolean coerce) {
-                return (Float) FLOAT.parse(value, false);
+                final Float result;


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

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

jpountz · 2017-08-07T12:51:57Z

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

        },
        FLOAT("float", NumericType.FLOAT) {
            @Override
            Float parse(Object value, boolean coerce) {
+                final Float result;


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

jpountz · 2017-08-07T12:52:47Z

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

@@ -308,16 +338,26 @@ Query rangeQuery(String field, Object lowerTerm, Object upperTerm,
                }
                return fields;
            }
+
+            private void validateParsed(Float value) {


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

jpountz · 2017-08-07T12:53:13Z

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

        },
        DOUBLE("double", NumericType.DOUBLE) {
            @Override
            Double parse(Object value, boolean coerce) {
-                return objectToDouble(value);
+                Double parsed = objectToDouble(value);


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

jpountz · 2017-08-07T12:53:34Z

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

@@ -379,6 +419,12 @@ Query rangeQuery(String field, Object lowerTerm, Object upperTerm,
                }
                return fields;
            }
+
+            private void validateParsed(Double value) {


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

jpountz · 2017-08-07T12:56:04Z

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

+            } catch (IllegalArgumentException e) {
+                assertThat("Incorrect error message for [" + item.type + "] with value [" + item.value + "]",
+                    e.getMessage(), containsString(item.message));
+            }


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

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.

fair enough

jpountz · 2017-08-07T12:57:38Z

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

+
+            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"),


can you test negative values too?

…of_range_numbers

2) no redudant checks in half_float validation 3) tests with negative values for half_float/float/double

fred84 · 2017-08-08T07:00:38Z

@jpountz I updated PR. Please take a look.

jpountz

LGTM. Thanks @fred84 !

jpountz · 2017-08-08T08:04:33Z

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

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


this helper method does not look very useful

colings86 · 2017-08-08T08:57:58Z

jenkins please test this

* validate half float values * test upper bound for numeric mapper * test for upper bound for float, double and half_float * more tests on NaN and Infinity for NumberFieldMapper * fix checkstyle errors * minor renaming * comments for disabled test * tests for byte/short/integer/long removed and will be added in separate PR * remove unused import * Fix scaledfloat out of range validation message * 1) delayed autoboxing in numbertype.parse(...) 2) no redudant checks in half_float validation 3) tests with negative values for half_float/float/double

colings86 · 2017-08-09T11:47:18Z

@fred84 thanks for the PR, its now merged and backported to 6.x and 6.0.

fred84 · 2017-08-09T12:33:22Z

@colings86 @jpountz Thanks for review!

Since elastic#25826 we reject infinite values for float, double and half_float datatypes. This change adds this restriction to the documentation for the supported datatypes. Closes elastic#27653

Since #25826 we reject infinite values for float, double and half_float datatypes. This change adds this restriction to the documentation for the supported datatypes. Closes #27653

fred84 added 5 commits July 12, 2017 09:47

validate half float values

2d0e5c1

Merge branch 'master' into 25534_reject_out_of_range_numbers

c0fff6f

test upper bound for numeric mapper

6e0a6ea

merge master

7d4f315

test for upper bound for float, double and half_float

d1ebd6f

fred84 changed the title ~~25534 reject out of range numbers~~ Reject out of range numbers Jul 21, 2017

fred84 changed the title ~~Reject out of range numbers~~ Reject out of range numbers for float, double and half_float Jul 21, 2017

colings86 requested changes Jul 21, 2017

View reviewed changes

rjernst reviewed Jul 21, 2017

View reviewed changes

fred84 added 5 commits July 22, 2017 06:08

Merge branch 'master' into 25533_reject_out_of_range_numbers

1358fed

more tests on NaN and Infinity for NumberFieldMapper

44983d8

Merge branch 'master' into 25534_reject_out_of_range_numbers

d072e69

fix checkstyle errors

ecf3424

minor renaming

b5d231d

colings86 requested changes Jul 26, 2017

View reviewed changes

fred84 added 3 commits July 27, 2017 15:09

resolve merge conflict and cleanup NumberFieldMapper out of range tests

187982a

Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…

d236158

…of_range_numbers

comments for disabled test

7423c4d

fred84 added 3 commits July 30, 2017 07:08

Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…

8e88c9d

…of_range_numbers

tests for byte/short/integer/long removed and will be added in separa…

11ce7c8

…te PR

remove unused import

3902911

colings86 added v6.0.0 v6.1.0 labels Aug 7, 2017

jpountz reviewed Aug 7, 2017

View reviewed changes

fred84 added 2 commits August 8, 2017 09:29

Merge remote-tracking branch 'upstream/master' into 25534_reject_out_…

b578b5a

…of_range_numbers

1) delayed autoboxing in numbertype.parse(...)

3c666a9

2) no redudant checks in half_float validation 3) tests with negative values for half_float/float/double

jpountz approved these changes Aug 8, 2017

View reviewed changes

colings86 merged commit d8ff6e9 into elastic:master Aug 9, 2017

colings86 added 6.0.0-beta2 v6.0.0-beta2 and removed v6.0.0 6.0.0-beta2 labels Aug 24, 2017

colings86 mentioned this pull request Sep 26, 2017

Histogram aggregation fails on NaN #26787

Closed

lcawl removed the v6.1.0 label Dec 12, 2017

This was referenced Jan 16, 2018

[Docs] Clarify numeric datatype ranges #28240

Merged

Reject out of range numeric values at index time #25534

Closed

fred84 deleted the 25534_reject_out_of_range_numbers branch January 16, 2018 11:18

markharwood mentioned this pull request Mar 16, 2018

Elasticsearch client node crashing with java.lang.StackOverflowError when running percentile calculations for long data sets #23003

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

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

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

Uh oh!

Conversation

fred84 commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 21, 2017

Uh oh!

colings86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented Jul 21, 2017

Uh oh!

colings86 commented Jul 21, 2017

Uh oh!

colings86 commented Jul 21, 2017

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fred84 commented Jul 23, 2017

Uh oh!

colings86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fred84 commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fred84 commented Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colings86 commented Aug 1, 2017

Uh oh!

fred84 commented Aug 1, 2017

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fred84 commented Jul 21, 2017 •

edited

Loading

colings86 left a comment •

edited

Loading

fred84 commented Jul 27, 2017 •

edited

Loading

fred84 commented Jul 28, 2017 •

edited

Loading

fred84 Aug 7, 2017 •

edited

Loading