Lucene - Core
  1. Lucene - Core
  2. LUCENE-5771

Review semantics of SpatialOperation predicates

    Details

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

      Description

      SpatialOperation (which I wish was named SpatialPredicate) is a bunch of predicates – methods that return true/false based on a pair of shapes. Some of them don't seem to be defined in a way consistent with their definitions on ESRI's site:
      http://edndoc.esri.com/arcsde/9.1/general_topics/understand_spatial_relations.htm (which is linked as a reference, and is in turn equivalent to OGC spec definitions, I believe).

      Problems:

      • the definitions make no mention of needing to have area or not, yet some of our predicates are defined as to require area on either the indexed or query shape.
      • the definitions make a distinction of the boundary of a shape, yet in Lucene-spatial, there is none. That suggests our predicates are wrongly chosen since there are official predicates that are boundary-neutral – namely "Covers" and "CoveredBy" in lieu of Contains and Within, respectively. If we don't rename our predicates, we should at least support the correct predicates names!
      • Overlaps appears totally wrong. It should be defined as indexedShape.relate(queryShape) == Intersects (and thus not Within or Contains or Disjoint). It's presently defined as the same as Intersects plus the query shape needing area.

        Activity

        Hide
        David Smiley added a comment -

        It's a simple patch. Not seen here is a change to BBoxStrategy to disavow it's support for Overlaps.

        Furthermore, I'm not sure "queryNeedsArea" and "targetNeedsArea" is needed or has correct values. Same for scoreIsMeaningful. Can't this thing be a simple enum? Could you take a look Ryan McKinley?

        Show
        David Smiley added a comment - It's a simple patch. Not seen here is a change to BBoxStrategy to disavow it's support for Overlaps. Furthermore, I'm not sure "queryNeedsArea" and "targetNeedsArea" is needed or has correct values. Same for scoreIsMeaningful. Can't this thing be a simple enum? Could you take a look Ryan McKinley ?
        Hide
        Ryan McKinley added a comment -

        change looks good

        I think the reason for "queryNeedsArea" and "targetNeedsArea" was so that we could decide if the input was valid simply based on the shape – but i think we find that out anyway, so we could just drop these.

        I don't think 'scoreIsMeaningful' is used, so it can also be dropped

        Show
        Ryan McKinley added a comment - change looks good I think the reason for "queryNeedsArea" and "targetNeedsArea" was so that we could decide if the input was valid simply based on the shape – but i think we find that out anyway, so we could just drop these. I don't think 'scoreIsMeaningful' is used, so it can also be dropped
        Hide
        David Smiley added a comment -

        The attached patch removes the "needs area" and "score is meaningful" notions from SpatialOperation.

        I also added aliases to the various predicates to align with the standard names. Ryan, why did you choose the non-standard names? E.g. why did you choose "IsEqualTo" when "Equals" is the standard name? And why the BBoxIntersects and BBoxWithin predicates which I'm not aware we use and are also non-standard and seem better addressed via other ways (e.g. some sort of function one adds that bbox'es a shape).

        Show
        David Smiley added a comment - The attached patch removes the "needs area" and "score is meaningful" notions from SpatialOperation. I also added aliases to the various predicates to align with the standard names. Ryan, why did you choose the non-standard names? E.g. why did you choose "IsEqualTo" when "Equals" is the standard name? And why the BBoxIntersects and BBoxWithin predicates which I'm not aware we use and are also non-standard and seem better addressed via other ways (e.g. some sort of function one adds that bbox'es a shape).
        Hide
        David Smiley added a comment -

        Well I'll commit my patch within a couple days. It'll deserve visible entries in CHANGES.txt so it's clear what aliases people can use. In fact I think such notices should suggest using the more standardized names – that's what standards are for! Arguably there should be constants for these standard names too.

        Show
        David Smiley added a comment - Well I'll commit my patch within a couple days. It'll deserve visible entries in CHANGES.txt so it's clear what aliases people can use. In fact I think such notices should suggest using the more standardized names – that's what standards are for! Arguably there should be constants for these standard names too.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5771: Remove BBoxStrategy's support for Overlaps because it never actually did work.

        This is a partial commit for this issue – just the BBox portion so as not to interfere with LUCENE-5779. Trunk only (bbox isn't in 4x yet).

        Show
        ASF subversion and git services added a comment - Commit 1606905 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1606905 ] LUCENE-5771 : Remove BBoxStrategy's support for Overlaps because it never actually did work. This is a partial commit for this issue – just the BBox portion so as not to interfere with LUCENE-5779 . Trunk only (bbox isn't in 4x yet).
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5771: SpatialOperation refactoring: (OGC alias names, removed area requirement, fixed overlap definition)

        Show
        ASF subversion and git services added a comment - Commit 1608922 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1608922 ] LUCENE-5771 : SpatialOperation refactoring: (OGC alias names, removed area requirement, fixed overlap definition)
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-5771: SpatialOperation refactoring: (OGC alias names, removed area requirement, fixed overlap definition)

        Show
        ASF subversion and git services added a comment - Commit 1608939 from David Smiley in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1608939 ] LUCENE-5771 : SpatialOperation refactoring: (OGC alias names, removed area requirement, fixed overlap definition)

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development