Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7853

Request to change visibility of classes in geo3d

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 7.0, 6.7
    • Component/s: modules/spatial3d
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      I am creating my own spatial context by wrapping the objects in lucene geo3d library and implement my own query which mixes the recursive and the serialized strategy to add precision to searches.

      I had the following issue specially with polygons: The factory for creating polygons is slow and when serializing shapes, I already know if I am dealing with convex or concave polygons (in my case they are always concave). So when de-serializing a polygon I do not want to go through the factory to decide the type of polygon to create.

      Wouldn't make sense to add the possibility to create a type of polygon directly? you cannot create the polygons directly as they are protected in the package. My suggestion request would be either to add methods in the polygon factory to create a type of polygons with no checking (e.g. makeConcavePolygon(…)) or change the visibility of the classes so that they can be instantiated directly.

      1. LUCENE-7853.patch
        10 kB
        Ignacio Vera

        Activity

        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, I'd be happy to help with this. I'd prefer the approach of just creating additional factory methods, if possible. Would you be willing to submit a patch along those lines? Thanks!

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , I'd be happy to help with this. I'd prefer the approach of just creating additional factory methods, if possible. Would you be willing to submit a patch along those lines? Thanks!
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        I agree with your apporach and yes I can submit the patch. On the other hand I am a bit new on this, How do I submit the patch?

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , I agree with your apporach and yes I can submit the patch. On the other hand I am a bit new on this, How do I submit the patch?
        Hide
        ivera Ignacio Vera added a comment -

        Do you see any isuue with the following proposal:

        I would like to add the following four methods to the GeoPolygonFactory which should only be used when we know the type of polygon we are delaing with, e.g. polygon has been serialized:

        public static GeoConcavePolygon makeGeoConcavePolygon(final PlanetModel planetModel,
        final List<GeoPoint> pointList)

        { return new GeoConcavePolygon(planetModel, pointList); }

        public static GeoConcavePolygon makeGeoConcavePolygon(final PlanetModel planetModel,
        final List<GeoPoint> pointList,
        final List<GeoPolygon> holes)

        { return new GeoConcavePolygon(planetModel,pointList, holes); }

        public static GeoConvexPolygon makeGeoConvexPolygon(final PlanetModel planetModel,
        final List<GeoPoint> pointList)

        { return new GeoConvexPolygon(planetModel, pointList); }

        public static GeoConvexPolygon makeGeoConvexPolygon(final PlanetModel planetModel,
        final List<GeoPoint> pointList,
        final List<GeoPolygon> holes)

        { return new GeoConvexPolygon(planetModel,pointList, holes); }
        Show
        ivera Ignacio Vera added a comment - Do you see any isuue with the following proposal: I would like to add the following four methods to the GeoPolygonFactory which should only be used when we know the type of polygon we are delaing with, e.g. polygon has been serialized: public static GeoConcavePolygon makeGeoConcavePolygon(final PlanetModel planetModel, final List<GeoPoint> pointList) { return new GeoConcavePolygon(planetModel, pointList); } public static GeoConcavePolygon makeGeoConcavePolygon(final PlanetModel planetModel, final List<GeoPoint> pointList, final List<GeoPolygon> holes) { return new GeoConcavePolygon(planetModel,pointList, holes); } public static GeoConvexPolygon makeGeoConvexPolygon(final PlanetModel planetModel, final List<GeoPoint> pointList) { return new GeoConvexPolygon(planetModel, pointList); } public static GeoConvexPolygon makeGeoConvexPolygon(final PlanetModel planetModel, final List<GeoPoint> pointList, final List<GeoPolygon> holes) { return new GeoConvexPolygon(planetModel,pointList, holes); }
        Hide
        ivera Ignacio Vera added a comment -

        Attached is that patch.

        One thing I noticed is that when I call the method makeGeopolygon from the polygon factory I always get back a GeoCompositePolygon even when building a simple four points polygon. Is that behavior expected?

        Show
        ivera Ignacio Vera added a comment - Attached is that patch. One thing I noticed is that when I call the method makeGeopolygon from the polygon factory I always get back a GeoCompositePolygon even when building a simple four points polygon. Is that behavior expected?
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, the patch seems to not be generated against master; it's generated against something that already has a method that builds a concave polygon. So I need the rest of it or I cannot apply it.

        To answer your question: the makeGeoPolygon() method is supposed to return a GeoPolygon, and that's what it does. There's no requirement in the contract that it be simplified to the minimal description of that polygon.

        Thanks...

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , the patch seems to not be generated against master; it's generated against something that already has a method that builds a concave polygon. So I need the rest of it or I cannot apply it. To answer your question: the makeGeoPolygon() method is supposed to return a GeoPolygon, and that's what it does. There's no requirement in the contract that it be simplified to the minimal description of that polygon. Thanks...
        Hide
        ivera Ignacio Vera added a comment -

        New patch attached, this time looks what you expect I hope.

        Thanks for he answer. In my case I would have to simplify the polygon to its minimal description because of performance. I can do that at indexing time.

        Cheers,

        I.

        Show
        ivera Ignacio Vera added a comment - New patch attached, this time looks what you expect I hope. Thanks for he answer. In my case I would have to simplify the polygon to its minimal description because of performance. I can do that at indexing time. Cheers, I.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera: A composite polygon isn't appreciably slower than a root-level concave or convex polygon. It's basically a wrapper when there's only one child. So I am not sure what you mean about performance?

        The new patch is fine except that I don't believe the comments are correct. The order of points does not matter for the new methods because concavity or convexity is assumed. If you can clarify that in the comments it would allow me to commit your patch unmodified; if not, it will need to wait until I have a moment to look in more detail.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera : A composite polygon isn't appreciably slower than a root-level concave or convex polygon. It's basically a wrapper when there's only one child. So I am not sure what you mean about performance? The new patch is fine except that I don't believe the comments are correct. The order of points does not matter for the new methods because concavity or convexity is assumed. If you can clarify that in the comments it would allow me to commit your patch unmodified; if not, it will need to wait until I have a moment to look in more detail.
        Hide
        ivera Ignacio Vera added a comment -

        Karl Wright: You are totally right, the order of the points (counter-clockwise or clockwise) does not matter as it always generates the convex polygon. I will try to re-phrase it an add a new patch.

        Composite polygons are not appreciably slower but my strategy is to serialize the objects to read them at query time for the boundaries of a spatial search (inspired from SerializedDVStrategy). Having to serialize the composite polygon and the polygon inside it and then read it during a query can have a performance penalty when dealing with hundreds or even thousand of them.

        Another more philosofical doubt I have is that composite polygons can be confused with multipolygons. One thing I have learnt the hard way is that multipolygons cannot be composed of polygons that intersect to each other. I guess this is not the case for Geo3d composite polygons.

        Show
        ivera Ignacio Vera added a comment - Karl Wright : You are totally right, the order of the points (counter-clockwise or clockwise) does not matter as it always generates the convex polygon. I will try to re-phrase it an add a new patch. Composite polygons are not appreciably slower but my strategy is to serialize the objects to read them at query time for the boundaries of a spatial search (inspired from SerializedDVStrategy). Having to serialize the composite polygon and the polygon inside it and then read it during a query can have a performance penalty when dealing with hundreds or even thousand of them. Another more philosofical doubt I have is that composite polygons can be confused with multipolygons. One thing I have learnt the hard way is that multipolygons cannot be composed of polygons that intersect to each other. I guess this is not the case for Geo3d composite polygons.
        Hide
        ivera Ignacio Vera added a comment -

        Attached a new patch with updated documentation. I hope this is closer to what is expected. It was a good exercise for me as now I have a better understanding how it works for complex polygons.

        Thanks,

        I.

        Show
        ivera Ignacio Vera added a comment - Attached a new patch with updated documentation. I hope this is closer to what is expected. It was a good exercise for me as now I have a better understanding how it works for complex polygons. Thanks, I.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera: looks good to me. I will commit.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera : looks good to me. I will commit.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 71411df0cfec385fbd0fcd0f0291c6a06c05a428 in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=71411df ]

        LUCENE-7853: Add methods for people to use who know their polygon's characteristics

        Show
        jira-bot ASF subversion and git services added a comment - Commit 71411df0cfec385fbd0fcd0f0291c6a06c05a428 in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=71411df ] LUCENE-7853 : Add methods for people to use who know their polygon's characteristics
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 8cf8546134fb54a2c5058e50e1fabfe52f977454 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8cf8546 ]

        LUCENE-7853: Add methods for people to use who know their polygon's characteristics

        Show
        jira-bot ASF subversion and git services added a comment - Commit 8cf8546134fb54a2c5058e50e1fabfe52f977454 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8cf8546 ] LUCENE-7853 : Add methods for people to use who know their polygon's characteristics
        Hide
        hossman Hoss Man added a comment -

        precommit failures from documentation-lint...

             [echo] checking for broken html...
            [jtidy] Checking for broken html (such as invalid tags)...
           [delete] Deleting directory /home/hossman/lucene/dev/lucene/build/jtidy_tmp
             [echo] Checking for broken links...
             [exec] 
             [exec] Crawl/parse...
             [exec] 
             [exec] Verify...
             [exec] 
             [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/GeoPolygonFactory.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec] 
             [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/class-use/GeoPoint.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec] 
             [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/class-use/GeoPolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec] 
             [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/class-use/PlanetModel.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec]   BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html
             [exec] 
             [exec] Broken javadocs links were found!
        
        Show
        hossman Hoss Man added a comment - precommit failures from documentation-lint... [echo] checking for broken html... [jtidy] Checking for broken html (such as invalid tags)... [delete] Deleting directory /home/hossman/lucene/dev/lucene/build/jtidy_tmp [echo] Checking for broken links... [exec] [exec] Crawl/parse... [exec] [exec] Verify... [exec] [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/GeoPolygonFactory.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/class-use/GeoPoint.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/class-use/GeoPolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] [exec] file:///build/docs/spatial3d/org/apache/lucene/spatial3d/geom/class-use/PlanetModel.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConcavePolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] BROKEN LINK: file:///build/docs/core/org/apache/lucene/spatial3d.geom.GeoConvexPolygon.html [exec] [exec] Broken javadocs links were found!
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hoss Man: This is odd because it's complaining about stuff that hasn't changed in a year.

        Show
        kwright@metacarta.com Karl Wright added a comment - Hoss Man : This is odd because it's complaining about stuff that hasn't changed in a year.
        Hide
        hossman Hoss Man added a comment -

        Huh?

        You just changed GeoPolygonFactory a few hours ago, and added new public methods that refer to classes like GeoConcavePolygon and GeoConvexPolygon in the new method sigs – but those classes are not public, so the links in the generated javadocs are broken.

        I suggest rolling this change back until there's clarity on what exactly should be public ... public methods method declaring that they return package hidden classes makes no sense.

        Show
        hossman Hoss Man added a comment - Huh? You just changed GeoPolygonFactory a few hours ago, and added new public methods that refer to classes like GeoConcavePolygon and GeoConvexPolygon in the new method sigs – but those classes are not public, so the links in the generated javadocs are broken. I suggest rolling this change back until there's clarity on what exactly should be public ... public methods method declaring that they return package hidden classes makes no sense.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hoss Man: If that's the problem it is unclear from precommit's output. Precommit is complaining about references from GeoPoint etc. which were not updated in any way in a very long time, and GeoPoint does not return package-private classes either.

        Show
        kwright@metacarta.com Karl Wright added a comment - Hoss Man : If that's the problem it is unclear from precommit's output. Precommit is complaining about references from GeoPoint etc. which were not updated in any way in a very long time, and GeoPoint does not return package-private classes either.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 08ea9a787e624db4440c57b882afcfecddecc86b in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=08ea9a7 ]

        LUCENE-7853: Return the public interface, not the private implementation

        Show
        jira-bot ASF subversion and git services added a comment - Commit 08ea9a787e624db4440c57b882afcfecddecc86b in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=08ea9a7 ] LUCENE-7853 : Return the public interface, not the private implementation
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 01c7bf1fc3b3f13943193af66749c0f733d102e2 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=01c7bf1 ]

        LUCENE-7853: Return the public interface, not the private implementation

        Show
        jira-bot ASF subversion and git services added a comment - Commit 01c7bf1fc3b3f13943193af66749c0f733d102e2 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=01c7bf1 ] LUCENE-7853 : Return the public interface, not the private implementation
        Hide
        hossman Hoss Man added a comment -

        Precommit is complaining about references from GeoPoint etc.

        documentation-lint is very explicit when it complains – it doesn't complain about classes it complains about documentation – specifically broken links in the documentation. If the link is manually created that's one problem. if the link is generated automatically by javadoc then that's an indirect indication of other problems (like referring to package protected classes in public method sigs) but you have to actually look at the details of the errors.

        In this specfic case, all of the errors stem from the root problem i specifically drew your attention to. the other broken links are all in "class-use" pages, where all methods that refer to a class are listed. Whether the GeoPoint class has been modified in a very long time is irrelevant to the question of why links on it's "class-use" page might be broken – what matters is if any methods that refer to GeoPoint have been added/removed/modified and result in broken links.

        Bottom line: we shouldn't just ignore documentation-lint errors because it looks like it's refering to classes that haven't changed "in a very long time" If you actually look at check the exact errors & html files it directs you to, the root cause is usually very clear.

        Show
        hossman Hoss Man added a comment - Precommit is complaining about references from GeoPoint etc. documentation-lint is very explicit when it complains – it doesn't complain about classes it complains about documentation – specifically broken links in the documentation. If the link is manually created that's one problem. if the link is generated automatically by javadoc then that's an indirect indication of other problems (like referring to package protected classes in public method sigs) but you have to actually look at the details of the errors. In this specfic case, all of the errors stem from the root problem i specifically drew your attention to. the other broken links are all in "class-use" pages, where all methods that refer to a class are listed. Whether the GeoPoint class has been modified in a very long time is irrelevant to the question of why links on it's "class-use" page might be broken – what matters is if any methods that refer to GeoPoint have been added/removed/modified and result in broken links. Bottom line: we shouldn't just ignore documentation-lint errors because it looks like it's refering to classes that haven't changed "in a very long time" If you actually look at check the exact errors & html files it directs you to, the root cause is usually very clear.
        Hide
        cpoerschke Christine Poerschke added a comment -

        LUCENE-7858 to refine the 'broken links' message provided by ant documentation-lint which is part of ant precommit logic. What do you think?

        Show
        cpoerschke Christine Poerschke added a comment - LUCENE-7858 to refine the 'broken links' message provided by ant documentation-lint which is part of ant precommit logic. What do you think?
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Anything that helps is great!

        Show
        kwright@metacarta.com Karl Wright added a comment - Anything that helps is great!
        Hide
        ivera Ignacio Vera added a comment -

        Change applied.

        Thanks!

        Show
        ivera Ignacio Vera added a comment - Change applied. Thanks!

          People

          • Assignee:
            kwright@metacarta.com Karl Wright
            Reporter:
            ivera Ignacio Vera
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development