Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.9
    • Component/s: modules/spatial
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Local Lucene (Geo-search) has been donated to the Lucene project, per https://issues.apache.org/jira/browse/INCUBATOR-77. This issue is to handle the Lucene portion of integration.

      See http://lucene.markmail.org/message/orzro22sqdj3wows?q=LocalLucene

      1. spatial.tar.gz
        26 kB
        Ryan McKinley
      2. spatial.zip
        1.32 MB
        Ryan McKinley
      3. spatial-lucene.zip
        813 kB
        patrick o'leary

        Issue Links

          Activity

          Hide
          Grant Ingersoll added a comment -

          NOTE: This code cannot be released until the LGPL dependencies are removed.

          Show
          Grant Ingersoll added a comment - NOTE: This code cannot be released until the LGPL dependencies are removed.
          Hide
          Ryan McKinley added a comment -

          I'm in the process of removing the LGPL dependencies now....

          What are the thoughts on where this should live once things are squared away?

          Currently, the package names are:
          com.pjaol.search.*
          com.mapquest.spatialbase;
          com.mapquest.localsolr;

          Where do we want them to live in lucene?

          org.apache.lucene.spatial
          org.apache.solr.spatial

          perhaps "geo", "gis", "geosearch"

          I lean towards "spatial"

          Show
          Ryan McKinley added a comment - I'm in the process of removing the LGPL dependencies now.... What are the thoughts on where this should live once things are squared away? Currently, the package names are: com.pjaol.search.* com.mapquest.spatialbase; com.mapquest.localsolr; Where do we want them to live in lucene? org.apache.lucene.spatial org.apache.solr.spatial perhaps "geo", "gis", "geosearch" I lean towards "spatial"
          Hide
          Ryan McKinley added a comment -

          locallucene has some java 1.5 code in it – enums and a few iterators.

          What is the policy with java 1.5 for contribs?

          We could probably make it 1.4 compatible, but why fight the future?!

          Show
          Ryan McKinley added a comment - locallucene has some java 1.5 code in it – enums and a few iterators. What is the policy with java 1.5 for contribs? We could probably make it 1.4 compatible, but why fight the future?!
          Hide
          Ryan McKinley added a comment -

          How do lucene contribs usually do logging?

          My understanding is that lucene core does not do logging... can we do SLF4J logging with this? That would fit nicely with SOLR-560

          Show
          Ryan McKinley added a comment - How do lucene contribs usually do logging? My understanding is that lucene core does not do logging... can we do SLF4J logging with this? That would fit nicely with SOLR-560
          Hide
          Ryan McKinley added a comment -

          Ok, I've got this compiling and running without the LGPL libraries/files

          I moved mqspatialbase into locallucene and use the mqspatialbase math (LatLng.arcDistance( LatLng )) rather then the stuff from LatLonTrig.java

          What are the next steps?

          If we add a "spatial" contrib to lucene now, can it be excluded from 2.4?

          Show
          Ryan McKinley added a comment - Ok, I've got this compiling and running without the LGPL libraries/files I moved mqspatialbase into locallucene and use the mqspatialbase math (LatLng.arcDistance( LatLng )) rather then the stuff from LatLonTrig.java What are the next steps? If we add a "spatial" contrib to lucene now, can it be excluded from 2.4?
          Hide
          Grant Ingersoll added a comment -

          I think we can wait until after 2.4, which should be soon. Mike said he was going to branch today anyway, so...

          I think we could put in a contrib in 2.9 if we wanted to.

          As for JDK 1.5, we allow new contribs to be 1.5.

          Show
          Grant Ingersoll added a comment - I think we can wait until after 2.4, which should be soon. Mike said he was going to branch today anyway, so... I think we could put in a contrib in 2.9 if we wanted to. As for JDK 1.5, we allow new contribs to be 1.5.
          Hide
          Grant Ingersoll added a comment -

          Logging: Lucene contribs handler their own logging. SLF4J sounds good to me

          As for package name, spatial works for me

          What are the next steps?

          Create a patch per the HowToContribute section on the wiki that hooks it into the build system like the other contribs. Make sure it has tests, docs, etc.

          Show
          Grant Ingersoll added a comment - Logging: Lucene contribs handler their own logging. SLF4J sounds good to me As for package name, spatial works for me What are the next steps? Create a patch per the HowToContribute section on the wiki that hooks it into the build system like the other contribs. Make sure it has tests, docs, etc.
          Hide
          Ryan McKinley added a comment -

          Here is a modified version of the grant with no LGPL dependencies and moved to org.apache.lucene.spatial.

          Since 2.4 has been branched, it would be nice to add this soon so better qualified folks can push on it easily

          Show
          Ryan McKinley added a comment - Here is a modified version of the grant with no LGPL dependencies and moved to org.apache.lucene.spatial. Since 2.4 has been branched, it would be nice to add this soon so better qualified folks can push on it easily
          Hide
          Ryan McKinley added a comment -

          I'm struggling to get two of the existing tests to pass... I don't think it is from my modifications since they don't pass on the original either.

          The two are:
          TestCartesian.java
          TestDistance.java

          originally, they get 0 results for everything. After I add: writer.commit(); in setup, I get more results, but still not matching the existing tests expectaions.

          For TestCartesian, I get:
          Distances should be 10 14
          Results should be 6 7

          For TestDistanc I get:
          Distance Filter filtered: 7
          Results: 7
          (it expects 6 for both...)

          Any ideas?

          Show
          Ryan McKinley added a comment - I'm struggling to get two of the existing tests to pass... I don't think it is from my modifications since they don't pass on the original either. The two are: TestCartesian.java TestDistance.java originally, they get 0 results for everything. After I add: writer.commit(); in setup, I get more results, but still not matching the existing tests expectaions. For TestCartesian, I get: Distances should be 10 14 Results should be 6 7 For TestDistanc I get: Distance Filter filtered: 7 Results: 7 (it expects 6 for both...) Any ideas?
          Hide
          Karl Wettin added a comment -

          I'm struggling to get two of the existing tests to pass... I don't think it is from my modifications since they don't pass on the original either.

          On my box the test fails with different results due to the writer not beeing comitted in setUp, giving me 0 results. After adding a commit it fails with the results you are reporting here.

          Is it possible that you are getting one sort of result in the original due to non committed writer and another error in this version due to your changes to the distance measurement? All points in the list are rather close to each other so very small changes to the algorithm might be the problem.

          I have a hard time tracing the code and I'm sort of hoping this might be the problem.

          Show
          Karl Wettin added a comment - I'm struggling to get two of the existing tests to pass... I don't think it is from my modifications since they don't pass on the original either. On my box the test fails with different results due to the writer not beeing comitted in setUp, giving me 0 results. After adding a commit it fails with the results you are reporting here. Is it possible that you are getting one sort of result in the original due to non committed writer and another error in this version due to your changes to the distance measurement? All points in the list are rather close to each other so very small changes to the algorithm might be the problem. I have a hard time tracing the code and I'm sort of hoping this might be the problem.
          Hide
          patrick o'leary added a comment -

          Yeah, the tests numbers are wrong, I'll put together better tests later today for it.
          It was brought to my attention recently when someone was trying lucene 2.4, I just didn't get around to resolving it.

          Show
          patrick o'leary added a comment - Yeah, the tests numbers are wrong, I'll put together better tests later today for it. It was brought to my attention recently when someone was trying lucene 2.4, I just didn't get around to resolving it.
          Hide
          Xibin Zeng added a comment -

          Hey Guys! Where is this now? Has it been checked in yet? I am asking as I am currently planning a feature and wanted to know if it is realistic to take advantage of it now. Any update is appreciated!

          Show
          Xibin Zeng added a comment - Hey Guys! Where is this now? Has it been checked in yet? I am asking as I am currently planning a feature and wanted to know if it is realistic to take advantage of it now. Any update is appreciated!
          Hide
          patrick o'leary added a comment -

          Latest version of local / spatial lucene with LGPL dependencies removed
          and working unit tests. The code's only dependency is on JUnit for tests during compilation.

          All the code's header's should be changed to Apache License as well.

          Show
          patrick o'leary added a comment - Latest version of local / spatial lucene with LGPL dependencies removed and working unit tests. The code's only dependency is on JUnit for tests during compilation. All the code's header's should be changed to Apache License as well.
          Hide
          Ryan McKinley added a comment -

          Looks great patrick!

          At quick glance, all the headers look fine – i'll run it through RAT to make sure though.

          Some notes about the changes here:

          • this includes a copy of NumberUtils from solr rather then depending on the library – I think that is the best option till NumberUtils can be moved into Lucene
          • this uses the package name: org.apache.lucene.spatial.tier

          I'll try to get the ball rolling to get this committed soon. Then we will have a base to work from and integrate with solr.

          Show
          Ryan McKinley added a comment - Looks great patrick! At quick glance, all the headers look fine – i'll run it through RAT to make sure though. Some notes about the changes here: this includes a copy of NumberUtils from solr rather then depending on the library – I think that is the best option till NumberUtils can be moved into Lucene this uses the package name: org.apache.lucene.spatial.tier I'll try to get the ball rolling to get this committed soon. Then we will have a base to work from and integrate with solr.
          Hide
          Ryan McKinley added a comment -

          Just noticed something...

          The TestCartesian creates a temp index like this:

          File luceneDirectory = new File("/tmp/example-lucene");
              if (! luceneDirectory.exists()) 
                luceneDirectory.mkdir();
              
              directory = FSDirectory.getDirectory(luceneDirectory);
          

          For simplicity, we should probably just use:

             directory = new RAMDirectory();
          
          Show
          Ryan McKinley added a comment - Just noticed something... The TestCartesian creates a temp index like this: File luceneDirectory = new File( "/tmp/example-lucene" ); if (! luceneDirectory.exists()) luceneDirectory.mkdir(); directory = FSDirectory.getDirectory(luceneDirectory); For simplicity, we should probably just use: directory = new RAMDirectory();
          Hide
          Ryan McKinley added a comment -

          here is an updated zip.

          This one can be dropped into:
          http://svn.apache.org/repos/asf/lucene/java/trunk/contrib

          and hooks into that build system.

          Now i really think this is ready to go....

          Show
          Ryan McKinley added a comment - here is an updated zip. This one can be dropped into: http://svn.apache.org/repos/asf/lucene/java/trunk/contrib and hooks into that build system. Now i really think this is ready to go....
          Hide
          Erik Hatcher added a comment -

          I've taken some quick peeks into the code, run the unit tests, nicely packaged and presented!

          A couple of thoughts:

          • Maybe the Filter's should be using the DocIdSet API rather than the BitSet deprecated stuff? We can refactor that after being committed I supposed, but not something we want to leave like that.
          • DistanceQuery is awkwardly named. It's not an (extends) Query.... it's a POJO with helpers. Maybe DistanceQueryFactory? (but it creates a Filter also)
          • CartesianPolyFilter is not a Filter (but CartesianShapeFilter is)

          I think this looks good enough to commit as well, just noting the above for cosmetic refactoring consideration after the code is in.

          Show
          Erik Hatcher added a comment - I've taken some quick peeks into the code, run the unit tests, nicely packaged and presented! A couple of thoughts: Maybe the Filter's should be using the DocIdSet API rather than the BitSet deprecated stuff? We can refactor that after being committed I supposed, but not something we want to leave like that. DistanceQuery is awkwardly named. It's not an (extends) Query.... it's a POJO with helpers. Maybe DistanceQueryFactory? (but it creates a Filter also) CartesianPolyFilter is not a Filter (but CartesianShapeFilter is) I think this looks good enough to commit as well, just noting the above for cosmetic refactoring consideration after the code is in.
          Hide
          Ryan McKinley added a comment -

          Agree – but these changes are best made in svn.

          More eyes will help out

          Show
          Ryan McKinley added a comment - Agree – but these changes are best made in svn. More eyes will help out
          Hide
          Ryan McKinley added a comment -

          added in #730067

          thanks everyone.

          Show
          Ryan McKinley added a comment - added in #730067 thanks everyone.
          Hide
          Earwin Burrfoot added a comment - - edited

          LatLonDistanceFilter.java:

          public BitSet bits(IndexReader reader) throws IOException {

          /* Create a BitSet to store the result */
          int maxdocs = reader.numDocs(); <----- probably reader.maxDoc ?
          BitSet bits = new BitSet(maxdocs);

          Show
          Earwin Burrfoot added a comment - - edited LatLonDistanceFilter.java: public BitSet bits(IndexReader reader) throws IOException { /* Create a BitSet to store the result */ int maxdocs = reader.numDocs(); <----- probably reader.maxDoc ? BitSet bits = new BitSet(maxdocs);
          Hide
          Grant Ingersoll added a comment -

          FWIW, you might find the discussion on SOLR-773 interesting.

          Show
          Grant Ingersoll added a comment - FWIW, you might find the discussion on SOLR-773 interesting.
          Hide
          Michael McCandless added a comment -

          I committed that fix; thanks Earwin!

          Show
          Michael McCandless added a comment - I committed that fix; thanks Earwin!

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Grant Ingersoll
            • Votes:
              1 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development