Lucene - Core
  1. Lucene - Core
  2. LUCENE-5790

broken compareTo in MutableValueDouble and MutableValueBool - affects grouping when exists==false

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.10, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      On the solr-user mailing list, Ebisawa & Alex both commented that they've noticed bugs in the grouping code when some documents don't have values in the grouping field.

      In Ebisawa's case, he tracked this down to what appears to be some bugs in the logic of the "compareSameType" method of some of the MutableValue implementations.

      Thread: https://mail-archives.apache.org/mod_mbox/lucene-solr-user/201406.mbox/%3C84f86fce4b8f42268048aecfb2806a8c@SIXPR04MB045.apcprd04.prod.outlook.com%3E

      1. LUCENE-5790.patch
        27 kB
        Hoss Man
      2. LUCENE-5790.patch
        24 kB
        Hoss Man
      3. LUCENE-5790.patch
        5 kB
        Hoss Man
      4. LUCENE-5790.patch
        3 kB
        Hoss Man

        Activity

        Hide
        Hoss Man added a comment -

        Starting point for a more comprehensive test patch of the MutableValue API (it doesn't seem to have any specific unit tests yet – just tests via Grouping)

        This is the most trivial way to demonstrate the failure...

        public void testDoubleTrivially() {
          MutableValueDouble xxx = new MutableValueDouble();
          xxx.exists = false;
          assertEquals(0, xxx.compareTo(xxx));
        }
        

        ...a mutable double that does not exist can't be compared to itself.

        Ebisawa observed simila problems in other MutableValue subclasses that we should also audit & test for.

        Show
        Hoss Man added a comment - Starting point for a more comprehensive test patch of the MutableValue API (it doesn't seem to have any specific unit tests yet – just tests via Grouping) This is the most trivial way to demonstrate the failure... public void testDoubleTrivially() { MutableValueDouble xxx = new MutableValueDouble(); xxx.exists = false; assertEquals(0, xxx.compareTo(xxx)); } ...a mutable double that does not exist can't be compared to itself. Ebisawa observed simila problems in other MutableValue subclasses that we should also audit & test for.
        Hide
        Yonik Seeley added a comment -

        Yep... this code is incorrect:

            MutableValueDouble b = (MutableValueDouble)other;
            int c = Double.compare(value, b.value);
            if (c != 0) return c;
            if (!exists) return -1;
            if (!b.exists) return 1;
            return 0;
        

        It should be:

            MutableValueDouble b = (MutableValueDouble)other;
            int c = Double.compare(value, b.value);
            if (c != 0) return c;
            if (exists == b.exists) return 0;
            return exists ? 1 : -1;
        

        I'm surprised the random grouping code didn't catch this. Perhaps the test didn't use any missing values?

        Show
        Yonik Seeley added a comment - Yep... this code is incorrect: MutableValueDouble b = (MutableValueDouble)other; int c = Double .compare(value, b.value); if (c != 0) return c; if (!exists) return -1; if (!b.exists) return 1; return 0; It should be: MutableValueDouble b = (MutableValueDouble)other; int c = Double .compare(value, b.value); if (c != 0) return c; if (exists == b.exists) return 0; return exists ? 1 : -1; I'm surprised the random grouping code didn't catch this. Perhaps the test didn't use any missing values?
        Hide
        Hoss Man added a comment - - edited

        It should be: ...

        Even that looks wrong to me based on the mutable contract as it exists – as things stand, it seems like the very first check must be if (!(exists == b.exists) ) return 0; ... because if the exist flag on both values is false, then we should not make any comparison of the values.

        Example: shouldn't this assert be valid?

        mvalXXX.value = 42;
        mvalYYY.value = 999;
        mvalXXX.exists = mvalYYY.exists = false;
        assertEquals(0, mvalXXX.compareTo(mvalYYY));
        

        EDIT: ment to have: if (!(exists || b.exists) ) return 0;

        Show
        Hoss Man added a comment - - edited It should be: ... Even that looks wrong to me based on the mutable contract as it exists – as things stand, it seems like the very first check must be if (!(exists == b.exists) ) return 0; ... because if the exist flag on both values is false, then we should not make any comparison of the values. Example: shouldn't this assert be valid? mvalXXX.value = 42; mvalYYY.value = 999; mvalXXX.exists = mvalYYY.exists = false ; assertEquals(0, mvalXXX.compareTo(mvalYYY)); EDIT : ment to have: if (!(exists || b.exists) ) return 0;
        Hide
        Hoss Man added a comment -

        updated description: forgot mail thread link when i filed issue

        Show
        Hoss Man added a comment - updated description: forgot mail thread link when i filed issue
        Hide
        Yonik Seeley added a comment -

        because if the exist flag on both values is false, then we should not make any comparison of the values.

        If you give free reign for anyone to set the values independently, yes.
        Value setters currently ensure that the default value (0 for numerics) is set if exists == false.

        Show
        Yonik Seeley added a comment - because if the exist flag on both values is false, then we should not make any comparison of the values. If you give free reign for anyone to set the values independently, yes. Value setters currently ensure that the default value (0 for numerics) is set if exists == false.
        Hide
        Hoss Man added a comment -

        If you give free reign for anyone to set the values independently, yes.

        that is, in fact, exactly what the API currently does.

        Value setters currently ensure that the default value (0 for numerics) is set if exists == false.

        if we're going to have that expectation on the callers it should be documented and there should be some asserts in the code to ensure it.


        Updated patch with yonik's suggested fix + some class level docs explaining the requirements on the callers + some assertions in key methods that it's pre-conditions aren't violated.

        If folks think this is a better way to go then accounting for the possibility that "value!=0" when "exists==false" in hashCode/equals/compareTo i'm fine with that – but we still need to audit the other impls & add tests for them as well.

        Show
        Hoss Man added a comment - If you give free reign for anyone to set the values independently, yes. that is, in fact, exactly what the API currently does. Value setters currently ensure that the default value (0 for numerics) is set if exists == false. if we're going to have that expectation on the callers it should be documented and there should be some asserts in the code to ensure it. Updated patch with yonik's suggested fix + some class level docs explaining the requirements on the callers + some assertions in key methods that it's pre-conditions aren't violated. If folks think this is a better way to go then accounting for the possibility that "value!=0" when "exists==false" in hashCode/equals/compareTo i'm fine with that – but we still need to audit the other impls & add tests for them as well.
        Hide
        Hoss Man added a comment -

        updated patch:

        • continues along assumption of the previous patch (that callers who set exist=false must reset value to default) per yonik's comments
        • adds class level javadocs explaining this expecation of the caller
        • adds additional tests of each type of MutableValue
        • in addition to yonik's MutableValueDouble fix from the previous patch, this also includes Ebisawa's MutableValueBool fix.
        • Also includes a randomized solr grouping test that heavily stresses docs with missing values in the grouping fields, and demonstrates both of the bugs Ebisawa mentioned in his email (w/o the fixes of course)

        I think this is ready to commit.

        Show
        Hoss Man added a comment - updated patch: continues along assumption of the previous patch (that callers who set exist=false must reset value to default) per yonik's comments adds class level javadocs explaining this expecation of the caller adds additional tests of each type of MutableValue in addition to yonik's MutableValueDouble fix from the previous patch, this also includes Ebisawa's MutableValueBool fix. Also includes a randomized solr grouping test that heavily stresses docs with missing values in the grouping fields, and demonstrates both of the bugs Ebisawa mentioned in his email (w/o the fixes of course) I think this is ready to commit.
        Hide
        Hoss Man added a comment -

        the last patch included the javadoc changes to all the MutableValue classes, but i had forgetten to copy the asserts from the early patch to MutableValueDouble into the rest of the classes this patch rectifies that.

        I'll commit once my test run finishes.

        Show
        Hoss Man added a comment - the last patch included the javadoc changes to all the MutableValue classes, but i had forgetten to copy the asserts from the early patch to MutableValueDouble into the rest of the classes this patch rectifies that. I'll commit once my test run finishes.
        Hide
        ASF subversion and git services added a comment -

        Commit 1608639 from hossman@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1608639 ]

        LUCENE-5790: Fix compareTo in MutableValueDouble and MutableValueBool, this caused incorrect results when grouping on fields with missing values

        Show
        ASF subversion and git services added a comment - Commit 1608639 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1608639 ] LUCENE-5790 : Fix compareTo in MutableValueDouble and MutableValueBool, this caused incorrect results when grouping on fields with missing values
        Hide
        ASF subversion and git services added a comment -

        Commit 1608640 from hossman@apache.org in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1608640 ]

        LUCENE-5790: Fix compareTo in MutableValueDouble and MutableValueBool, this caused incorrect results when grouping on fields with missing values (merge r1608639)

        Show
        ASF subversion and git services added a comment - Commit 1608640 from hossman@apache.org in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1608640 ] LUCENE-5790 : Fix compareTo in MutableValueDouble and MutableValueBool, this caused incorrect results when grouping on fields with missing values (merge r1608639)
        Hide
        Hoss Man added a comment -

        update summary

        Show
        Hoss Man added a comment - update summary
        Hide
        Hoss Man added a comment -

        Committed revision 1608639.
        Committed revision 1608640.

        thanks everybody.

        Show
        Hoss Man added a comment - Committed revision 1608639. Committed revision 1608640. thanks everybody.

          People

          • Assignee:
            Hoss Man
            Reporter:
            Hoss Man
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development