Skip to content

Conversation

@original-brownbear
Copy link
Contributor

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

Copy link
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.

Copy link
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

original-brownbear commented Nov 3, 2017

@jpountz added your assertions :)

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

dakrone commented Nov 3, 2017

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

@original-brownbear
Copy link
Contributor Author

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
Contributor Author

@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
*  #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
*  #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
* 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
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

scaled_float has precision problem

4 participants