Skip to content

Fixed rounding of bounds in scaled float comparison#27207

Merged
dakrone merged 2 commits intoelastic:masterfrom
original-brownbear:27189
Nov 3, 2017
Merged

Fixed rounding of bounds in scaled float comparison#27207
dakrone merged 2 commits intoelastic:masterfrom
original-brownbear:27189

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

@original-brownbear original-brownbear commented Nov 1, 2017

This fixes #27189 I think:

  • Fixed by switching the order of operations between nextDown, nextUp and multiplication by the scaling factor to reduce rounding errors. Now the error incurred by nextUp and nextDown doesn't propagate into the multiplication by the scaling factor, whereas without this change the error incurred when multiplying two double can cancel out the Math.nextDown step as seen in the example below 
Math.floor(Math.nextDown(0.1) * 100.0) = 10.0

while

Math.floor(Math.nextDown(0.1 * 100.0)) = 9.0

jpountz
jpountz approved these changes Nov 3, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I left a comment about testing more cases, but your change looks good to me.

...es/mapper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldTypeTests.java Outdated
Copy link
Copy Markdown
Contributor

@jpountz jpountz Nov 3, 2017

Choose a reason for hiding this comment

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

I expanded your assertions a bit if you are ok with them:

    public void testRoundsUpperBoundCorrectly() {
        ScaledFloatFieldMapper.ScaledFloatFieldType ft = new ScaledFloatFieldMapper.ScaledFloatFieldType();
        ft.setName("scaled_float");
        ft.setScalingFactor(100.0);
        Query scaledFloatQ = ft.rangeQuery(null, 0.1, true, false, null);
        assertEquals("scaled_float:[-9223372036854775808 TO 9]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(null, 0.1, true, true, null);
        assertEquals("scaled_float:[-9223372036854775808 TO 10]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(null, 0.095, true, false, null);
        assertEquals("scaled_float:[-9223372036854775808 TO 9]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(null, 0.095, true, true, null);
        assertEquals("scaled_float:[-9223372036854775808 TO 9]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(null, 0.105, true, false, null);
        assertEquals("scaled_float:[-9223372036854775808 TO 10]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(null, 0.105, true, true, null);
        assertEquals("scaled_float:[-9223372036854775808 TO 10]", scaledFloatQ.toString());
    }

    public void testRoundsLowerBoundCorrectly() {
        ScaledFloatFieldMapper.ScaledFloatFieldType ft = new ScaledFloatFieldMapper.ScaledFloatFieldType();
        ft.setName("scaled_float");
        ft.setScalingFactor(100.0);
        Query scaledFloatQ = ft.rangeQuery(-0.1, null, false, true, null);
        assertEquals("scaled_float:[-9 TO 9223372036854775807]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(-0.1, null, true, true, null);
        assertEquals("scaled_float:[-10 TO 9223372036854775807]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(-0.095, null, false, true, null);
        assertEquals("scaled_float:[-9 TO 9223372036854775807]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(-0.095, null, true, true, null);
        assertEquals("scaled_float:[-9 TO 9223372036854775807]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(-0.105, null, false, true, null);
        assertEquals("scaled_float:[-10 TO 9223372036854775807]", scaledFloatQ.toString());
        scaledFloatQ = ft.rangeQuery(-0.105, null, true, true, null);
        assertEquals("scaled_float:[-10 TO 9223372036854775807]", scaledFloatQ.toString());
    }

Copy link
Copy Markdown
Contributor Author

@original-brownbear original-brownbear Nov 3, 2017

Choose a reason for hiding this comment

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

@jpountz thanks for reviewing, I like adding some more assertions :) I'll just add those to this PR?

Copy link
Copy Markdown
Contributor

@jpountz jpountz Nov 3, 2017

Choose a reason for hiding this comment

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

Yes please!

@original-brownbear
Copy link
Copy Markdown
Contributor Author

original-brownbear commented Nov 3, 2017 •
edited
Loading

@jpountz added your assertions :)

@dakrone dakrone self-assigned this Nov 3, 2017
@dakrone
Copy link
Copy Markdown
Member

dakrone commented Nov 3, 2017

Thanks @original-brownbear, I'll wait for CI to finish and then merge this

@original-brownbear
Copy link
Copy Markdown
Contributor Author

original-brownbear commented Nov 3, 2017

Rebased this one just now since it failed the BWC tests here https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/5329/console

@original-brownbear
Copy link
Copy Markdown
Contributor Author

original-brownbear commented Nov 3, 2017

@dakrone now it's green :)

@dakrone dakrone merged commit 8f0f024 into elastic:master Nov 3, 2017
dakrone pushed a commit that referenced this pull request Nov 3, 2017
@original-brownbear @dakrone
#27189 Fixed rounding of bounds in scaled float comparison (#27207)
46b9ccf 
*  #27189 Fixed rounding of bounds in scaled float comparison

*  #27189 more assertions from CR
dakrone pushed a commit that referenced this pull request Nov 3, 2017
@original-brownbear @dakrone
#27189 Fixed rounding of bounds in scaled float comparison (#27207)
525c413 
*  #27189 Fixed rounding of bounds in scaled float comparison

*  #27189 more assertions from CR
@dakrone dakrone added :Search Foundations/Mapping Index mappings, including merging and defining field types >bug v6.0.1 v6.1.0 v7.0.0 labels Nov 3, 2017
@original-brownbear original-brownbear deleted the 27189 branch November 4, 2017 07:54
martijnvg added a commit that referenced this pull request Nov 4, 2017
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
e61e4b8 
* es/master:
  Fix snapshot getting stuck in INIT state (#27214)
  Add an example of dynamic field names (#27255)
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix InternalStatsTests
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
martijnvg added a commit that referenced this pull request Nov 4, 2017
@martijnvg
Merge branch '6.x' into ccr-6.x
3ed5aaa 
* 6.x:
  Add an example of dynamic field names (#27255)
  fixed checkstyle violation
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  [TEST] Fix failing exists query test
  test: Do not run old parent/child tests against a cluster with minimum version greater than 6.0.0
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  fix compil after backport
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
@clintongormley clintongormley changed the title #27189 Fixed rounding of bounds in scaled float comparison Fixed rounding of bounds in scaled float comparison Nov 6, 2017
@kyreddie kyreddie mentioned this pull request Aug 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jpountz jpountz jpountz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dakrone dakrone

Labels

>bug :Search Foundations/Mapping Index mappings, including merging and defining field types v6.0.1 v6.1.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

scaled_float has precision problem

4 participants

@original-brownbear @dakrone @jpountz @colings86