Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-8859

AbstractSpatialFieldType can use ShapeContext to read/write shapes (WKT, GeoJSON)

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.1, master (7.0)
    • Component/s: None
    • Labels:
      None
    • Flags:
      Patch

      Description

      Right now the AbstractSpatialFieldType throws exceptions if it needs to convert to/from a string. We should use the context to convert

      1. SOLR-8859_part2.patch
        11 kB
        David Smiley
      2. SOLR-8859.patch
        8 kB
        Ryan McKinley

        Activity

        Hide
        dsmiley David Smiley added a comment -

        +1

        Show
        dsmiley David Smiley added a comment - +1
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 022877fefabadd5865c335a5b289874d182ed852 in lucene-solr's branch refs/heads/master from Ryan McKinley
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=022877f ]

        SOLR-8859: read/write Shapes to String

        Show
        jira-bot ASF subversion and git services added a comment - Commit 022877fefabadd5865c335a5b289874d182ed852 in lucene-solr's branch refs/heads/master from Ryan McKinley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=022877f ] SOLR-8859 : read/write Shapes to String
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7687667b5ff7867249762d104707a91834d30ce3 in lucene-solr's branch refs/heads/branch_6x from Ryan McKinley
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7687667 ]

        SOLR-8859: read/write Shapes to String

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7687667b5ff7867249762d104707a91834d30ce3 in lucene-solr's branch refs/heads/branch_6x from Ryan McKinley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7687667 ] SOLR-8859 : read/write Shapes to String
        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
        Hide
        dsmiley David Smiley added a comment -

        While looking at the code for this again, I saw a couple problems, which I verified through the debugger.

        The former behavior used to support 2 formats: "lat,lon" and WKT, and it did this very efficiently – it could tell it's WKT immediately and if not it parsed using SpatialUtils.parsePointSolrException (the lat,lon format).

        Now, it'll use the Spatial4j shapeReader configured, which if not configured (true for existing configs and I suspect for the majority of folks going forward still?) it'll be arbitrarily the first one. By coincidence that's GeoJSON. No matter what it is, however, parseShape will try all ShapeReaders in Spatial4j registered with the SpatialContext. This even includes, sadly, LegacyShapeReader which parses "lat,lon" and some stuff like "Circle(x y d=dist)" that we stopped supporting many releases ago. Some of these readIfSupported impls throw exceptions to signal they don't support it. So in summary, (1) this is slow, and (2) IMO if you configure= format X then we should only parse as format X. I have reservations about the notion of cycling through all formats to see what it is.

        RptWithGeometrySpatialField.init doesn't call super.init and thus the shapeReader & shapeWriter isn't initialized. That's perhaps not a bug in this issue but it was a non-issue before and now it is. It's a challenging conundrum; ideally it would but I had difficulty with a cach-22 of extending it and also supplying it to CompositeSpatialStrategy. I can revisit that in a separate issue.

        Show
        dsmiley David Smiley added a comment - While looking at the code for this again, I saw a couple problems, which I verified through the debugger. The former behavior used to support 2 formats: "lat,lon" and WKT, and it did this very efficiently – it could tell it's WKT immediately and if not it parsed using SpatialUtils.parsePointSolrException (the lat,lon format). Now, it'll use the Spatial4j shapeReader configured, which if not configured (true for existing configs and I suspect for the majority of folks going forward still?) it'll be arbitrarily the first one. By coincidence that's GeoJSON. No matter what it is, however, parseShape will try all ShapeReaders in Spatial4j registered with the SpatialContext. This even includes, sadly, LegacyShapeReader which parses "lat,lon" and some stuff like "Circle(x y d=dist)" that we stopped supporting many releases ago. Some of these readIfSupported impls throw exceptions to signal they don't support it. So in summary, (1) this is slow, and (2) IMO if you configure= format X then we should only parse as format X. I have reservations about the notion of cycling through all formats to see what it is. RptWithGeometrySpatialField.init doesn't call super.init and thus the shapeReader & shapeWriter isn't initialized. That's perhaps not a bug in this issue but it was a non-issue before and now it is. It's a challenging conundrum; ideally it would but I had difficulty with a cach-22 of extending it and also supplying it to CompositeSpatialStrategy. I can revisit that in a separate issue.
        Hide
        dsmiley David Smiley added a comment -

        The attached patch rectifies the aforementioned problems includes more testing. Fixing RptWithGeometrySpatialField was actually pretty easy so I just did it here. The patch also clarifies the CHANGES.txt to make it more apparent that this adds GeoJSON:

        * SOLR-8859: Spatial fields like RPT can now be configured to use Spatial4j registered shape formats
          e.g. via format="GeoJSON".  (ryan, David Smiley)
        

        I plan to commit this tomorrow.

        Show
        dsmiley David Smiley added a comment - The attached patch rectifies the aforementioned problems includes more testing. Fixing RptWithGeometrySpatialField was actually pretty easy so I just did it here. The patch also clarifies the CHANGES.txt to make it more apparent that this adds GeoJSON: * SOLR-8859: Spatial fields like RPT can now be configured to use Spatial4j registered shape formats e.g. via format="GeoJSON". (ryan, David Smiley) I plan to commit this tomorrow.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit fb37b3eb8c4130c8b5f53e1741e9585743b26e4d in lucene-solr's branch refs/heads/master from David Smiley
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fb37b3e ]

        SOLR-8859: Fix AbstractSpatialFieldType to not cycle through all Spatial4j provided formats.
        And Fix RptWithGeometrySpatialField to be less brittle on init()

        Show
        jira-bot ASF subversion and git services added a comment - Commit fb37b3eb8c4130c8b5f53e1741e9585743b26e4d in lucene-solr's branch refs/heads/master from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fb37b3e ] SOLR-8859 : Fix AbstractSpatialFieldType to not cycle through all Spatial4j provided formats. And Fix RptWithGeometrySpatialField to be less brittle on init()
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 3a57beaa9e202c848061881d3c887ec076f19282 in lucene-solr's branch refs/heads/branch_6x from David Smiley
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3a57bea ]

        SOLR-8859: Fix AbstractSpatialFieldType to not cycle through all Spatial4j provided formats.
        And Fix RptWithGeometrySpatialField to be less brittle on init()
        (cherry picked from commit fb37b3e)

        Show
        jira-bot ASF subversion and git services added a comment - Commit 3a57beaa9e202c848061881d3c887ec076f19282 in lucene-solr's branch refs/heads/branch_6x from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3a57bea ] SOLR-8859 : Fix AbstractSpatialFieldType to not cycle through all Spatial4j provided formats. And Fix RptWithGeometrySpatialField to be less brittle on init() (cherry picked from commit fb37b3e)
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit fb37b3eb8c4130c8b5f53e1741e9585743b26e4d in lucene-solr's branch refs/heads/SOLR-9191 from David Smiley
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fb37b3e ]

        SOLR-8859: Fix AbstractSpatialFieldType to not cycle through all Spatial4j provided formats.
        And Fix RptWithGeometrySpatialField to be less brittle on init()

        Show
        jira-bot ASF subversion and git services added a comment - Commit fb37b3eb8c4130c8b5f53e1741e9585743b26e4d in lucene-solr's branch refs/heads/ SOLR-9191 from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fb37b3e ] SOLR-8859 : Fix AbstractSpatialFieldType to not cycle through all Spatial4j provided formats. And Fix RptWithGeometrySpatialField to be less brittle on init()

          People

          • Assignee:
            ryantxu Ryan McKinley
            Reporter:
            ryantxu Ryan McKinley
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development