Lucene - Core
  1. Lucene - Core
  2. LUCENE-5565

Remove String based encoding from SpatialPrefixTree/Cell API; just use bytes

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.8, 6.0
    • Component/s: modules/spatial
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      The SpatialPrefixTree/Cell API supports bytes and String encoding/decoding dually. I want to remove the String side to keep the API simpler. Included in this issue, I'd like to make some small refactorings to reduce assumptions the filters make of the underlying encoding such that future encodings can work a in more different ways with less impact on the filters.

      String encode/decode will exist for the Geohash one for now since GeohashUtils works off of Strings, but Quad could change more easily.

        Issue Links

          Activity

          Hide
          David Smiley added a comment -

          The attached patch:

          • removes String based token of Cell & SpatialPrefixTree. Geohash Cell still uses Strings though; Quad was improved to not.
          • renamed getTokenBytes() to getTokenBytesNoLeaf(), plus added getTokenBytes() that will have a leaf if the leaf is set.
          • instead of using a boolean leaf flag; we examine the encoding
          • Cell is no longer Comparable (wasn't needed)
          • The former inner class CellTokenStream is now its own class and modeled heavily off of NumericTokenStream.

          I'll do a performance test to ensure the performance is at least the same.

          Show
          David Smiley added a comment - The attached patch: removes String based token of Cell & SpatialPrefixTree. Geohash Cell still uses Strings though; Quad was improved to not. renamed getTokenBytes() to getTokenBytesNoLeaf(), plus added getTokenBytes() that will have a leaf if the leaf is set. instead of using a boolean leaf flag; we examine the encoding Cell is no longer Comparable (wasn't needed) The former inner class CellTokenStream is now its own class and modeled heavily off of NumericTokenStream. I'll do a performance test to ensure the performance is at least the same.
          Hide
          David Smiley added a comment -

          Indexing & search performance appears to be very slightly faster; in no test run was it slower.

          I'll commit tonight.

          Show
          David Smiley added a comment - Indexing & search performance appears to be very slightly faster; in no test run was it slower. I'll commit tonight.
          Hide
          ASF subversion and git services added a comment -

          Commit 1584793 from David Smiley in branch 'dev/trunk'
          [ https://svn.apache.org/r1584793 ]

          LUCENE-5565: Refactor SpatialPrefixTree/Cell to not use Strings. Other misc API changes to these classes too.

          Show
          ASF subversion and git services added a comment - Commit 1584793 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1584793 ] LUCENE-5565 : Refactor SpatialPrefixTree/Cell to not use Strings. Other misc API changes to these classes too.
          Hide
          ASF subversion and git services added a comment -

          Commit 1584795 from David Smiley in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1584795 ]

          LUCENE-5565: Refactor SpatialPrefixTree/Cell to not use Strings. Other misc API changes to these classes too.

          Show
          ASF subversion and git services added a comment - Commit 1584795 from David Smiley in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1584795 ] LUCENE-5565 : Refactor SpatialPrefixTree/Cell to not use Strings. Other misc API changes to these classes too.
          Hide
          David Smiley added a comment -

          Committed with couple minor modifications from the patch. I made CellTokenStream package-private, and I made Cell.LEAF_BYTE private.

          Show
          David Smiley added a comment - Committed with couple minor modifications from the patch. I made CellTokenStream package-private, and I made Cell.LEAF_BYTE private.
          Hide
          David Smiley added a comment -

          I'm going to back this out of branch 4x (and thus 4.8) because I (now) view it one incremental step of a larger thing which is LUCENE-5608 (SPT API refactor). LUCENE-5608 won't make it into 4.8 so better to not see it partway done and get released. In fact LUCENE-5608 shouldn't be backported to 4x until at least one new SPT uses the API, which may trigger some changes to the new API.

          Show
          David Smiley added a comment - I'm going to back this out of branch 4x (and thus 4.8) because I (now) view it one incremental step of a larger thing which is LUCENE-5608 (SPT API refactor). LUCENE-5608 won't make it into 4.8 so better to not see it partway done and get released. In fact LUCENE-5608 shouldn't be backported to 4x until at least one new SPT uses the API, which may trigger some changes to the new API.
          Hide
          ASF subversion and git services added a comment -

          Commit 1588398 from David Smiley in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1588398 ]

          LUCENE-5565: reverting spatial API change; leave in trunk for now

          Show
          ASF subversion and git services added a comment - Commit 1588398 from David Smiley in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1588398 ] LUCENE-5565 : reverting spatial API change; leave in trunk for now
          Hide
          ASF subversion and git services added a comment -

          Commit 1588399 from David Smiley in branch 'dev/branches/lucene_solr_4_8'
          [ https://svn.apache.org/r1588399 ]

          LUCENE-5565: reverting spatial API change; leave in trunk for now

          Show
          ASF subversion and git services added a comment - Commit 1588399 from David Smiley in branch 'dev/branches/lucene_solr_4_8' [ https://svn.apache.org/r1588399 ] LUCENE-5565 : reverting spatial API change; leave in trunk for now
          Hide
          ASF subversion and git services added a comment -

          Commit 1588400 from David Smiley in branch 'dev/trunk'
          [ https://svn.apache.org/r1588400 ]

          LUCENE-5565: reverting spatial API change; leave in trunk for now

          Show
          ASF subversion and git services added a comment - Commit 1588400 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1588400 ] LUCENE-5565 : reverting spatial API change; leave in trunk for now
          Hide
          Uwe Schindler added a comment -

          Close issue after release of 4.8.0

          Show
          Uwe Schindler added a comment - Close issue after release of 4.8.0

            People

            • Assignee:
              David Smiley
              Reporter:
              David Smiley
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development