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

remove epsilon-based testing from lucene/spatial

    Details

    • Type: Test
    • 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, the random tests here allow a TOLERANCE and will fail if the error exceeds. But this is not fun to debug! It also keeps the door wide open for bugs to creep in.

      Alternatively, we can rework the tests like we did for sandbox/ points. This means the test is aware of the index-time quantization and so it can demand exact answers.

      Its more difficult at first, because even floating point error can cause a failure. It requires us to maybe work through corner cases/rework optimizations. If any epsilons must be added, they can be added to the optimizations themselves (e.g. bounding box) instead of the user's result.

      1. LUCENE-7127.patch
        65 kB
        Robert Muir
      2. LUCENE-7127.patch
        43 kB
        Robert Muir
      3. LUCENE-7127.patch
        43 kB
        Robert Muir
      4. LUCENE-7127.patch
        23 kB
        Robert Muir

        Activity

        Hide
        rcmuir Robert Muir added a comment -

        Here's my patch. Now GeoPoint and LatLonPoint are tested the exact same way (they are already generally returning exact same answers on any reasonable dataset, modulo bugs).

        Tests generally pass but there are sporatic corner case failures: e.g.

           [junit4] Started J0 PID(16923@localhost).
           [junit4] Suite: org.apache.lucene.spatial.geopoint.search.TestLegacyGeoPointQuery
           [junit4]   1> T1: id=1623 should not match but did
           [junit4]   1>   small=false query=GeoPointDistanceQuery: field=point: Center: [-78.95940156653523,-61.08293957076967] Distance: 5705904.858922883 meters] docID=1623
           [junit4]   1>   lat=-67.50789809972048 lon=95.3347154147923
           [junit4]   1>   deleted?=false
           [junit4]   1>   docID=1623 centerLon=-78.95940156653523 centerLat=-61.08293957076967 pointLon=95.3347154147923 pointLat=-67.50789809972048 distanceMeters=5715359.332616319 vs radiusMeters=5705904.858922883
           [junit4]   2> ??? 22, 2016 7:36:32 ? com.carrotsearch.randomizedtesting.RandomizedRunner$QueueUncaughtExceptionsHandler uncaughtException
           [junit4]   2> WARNING: Uncaught exception in thread: Thread[T1,5,TGRP-TestLegacyGeoPointQuery]
           [junit4]   2> java.lang.AssertionError: wrong hit (first of possibly more)
           [junit4]   2> 	at __randomizedtesting.SeedInfo.seed([2E99F05640D677C6]:0)
           [junit4]   2> 	at org.junit.Assert.fail(Assert.java:93)
           [junit4]   2> 	at org.apache.lucene.spatial.util.BaseGeoPointTestCase$VerifyHits.test(BaseGeoPointTestCase.java:553)
           [junit4]   2> 	at org.apache.lucene.spatial.util.BaseGeoPointTestCase$2._run(BaseGeoPointTestCase.java:772)
           [junit4]   2> 	at org.apache.lucene.spatial.util.BaseGeoPointTestCase$2.run(BaseGeoPointTestCase.java:637)
           [junit4]   2> 
           [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestLegacyGeoPointQuery -Dtests.method=testRandomWithThreads -Dtests.seed=2E99F05640D677C6 -Dtests.locale=ar-EG -Dtests.timezone=Antarctica/Syowa -Dtests.asserts=true -Dtests.file.encoding=US-ASCII
        

        I will start to try to debug through some of these, see if i can get things happy.

        Show
        rcmuir Robert Muir added a comment - Here's my patch. Now GeoPoint and LatLonPoint are tested the exact same way (they are already generally returning exact same answers on any reasonable dataset, modulo bugs). Tests generally pass but there are sporatic corner case failures: e.g. [junit4] Started J0 PID(16923@localhost). [junit4] Suite: org.apache.lucene.spatial.geopoint.search.TestLegacyGeoPointQuery [junit4] 1> T1: id=1623 should not match but did [junit4] 1> small=false query=GeoPointDistanceQuery: field=point: Center: [-78.95940156653523,-61.08293957076967] Distance: 5705904.858922883 meters] docID=1623 [junit4] 1> lat=-67.50789809972048 lon=95.3347154147923 [junit4] 1> deleted?=false [junit4] 1> docID=1623 centerLon=-78.95940156653523 centerLat=-61.08293957076967 pointLon=95.3347154147923 pointLat=-67.50789809972048 distanceMeters=5715359.332616319 vs radiusMeters=5705904.858922883 [junit4] 2> ??? 22, 2016 7:36:32 ? com.carrotsearch.randomizedtesting.RandomizedRunner$QueueUncaughtExceptionsHandler uncaughtException [junit4] 2> WARNING: Uncaught exception in thread: Thread[T1,5,TGRP-TestLegacyGeoPointQuery] [junit4] 2> java.lang.AssertionError: wrong hit (first of possibly more) [junit4] 2> at __randomizedtesting.SeedInfo.seed([2E99F05640D677C6]:0) [junit4] 2> at org.junit.Assert.fail(Assert.java:93) [junit4] 2> at org.apache.lucene.spatial.util.BaseGeoPointTestCase$VerifyHits.test(BaseGeoPointTestCase.java:553) [junit4] 2> at org.apache.lucene.spatial.util.BaseGeoPointTestCase$2._run(BaseGeoPointTestCase.java:772) [junit4] 2> at org.apache.lucene.spatial.util.BaseGeoPointTestCase$2.run(BaseGeoPointTestCase.java:637) [junit4] 2> [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestLegacyGeoPointQuery -Dtests.method=testRandomWithThreads -Dtests.seed=2E99F05640D677C6 -Dtests.locale=ar-EG -Dtests.timezone=Antarctica/Syowa -Dtests.asserts=true -Dtests.file.encoding=US-ASCII I will start to try to debug through some of these, see if i can get things happy.
        Hide
        rcmuir Robert Muir added a comment -

        Here is more iteration on the tests. I tried to make the multithreaded test reproduce better by using a random for each thread (i still have reproducibility issues when it fails).

        Since there are some issues around distances, I also moved the random distance tests from TestLatLonPointDistanceQuery here into the base class.

        With points this test always takes ~ 1s, but I got a hang. I'm not sure its something like simpletext since from the stack it seems like something else is happening, maybe a bug in the TermsEnum? Another thing to investigate.

           [junit4] "TEST-TestLegacyGeoPointQuery.testRandomDistance-seed#[8AC41F463F800FFE]" #13 prio=5 os_pri
           [junit4] o=0 tid=0x00007f038006a800 nid=0x52cb runnable [0x00007f03a733e000]
           [junit4]    java.lang.Thread.State: RUNNABLE
           [junit4] 	at org.apache.lucene.spatial.util.GeoRelationUtils.rectCrossesCircle(GeoRelationUtils.java:435)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointDistanceQueryImpl$GeoPointRadiusCellComparator.cellCrosses(GeoPointDistanceQueryImpl.java:57)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointDistanceQueryImpl$GeoPointRadiusCellComparator.cellIntersectsShape(GeoPointDistanceQueryImpl.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:91)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNu
           [junit4] mericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNum
           [junit4] ericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointN
           [junit4] umericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.<init>(GeoPointNumericTermsEnum.java:50)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointTermsEnum.newInstance(GeoPointTermsEnum.java:52)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointMultiTermQuery.getTermsEnum(GeoPointMultiTermQuery.java:94)
           [junit4] 	at org.apache.lucene.spatial.geopoint.search.GeoPointTermQueryConstantScoreWrapper$1.scorer(GeoPointTermQueryConstantScoreWrapper.java:94)
           [junit4] 	at org.apache.lucene.search.Weight.bulkScorer(Weight.java:135)
           [junit4] 	at org.apache.lucene.search.LRUQueryCache$CachingWrapperWeight.bulkScorer(LRUQueryCache.java:644)
           [junit4] 	at org.apache.lucene.search.AssertingWeight.bulkScorer(AssertingWeight.java:68)
           [junit4] 	at org.apache.lucene.search.AssertingWeight.bulkScorer(AssertingWeight.java:68)
           [junit4] 	at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:666)
           [junit4] 	at org.apache.lucene.search.AssertingIndexSearcher.search(AssertingIndexSearcher.java:91)
           [junit4] 	at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:473)
           [junit4] 	at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:592)
           [junit4] 	at org.apache.lucene.search.IndexSearcher.searchAfter(IndexSearcher.java:577)
           [junit4] 	at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:504)
           [junit4] 	at org.apache.lucene.spatial.util.BaseGeoPointTestCase.doRandomDistanceTest(BaseGeoPointTestCase.java:923)
           [junit4] 	at org.apache.lucene.spatial.util.BaseGeoPointTestCas
           [junit4] e.testRandomDistance(BaseGeoPointTestCase.java:855)
        
        Show
        rcmuir Robert Muir added a comment - Here is more iteration on the tests. I tried to make the multithreaded test reproduce better by using a random for each thread (i still have reproducibility issues when it fails). Since there are some issues around distances, I also moved the random distance tests from TestLatLonPointDistanceQuery here into the base class. With points this test always takes ~ 1s, but I got a hang. I'm not sure its something like simpletext since from the stack it seems like something else is happening, maybe a bug in the TermsEnum? Another thing to investigate. [junit4] "TEST-TestLegacyGeoPointQuery.testRandomDistance-seed#[8AC41F463F800FFE]" #13 prio=5 os_pri [junit4] o=0 tid=0x00007f038006a800 nid=0x52cb runnable [0x00007f03a733e000] [junit4] java.lang.Thread.State: RUNNABLE [junit4] at org.apache.lucene.spatial.util.GeoRelationUtils.rectCrossesCircle(GeoRelationUtils.java:435) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointDistanceQueryImpl$GeoPointRadiusCellComparator.cellCrosses(GeoPointDistanceQueryImpl.java:57) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointDistanceQueryImpl$GeoPointRadiusCellComparator.cellIntersectsShape(GeoPointDistanceQueryImpl.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:91) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNu [junit4] mericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNum [junit4] ericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointN [junit4] umericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:69) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.relateAndRecurse(GeoPointNumericTermsEnum.java:100) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.computeRange(GeoPointNumericTermsEnum.java:70) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointNumericTermsEnum.<init>(GeoPointNumericTermsEnum.java:50) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointTermsEnum.newInstance(GeoPointTermsEnum.java:52) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointMultiTermQuery.getTermsEnum(GeoPointMultiTermQuery.java:94) [junit4] at org.apache.lucene.spatial.geopoint.search.GeoPointTermQueryConstantScoreWrapper$1.scorer(GeoPointTermQueryConstantScoreWrapper.java:94) [junit4] at org.apache.lucene.search.Weight.bulkScorer(Weight.java:135) [junit4] at org.apache.lucene.search.LRUQueryCache$CachingWrapperWeight.bulkScorer(LRUQueryCache.java:644) [junit4] at org.apache.lucene.search.AssertingWeight.bulkScorer(AssertingWeight.java:68) [junit4] at org.apache.lucene.search.AssertingWeight.bulkScorer(AssertingWeight.java:68) [junit4] at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:666) [junit4] at org.apache.lucene.search.AssertingIndexSearcher.search(AssertingIndexSearcher.java:91) [junit4] at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:473) [junit4] at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:592) [junit4] at org.apache.lucene.search.IndexSearcher.searchAfter(IndexSearcher.java:577) [junit4] at org.apache.lucene.search.IndexSearcher.search(IndexSearcher.java:504) [junit4] at org.apache.lucene.spatial.util.BaseGeoPointTestCase.doRandomDistanceTest(BaseGeoPointTestCase.java:923) [junit4] at org.apache.lucene.spatial.util.BaseGeoPointTestCas [junit4] e.testRandomDistance(BaseGeoPointTestCase.java:855)
        Hide
        rcmuir Robert Muir added a comment -

        Just adding one more print to the new distance test to make it more debuggable.

        It shows a good fail like this which reproduces easily:

           [junit4] Started J0 PID(21772@localhost).
           [junit4] Suite: org.apache.lucene.spatial.geopoint.search.TestGeoPointQuery
           [junit4]   1> center: (-2.6859204112252826,-126.5501794954457), radius=1.933955591025525E7
           [junit4]   1> 0: (-37.8248053137213,30.965997129678726), distance=1.4967104936764924E7
           [junit4]   1> 1: (-88.12955391593277,-49.35365941375494), distance=9673775.807110526
           [junit4]   1> 2: (64.47744741104543,-10.247774831950665), distance=1.1518639744313335E7
           [junit4]   1> 3: (-19.651501905173063,16.937049888074398), distance=1.5335235632819101E7
           [junit4]   1> 4: (38.89845754019916,163.5567674227059), distance=8487195.64357184
           [junit4]   1> 5: (-2.730920696631074,-125.4338058270514), distance=124236.35394072857
           [junit4]   1> 6: (-9.463954903185368,-119.37913874164224), distance=1094829.0184535114
           [junit4]   1> 7: (2.784616006538272,58.78787197172642), distance=1.9443855071787946E7
           [junit4]   1> 8: (-76.14117253571749,151.79019374772906), distance=9506656.192332761
           [junit4]   1> 9: (66.5075888670981,1.8173518404364586), distance=1.1896270113358853E7
           [junit4]   2> NOTE: reproduce with: ant test  -Dtestcase=TestGeoPointQuery -Dtests.method=testRandomDistance -Dtests.seed=BFDD43F3DB3D2FDC -Dtests.locale=zh -Dtests.timezone=Pacific/Pago_Pago -Dtests.asserts=true -Dtests.file.encoding=UTF-8
           [junit4] FAILURE 12.2s | TestGeoPointQuery.testRandomDistance <<<
           [junit4]    > Throwable #1: java.lang.AssertionError: expected:<{0, 1, 2, 3, 4, 5, 6, 8, 9}> but was:<{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}>
        

        So document 7 is really too far away, but gets wrongly included in the result. I can probably debug it from here...

        Show
        rcmuir Robert Muir added a comment - Just adding one more print to the new distance test to make it more debuggable. It shows a good fail like this which reproduces easily: [junit4] Started J0 PID(21772@localhost). [junit4] Suite: org.apache.lucene.spatial.geopoint.search.TestGeoPointQuery [junit4] 1> center: (-2.6859204112252826,-126.5501794954457), radius=1.933955591025525E7 [junit4] 1> 0: (-37.8248053137213,30.965997129678726), distance=1.4967104936764924E7 [junit4] 1> 1: (-88.12955391593277,-49.35365941375494), distance=9673775.807110526 [junit4] 1> 2: (64.47744741104543,-10.247774831950665), distance=1.1518639744313335E7 [junit4] 1> 3: (-19.651501905173063,16.937049888074398), distance=1.5335235632819101E7 [junit4] 1> 4: (38.89845754019916,163.5567674227059), distance=8487195.64357184 [junit4] 1> 5: (-2.730920696631074,-125.4338058270514), distance=124236.35394072857 [junit4] 1> 6: (-9.463954903185368,-119.37913874164224), distance=1094829.0184535114 [junit4] 1> 7: (2.784616006538272,58.78787197172642), distance=1.9443855071787946E7 [junit4] 1> 8: (-76.14117253571749,151.79019374772906), distance=9506656.192332761 [junit4] 1> 9: (66.5075888670981,1.8173518404364586), distance=1.1896270113358853E7 [junit4] 2> NOTE: reproduce with: ant test -Dtestcase=TestGeoPointQuery -Dtests.method=testRandomDistance -Dtests.seed=BFDD43F3DB3D2FDC -Dtests.locale=zh -Dtests.timezone=Pacific/Pago_Pago -Dtests.asserts=true -Dtests.file.encoding=UTF-8 [junit4] FAILURE 12.2s | TestGeoPointQuery.testRandomDistance <<< [junit4] > Throwable #1: java.lang.AssertionError: expected:<{0, 1, 2, 3, 4, 5, 6, 8, 9}> but was:<{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}> So document 7 is really too far away, but gets wrongly included in the result. I can probably debug it from here...
        Hide
        rcmuir Robert Muir added a comment -

        The "fix" for that failure can look like this, the optimization is just not working:

        --- a/lucene/spatial/src/java/org/apache/lucene/spatial/geopoint/search/GeoPointDistanceQueryImpl.java
        +++ b/lucene/spatial/src/java/org/apache/lucene/spatial/geopoint/search/GeoPointDistanceQueryImpl.java
        @@ -60,7 +60,7 @@ final class GeoPointDistanceQueryImpl extends GeoPointInBBoxQueryImpl {
         
             @Override
             protected boolean cellWithin(final double minLon, final double minLat, final double maxLon, final double maxLat) {
        -      return GeoRelationUtils.rectWithinCircle(minLon, minLat, maxLon, maxLat,
        +      return false && GeoRelationUtils.rectWithinCircle(minLon, minLat, maxLon, maxLat,
                   centerLon, distanceQuery.centerLat, distanceQuery.radiusMeters, true);
             }
        

        I also noticed if i just unconditionally returned true for cellCrosses the perf issues with testing go away and its just as fast as points. So I think these problems boil down to the same struggles i encountered when i tried to use them to add distance query to LatLonPoint?

        A real/viable/alernative fix (which i can look at tomorrow), would be to just port the logic over from LatLonPointQuery for these? We can also port over its optimizations too to compensate. It might even end out faster.

        Show
        rcmuir Robert Muir added a comment - The "fix" for that failure can look like this, the optimization is just not working: --- a/lucene/spatial/src/java/org/apache/lucene/spatial/geopoint/search/GeoPointDistanceQueryImpl.java +++ b/lucene/spatial/src/java/org/apache/lucene/spatial/geopoint/search/GeoPointDistanceQueryImpl.java @@ -60,7 +60,7 @@ final class GeoPointDistanceQueryImpl extends GeoPointInBBoxQueryImpl { @Override protected boolean cellWithin(final double minLon, final double minLat, final double maxLon, final double maxLat) { - return GeoRelationUtils.rectWithinCircle(minLon, minLat, maxLon, maxLat, + return false && GeoRelationUtils.rectWithinCircle(minLon, minLat, maxLon, maxLat, centerLon, distanceQuery.centerLat, distanceQuery.radiusMeters, true); } I also noticed if i just unconditionally returned true for cellCrosses the perf issues with testing go away and its just as fast as points. So I think these problems boil down to the same struggles i encountered when i tried to use them to add distance query to LatLonPoint? A real/viable/alernative fix (which i can look at tomorrow), would be to just port the logic over from LatLonPointQuery for these? We can also port over its optimizations too to compensate. It might even end out faster.
        Hide
        dweiss Dawid Weiss added a comment -

        + final Random random = new Random(random().nextLong());
        Thread thread = new Thread() {

        Fyi, the test framework is consistent in assigning a new random instance (seed) to any new thread it observes - the seed will be the same regardless of how many or in which order you fork those new threads (random is a thread-local and is initialized based on the master seed for any new thread).

        Show
        dweiss Dawid Weiss added a comment - + final Random random = new Random(random().nextLong()); Thread thread = new Thread() { Fyi, the test framework is consistent in assigning a new random instance (seed) to any new thread it observes - the seed will be the same regardless of how many or in which order you fork those new threads (random is a thread-local and is initialized based on the master seed for any new thread).
        Hide
        rcmuir Robert Muir added a comment -

        Yeah but its not working: this multithreaded test doesnt reproduce correctly. I mean that it works like 75% of the time Something goes wrong in the test framework and it then it won't repro at all... so in desperation I go back to basics.

        I don't want to confuse the issue at hand here, what is happening on this issue is already difficult enough. We can fight the test framework separately elsewhere.

        I personally do not feel threads are warranted in the test at all. I will remove them.

        Show
        rcmuir Robert Muir added a comment - Yeah but its not working: this multithreaded test doesnt reproduce correctly. I mean that it works like 75% of the time Something goes wrong in the test framework and it then it won't repro at all... so in desperation I go back to basics. I don't want to confuse the issue at hand here, what is happening on this issue is already difficult enough. We can fight the test framework separately elsewhere. I personally do not feel threads are warranted in the test at all. I will remove them.
        Hide
        dweiss Dawid Weiss added a comment -

        Hmm. Whatever is wrong, I don't think it's the threading/randomness in the test framework - this is very well tested. If there is a race condition it should be somewhere higher up, perhaps there's state pollution across threads for some other reason. If you find out what's causing it let me know!

        Show
        dweiss Dawid Weiss added a comment - Hmm. Whatever is wrong, I don't think it's the threading/randomness in the test framework - this is very well tested. If there is a race condition it should be somewhere higher up, perhaps there's state pollution across threads for some other reason. If you find out what's causing it let me know!
        Hide
        rcmuir Robert Muir added a comment -

        I suspect it something happening in LuceneTestCase (some component being swapped in/some random.nextInt(4) in LuceneTestCase somewhere).

        Like i said, i'm gonna remove the threads completely. I don't have time for the distraction and we need to walk before we can try to run. Unfortunately I can't chase down whats wrong with multithreaded tests, and I really really don't want to confuse this issue. Its very important and already about at maximum possible confusion level

        Show
        rcmuir Robert Muir added a comment - I suspect it something happening in LuceneTestCase (some component being swapped in/some random.nextInt(4) in LuceneTestCase somewhere). Like i said, i'm gonna remove the threads completely. I don't have time for the distraction and we need to walk before we can try to run. Unfortunately I can't chase down whats wrong with multithreaded tests, and I really really don't want to confuse this issue. Its very important and already about at maximum possible confusion level
        Hide
        mikemccand Michael McCandless added a comment -

        +1 to just remove threads from the base test class.

        Show
        mikemccand Michael McCandless added a comment - +1 to just remove threads from the base test class.
        Hide
        rcmuir Robert Muir added a comment -

        Updated patch: all tests pass including the new ones.

        I used a simplified version of the same logic as LatLonPoint: the algorithm is sane. It also prevents the slowness in tests (and maybe in real queries?)

        I will try to figure out how to benchmark, to ensure there is no big regression or anything. If there is a minor drop in perf, I am happy to optimize it further, same as LatLonPoint, but at the moment I think we need to focus on correctness.

        Show
        rcmuir Robert Muir added a comment - Updated patch: all tests pass including the new ones. I used a simplified version of the same logic as LatLonPoint: the algorithm is sane. It also prevents the slowness in tests (and maybe in real queries?) I will try to figure out how to benchmark, to ensure there is no big regression or anything. If there is a minor drop in perf, I am happy to optimize it further, same as LatLonPoint, but at the moment I think we need to focus on correctness.
        Hide
        rcmuir Robert Muir added a comment -

        also i want to defer removal of the multithreading in the test and other things as much as possible. this issue has got enough going on as-is

        Show
        rcmuir Robert Muir added a comment - also i want to defer removal of the multithreading in the test and other things as much as possible. this issue has got enough going on as-is
        Hide
        mikemccand Michael McCandless added a comment -

        +1, this is a great simplification, and I love that the two queries (points and postings) use the same approach now.

        +1 to defer optimizing, threads removal, etc.

        Show
        mikemccand Michael McCandless added a comment - +1, this is a great simplification, and I love that the two queries (points and postings) use the same approach now. +1 to defer optimizing, threads removal, etc.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit ee1aca86435e501d02c23a93f480f076a8b72f34 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=ee1aca8 ]

        LUCENE-7127: remove epsilon-based testing from lucene/spatial, fix distance bugs.

        Show
        jira-bot ASF subversion and git services added a comment - Commit ee1aca86435e501d02c23a93f480f076a8b72f34 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=ee1aca8 ] LUCENE-7127 : remove epsilon-based testing from lucene/spatial, fix distance bugs.
        Hide
        mikemccand Michael McCandless added a comment -

        Yeah but its not working: this multithreaded test doesnt reproduce correctly.

        Rob suspected and I verified that it's the query cache that made the seeds not reproduce the threaded tests here.

        I disabled the query cache, then beasted an unrelated patch to a failing seed on the threads test and confirmed that it reliably reproduces now.

        It's because, depending on thread scheduling, and also because we seed all threads identically, different threads will hit a cache hit vs miss, leading to different random paths for each.

        Show
        mikemccand Michael McCandless added a comment - Yeah but its not working: this multithreaded test doesnt reproduce correctly. Rob suspected and I verified that it's the query cache that made the seeds not reproduce the threaded tests here. I disabled the query cache, then beasted an unrelated patch to a failing seed on the threads test and confirmed that it reliably reproduces now. It's because, depending on thread scheduling, and also because we seed all threads identically, different threads will hit a cache hit vs miss, leading to different random paths for each.
        Hide
        dweiss Dawid Weiss added a comment -

        Thanks for explanation Mike!

        Show
        dweiss Dawid Weiss added a comment - Thanks for explanation Mike!
        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S5 of LUCENE-7271


        FYI, not sure why gitbot didn't catch this, but the master commit was 1e5f74a02b8b04d9c9546410f5c49bf1d67abbbf

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271 FYI, not sure why gitbot didn't catch this, but the master commit was 1e5f74a02b8b04d9c9546410f5c49bf1d67abbbf

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development