Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7166

fix quantization bugs in LatLonPoint and GeoPointField, remove test leniency

    Details

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

      Description

      Currently a few remaining tests (around newRectQuery) are lenient and quantize the query rectangles. This is masking several bugs:
      1. Both LatLonPoint and GeoPointField's bbox queries quantize their endpoints incorrectly at query-time, which can e.g. cause it to bring in false positive results
      2. Tests have always been lenient about this (either by using epsilons or incorrectly quantizing the query rectangles in tests), hiding the above.
      3. Both LatLonPoint and GeoPointField still have rounding issues at quantization. For very special values they do not always consistently round in one direction.
      4. Random encoding tests will never find the above issue, hiding it. This is because you need very special double values that the current stuff (e.g. -180 + 360.0 * random().nextDouble() will never find!).

        Activity

        Hide
        rcmuir Robert Muir added a comment -

        Attached is a patch:

        • fixes Geo random tests to not quantize query rectangles.
        • fixes testRectBoundariesAreInclusive to properly quantize its rectangle (for an exact inclusive test), and then test around that boundary with Math.nextUp/Math.nextDown
        • fixes LatLonPoint encoding tests to not just use e.g. nextLatitude/nextLongitude, but to also walk the double space with Math.nextUp/nextDown.
        • fixes LatLonPoint encoding to always correctly round down.
        • fixes LatLonPoint box generation code to correctly round minimum values so false positives are never brought in.
        • fixes GeoPoint bounding box impl to not quantize range endpoints at all, so false positives are never brought in.

        Note that GeoPoint's quantization is still inconsistent. Its not always rounding down. I don't think it makes sense to fix that here, instead it will be easier to just fix it with LUCENE-7165. Instead I fixed it by just removing its quantization. Its box query is quite different from LatLonPoint: its a two-phase confirm approach already, so it weeds out the false positives that way (once i removed the bogus quantization of these endpoints). GeoPoint cannot be changed to work like LatLonPoint's bounding box (which is more efficient), until its encoding is fixed to have consistent rounding. This is no change from how it works today, but the inconsistency does kinda suck.

        Show
        rcmuir Robert Muir added a comment - Attached is a patch: fixes Geo random tests to not quantize query rectangles. fixes testRectBoundariesAreInclusive to properly quantize its rectangle (for an exact inclusive test), and then test around that boundary with Math.nextUp/Math.nextDown fixes LatLonPoint encoding tests to not just use e.g. nextLatitude/nextLongitude, but to also walk the double space with Math.nextUp/nextDown. fixes LatLonPoint encoding to always correctly round down. fixes LatLonPoint box generation code to correctly round minimum values so false positives are never brought in. fixes GeoPoint bounding box impl to not quantize range endpoints at all, so false positives are never brought in. Note that GeoPoint's quantization is still inconsistent. Its not always rounding down. I don't think it makes sense to fix that here, instead it will be easier to just fix it with LUCENE-7165 . Instead I fixed it by just removing its quantization. Its box query is quite different from LatLonPoint: its a two-phase confirm approach already, so it weeds out the false positives that way (once i removed the bogus quantization of these endpoints). GeoPoint cannot be changed to work like LatLonPoint's bounding box (which is more efficient), until its encoding is fixed to have consistent rounding. This is no change from how it works today, but the inconsistency does kinda suck.
        Hide
        mikemccand Michael McCandless added a comment -

        +1, tricky

        Show
        mikemccand Michael McCandless added a comment - +1, tricky
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit f8ea8b855e43fc0a2fa434ab8c366de810047c8f in lucene-solr's branch refs/heads/master from Robert Muir
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f8ea8b8 ]

        LUCENE-7166: fix quantization bugs in LatLonPoint and GeoPointField, remove test leniency

        Squashed commit of the following:

        commit 83c0f9b6158495b8b3d7108059a23bdf38e0f7f3
        Author: Robert Muir <rmuir@apache.org>
        Date: Fri Apr 1 23:33:29 2016 -0400

        fix geopoint

        commit 97ebd2de516e61c236542fb2fb28e71cf6bdc403
        Author: Robert Muir <rmuir@apache.org>
        Date: Fri Apr 1 23:06:05 2016 -0400

        fix test and LatLonPoint encoding/quantization/box queries

        Show
        jira-bot ASF subversion and git services added a comment - Commit f8ea8b855e43fc0a2fa434ab8c366de810047c8f in lucene-solr's branch refs/heads/master from Robert Muir [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=f8ea8b8 ] LUCENE-7166 : fix quantization bugs in LatLonPoint and GeoPointField, remove test leniency Squashed commit of the following: commit 83c0f9b6158495b8b3d7108059a23bdf38e0f7f3 Author: Robert Muir <rmuir@apache.org> Date: Fri Apr 1 23:33:29 2016 -0400 fix geopoint commit 97ebd2de516e61c236542fb2fb28e71cf6bdc403 Author: Robert Muir <rmuir@apache.org> Date: Fri Apr 1 23:06:05 2016 -0400 fix test and LatLonPoint encoding/quantization/box queries
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 99d16feb7e5b7be70c7bf172c19f82c4df4e6b0e in lucene-solr's branch refs/heads/branch_6x from Robert Muir
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=99d16fe ]

        LUCENE-7166: fix quantization bugs in LatLonPoint and GeoPointField, remove test leniency

        Squashed commit of the following:

        commit 83c0f9b6158495b8b3d7108059a23bdf38e0f7f3
        Author: Robert Muir <rmuir@apache.org>
        Date: Fri Apr 1 23:33:29 2016 -0400

        fix geopoint

        commit 97ebd2de516e61c236542fb2fb28e71cf6bdc403
        Author: Robert Muir <rmuir@apache.org>
        Date: Fri Apr 1 23:06:05 2016 -0400

        fix test and LatLonPoint encoding/quantization/box queries

        Show
        jira-bot ASF subversion and git services added a comment - Commit 99d16feb7e5b7be70c7bf172c19f82c4df4e6b0e in lucene-solr's branch refs/heads/branch_6x from Robert Muir [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=99d16fe ] LUCENE-7166 : fix quantization bugs in LatLonPoint and GeoPointField, remove test leniency Squashed commit of the following: commit 83c0f9b6158495b8b3d7108059a23bdf38e0f7f3 Author: Robert Muir <rmuir@apache.org> Date: Fri Apr 1 23:33:29 2016 -0400 fix geopoint commit 97ebd2de516e61c236542fb2fb28e71cf6bdc403 Author: Robert Muir <rmuir@apache.org> Date: Fri Apr 1 23:06:05 2016 -0400 fix test and LatLonPoint encoding/quantization/box queries
        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S5 of LUCENE-7271

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271

          People

          • Assignee:
            Unassigned
            Reporter:
            rcmuir Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development