Solr
  1. Solr
  2. SOLR-2345

Extend geodist() to support MultiValued lat long field

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5, 6.0
    • Component/s: spatial
    • Labels:
      None

      Description

      Extend geodist() and

      {!geofilt}

      to support a multiValued lat,long field without using geohash.

      sort=geodist() asc

        Issue Links

          Activity

          Hide
          Grant Ingersoll added a comment -

          Hi Bill,

          Can you clarify what you are trying to do here? You should be able to sort/score by function already, so I am not sure what the extension is.

          Show
          Grant Ingersoll added a comment - Hi Bill, Can you clarify what you are trying to do here? You should be able to sort/score by function already, so I am not sure what the extension is.
          Hide
          Bill Bell added a comment - - edited

          Here is more info:

          id=56
          store=43,-96
          store=42,-97.5
          id=57
          store=42,-97
          store=41,-95
          id=58
          store=40,-95

          I am trying to do:

          http://localhost:8983/solr/select?q=*:*&fq=

          {!geofilt}

          &pt=42,-97&sfield=store&d=10&sort=geodist() asc

          I want to change return the point that is closest and limit by 10km.

          return order:

          id=57

          {42,-97}

          id=56

          {42,-97.5}
          Show
          Bill Bell added a comment - - edited Here is more info: id=56 store=43,-96 store=42,-97.5 id=57 store=42,-97 store=41,-95 id=58 store=40,-95 I am trying to do: http://localhost:8983/solr/select?q=*:*&fq= {!geofilt} &pt=42,-97&sfield=store&d=10&sort=geodist() asc I want to change return the point that is closest and limit by 10km. return order: id=57 {42,-97} id=56 {42,-97.5}
          Hide
          Bill Bell added a comment -

          If we can change geodist() and

          {!geofilt}

          to work on lat_long multiValues. This would allow more flexibility. So a person with multiple addresses will be supported.

          I worked on geomultidist() in SOLR-2155. We would just need to support this in the actual filter to limit the results for the multi value list.

          Thoughts?

          Thanks.

          Show
          Bill Bell added a comment - If we can change geodist() and {!geofilt} to work on lat_long multiValues. This would allow more flexibility. So a person with multiple addresses will be supported. I worked on geomultidist() in SOLR-2155 . We would just need to support this in the actual filter to limit the results for the multi value list. Thoughts? Thanks.
          Hide
          David Smiley added a comment -

          I wouldn't expect to see any progress here any time soon. The underlying problem is that the FieldCache architecture is single-valued. Lucene needs a solution to rectify that. From what I hear, grand plans to rectify this FieldCache limitation in the past have failed (I don't know why). But even if this hurdle was resolved, it'd only be half the battle. geodist() & geofilt() code is built around the notion of multiple fields supplying the lat & lon (VectorValueSource or something like that), not around it coming from a single field. This limitation is at the core of these functions.

          By comparison, I've found working on the "Lucene-Spatial-Playground" (temporary name) a breath of fresh air. Multi-valued fields is not in the FieldCache; we have a spatial substitute for that. And we don't use geodist() & geofilt() at all.

          Show
          David Smiley added a comment - I wouldn't expect to see any progress here any time soon. The underlying problem is that the FieldCache architecture is single-valued. Lucene needs a solution to rectify that. From what I hear, grand plans to rectify this FieldCache limitation in the past have failed (I don't know why). But even if this hurdle was resolved, it'd only be half the battle. geodist() & geofilt() code is built around the notion of multiple fields supplying the lat & lon (VectorValueSource or something like that), not around it coming from a single field. This limitation is at the core of these functions. By comparison, I've found working on the "Lucene-Spatial-Playground" (temporary name) a breath of fresh air. Multi-valued fields is not in the FieldCache; we have a spatial substitute for that. And we don't use geodist() & geofilt() at all.
          Hide
          David Smiley added a comment -

          I re-labelled this issue to focus on geodist().

          {!geofilt}

          , as of v4.2 works with the multi-valued capable field: SpatialRecursivePrefixTreeFieldType (RPT for short).

          What I previously commented on, regarding a multiValued FieldCache was implemented already in Lucene 4.2 but making geodist() work for RPT as well as LatLonType isn't so much related to that actually.

          It'd be a real hack to make geodist() work with RPT, as geodist() internally has very different expectations about how points are obtained per-document, between how LatLonType works and how the Lucene spatial Strategy abstraction works (what's underneath RPT). However, I'm increasingly inclined to think the hack would be worth it from a developer ease-of-use point of view. I'll keep this issue on my TODO-list radar.

          Show
          David Smiley added a comment - I re-labelled this issue to focus on geodist(). {!geofilt} , as of v4.2 works with the multi-valued capable field: SpatialRecursivePrefixTreeFieldType (RPT for short). What I previously commented on, regarding a multiValued FieldCache was implemented already in Lucene 4.2 but making geodist() work for RPT as well as LatLonType isn't so much related to that actually. It'd be a real hack to make geodist() work with RPT, as geodist() internally has very different expectations about how points are obtained per-document, between how LatLonType works and how the Lucene spatial Strategy abstraction works (what's underneath RPT). However, I'm increasingly inclined to think the hack would be worth it from a developer ease-of-use point of view. I'll keep this issue on my TODO-list radar.
          Hide
          David Smiley added a comment -

          Step one is this patch which simply does a refactoring to pull out the very complicated geodist ValueSourceParser out of HaversineConstFunction into its own class. I'll commit this in a couple days, subject to feedback if any.

          Step two will be another patch following this that actually adds RPT (short for SpatialRecursivePrefixTreeFieldType) support to geodist.

          Show
          David Smiley added a comment - Step one is this patch which simply does a refactoring to pull out the very complicated geodist ValueSourceParser out of HaversineConstFunction into its own class. I'll commit this in a couple days, subject to feedback if any. Step two will be another patch following this that actually adds RPT (short for SpatialRecursivePrefixTreeFieldType) support to geodist.
          Hide
          David Smiley added a comment -

          By the way, geodist() handles a variety of invocation approaches, not all of which involve "sfield". From the comments:

              // "m" is a multi-value source, "x" is a single-value source
              // allow (m,m) (m,x,x) (x,x,m) (x,x,x,x)
              // if not enough points are present, "pt" will be checked first, followed by "sfield".
          

          Adapting geodist() to support RPT will only work with explicit use of sfield & pt.

          Show
          David Smiley added a comment - By the way, geodist() handles a variety of invocation approaches, not all of which involve "sfield". From the comments: // "m" is a multi-value source, "x" is a single-value source // allow (m,m) (m,x,x) (x,x,m) (x,x,x,x) // if not enough points are present, "pt" will be checked first, followed by "sfield" . Adapting geodist() to support RPT will only work with explicit use of sfield & pt.
          Hide
          David Smiley added a comment -

          The attached patch implements the desired functionality. It depends on LUCENE-5118 being applied first.

          This hack wasn't too terrible after all.

          Show
          David Smiley added a comment - The attached patch implements the desired functionality. It depends on LUCENE-5118 being applied first. This hack wasn't too terrible after all.
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-2345: geodist support for RPT

          Show
          ASF subversion and git services added a comment - Commit 1507409 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1507409 ] SOLR-2345 : geodist support for RPT
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-2345: geodist support for RPT

          Show
          ASF subversion and git services added a comment - Commit 1507410 from David Smiley in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1507410 ] SOLR-2345 : geodist support for RPT
          Hide
          David Smiley added a comment -

          Closed. New feature:

          * SOLR-2345: Enhanced geodist() to work with an RPT field, provided that the
            field is referenced via 'sfield' and the query point is constant.
            (David Smiley)
          
          Show
          David Smiley added a comment - Closed. New feature: * SOLR-2345: Enhanced geodist() to work with an RPT field, provided that the field is referenced via 'sfield' and the query point is constant. (David Smiley)
          Hide
          Bill Bell added a comment -

          Can you please apply this to branches/lucene_solr_4_4 with LUCENE-5118 as well?

          Show
          Bill Bell added a comment - Can you please apply this to branches/lucene_solr_4_4 with LUCENE-5118 as well?
          Hide
          David Smiley added a comment -

          That branch is done as 4.4 was released. It'll make it into 4.5.

          Show
          David Smiley added a comment - That branch is done as 4.4 was released. It'll make it into 4.5.
          Hide
          Bill Bell added a comment -

          We are suing it in PROD, and so far no issues

          Show
          Bill Bell added a comment - We are suing it in PROD, and so far no issues
          Hide
          Adrien Grand added a comment -

          4.5 release -> bulk close

          Show
          Adrien Grand added a comment - 4.5 release -> bulk close

            People

            • Assignee:
              David Smiley
              Reporter:
              Bill Bell
            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development