Lucene - Core
  1. Lucene - Core
  2. LUCENE-4937

sort order different in branch_4x than trunk

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.3
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I will buy a beer to whoever figures out why +0 sorts before -0 in branch_4x, but works correctly in trunk

      1. LUCENE-4937_test.patch
        3 kB
        Robert Muir
      2. LUCENE-4937.patch
        11 kB
        Uwe Schindler
      3. LUCENE-4937.patch
        7 kB
        Uwe Schindler
      4. SOLR-4723_test.patch
        1 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          here's my test.

          Show
          Robert Muir added a comment - here's my test.
          Hide
          Robert Muir added a comment -

          and this might be a lucene bug. i just pulled out a lot of hair trying to track down WTF so i need a break.

          Show
          Robert Muir added a comment - and this might be a lucene bug. i just pulled out a lot of hair trying to track down WTF so i need a break.
          Hide
          Robert Muir added a comment -

          It might be that trunk uses Double.compare (from a java7 commit?) and branch_4x does this:

          if (v1 > v2) {
            return 1;
          } else if (v1 < v2) {
            return -1;
          } else {
            return 0;
          }
          

          If so this is not right, because NumericUtils documents it is consistent with Double.compareTo and Float.compareTo, so something needs to be fixed.

          Show
          Robert Muir added a comment - It might be that trunk uses Double.compare (from a java7 commit?) and branch_4x does this: if (v1 > v2) { return 1; } else if (v1 < v2) { return -1; } else { return 0; } If so this is not right, because NumericUtils documents it is consistent with Double.compareTo and Float.compareTo, so something needs to be fixed.
          Hide
          Uwe Schindler added a comment -

          The pre-trunk code is wrong for comparing doubles/floats, as v1 > v2 does not work with +/-0 (its identical for operators <, >, ==). In Java 7, Double/Float.compare() compares the bits.

          The easiest way to solve the problem is using NumericUtils.doubleToSortableLong/floatToSortableInt and then compare the long/int. NumericUtils converts the floating point value to an integer, with fixing the sign to be comparable like a int (see javadocs).

          I think this is a corner case (like the crazy bugs if the score is NaN), so maybe we should only fix in 5.0 but leave 4.x and all previous lucene versions as they are.

          Show
          Uwe Schindler added a comment - The pre-trunk code is wrong for comparing doubles/floats, as v1 > v2 does not work with +/-0 (its identical for operators <, >, ==). In Java 7, Double/Float.compare() compares the bits. The easiest way to solve the problem is using NumericUtils.doubleToSortableLong/floatToSortableInt and then compare the long/int. NumericUtils converts the floating point value to an integer, with fixing the sign to be comparable like a int (see javadocs). I think this is a corner case (like the crazy bugs if the score is NaN), so maybe we should only fix in 5.0 but leave 4.x and all previous lucene versions as they are.
          Hide
          Uwe Schindler added a comment -

          We can fix the bug also in Java 6, because Float.compare(float,float) and Double.compare(double,double) are there since Java 1.4.

          It is only integer's compare which iss missing.

          So we should really use the correct Double/Float functions:

          Show
          Uwe Schindler added a comment - We can fix the bug also in Java 6, because Float.compare(float,float) and Double.compare(double,double) are there since Java 1.4. It is only integer's compare which iss missing. So we should really use the correct Double/Float functions: http://docs.oracle.com/javase/1.4.2/docs/api/java/lang/Double.html#compare(double, double) http://docs.oracle.com/javase/1.4.2/docs/api/java/lang/Float.html#compare(float, float)
          Hide
          Robert Muir added a comment -

          A documentation fix is fine by me... I just found it surprising since i couldnt reproduce from trunk.

          Show
          Robert Muir added a comment - A documentation fix is fine by me... I just found it surprising since i couldnt reproduce from trunk.
          Hide
          Uwe Schindler added a comment -

          See my comment, we can fix it. The static compare methods are available in since Java 1.4.

          Show
          Uwe Schindler added a comment - See my comment, we can fix it. The static compare methods are available in since Java 1.4.
          Hide
          Robert Muir added a comment -

          here's some tests for lucene (fieldcache and docvalues, both fail in 4.x)

          Show
          Robert Muir added a comment - here's some tests for lucene (fieldcache and docvalues, both fail in 4.x)
          Hide
          Uwe Schindler added a comment -

          Here the patch, what I did:

          • I merged Adriens patch for from trunk to 4.x
          • I reverted all Java 7 Integer.compare(),...
          Show
          Uwe Schindler added a comment - Here the patch, what I did: I merged Adriens patch for from trunk to 4.x I reverted all Java 7 Integer.compare(),...
          Hide
          Uwe Schindler added a comment -

          Patch including all tests on this issue.

          I will commit this to 4.x and then merge to trunk (the tests)

          Show
          Uwe Schindler added a comment - Patch including all tests on this issue. I will commit this to 4.x and then merge to trunk (the tests)
          Hide
          Robert Muir added a comment -

          Thanks Uwe: now i owe some beer!

          Show
          Robert Muir added a comment - Thanks Uwe: now i owe some beer!
          Hide
          Uwe Schindler added a comment -

          Looking forward to see you in Berlin!

          Show
          Uwe Schindler added a comment - Looking forward to see you in Berlin!
          Hide
          Uwe Schindler added a comment -

          I committed to 4.x branch and merged the tests to trunk, the other changes merged successfully as identical code now.

          Show
          Uwe Schindler added a comment - I committed to 4.x branch and merged the tests to trunk, the other changes merged successfully as identical code now.
          Hide
          Adrien Grand added a comment -

          Thanks Uwe!

          Show
          Adrien Grand added a comment - Thanks Uwe!
          Hide
          Uwe Schindler added a comment -

          Closed after release.

          Show
          Uwe Schindler added a comment - Closed after release.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development