Lucene - Core
  1. Lucene - Core
  2. LUCENE-6997

Graduate GeoUtils and postings based GeoPointField from sandbox...

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Done
    • Affects Version/s: None
    • Fix Version/s: 5.5, trunk
    • Component/s: modules/spatial
    • Labels:
    • Lucene Fields:
      New

      Description

      GeoPointField is a lightweight dependency-free postings based geo field currently in sandbox. It has evolved into a very fast lightweight geo option that heavily leverages the optimized performance of the postings structure. It was originally intended to graduate to core but this does not seem appropriate given the variety of "built on postings" term encoding options (e.g., see LUCENE-6930).

      Additionally, the Geo*Utils classes are dependency free lightweight relational approximation utilities used by both GeoPointField and the BKD based LatLonField and can also be applied to benefit the lucene-spatial module.

      These classes have been evolving and baking for some time and are at a maturity level qualifying for promotion from sandbox. This will allow support for experimental encoding methods with (minimal) backwards compatibility - something sandbox does not allow.

      Since GeoPoint classes are dependency free, all GeoPointField and support and utility classes currently in sandbox would be promoted to the spatial3d package. (possibly a separate issue to rename spatial3d to spatialcore or spatiallite?) Such that for basic lightweight Geo support one would only need a handful of lucene jars. By simply adding the lucene-spatial module and its dependency jars users can obtain more advanced geospatial support (heatmap facets, full shape relations, etc).

      1. LUCENE-6997.patch
        28 kB
        Nicholas Knize
      2. LUCENE-6997.patch
        69 kB
        Nicholas Knize
      3. LUCENE-6997.patch
        165 kB
        Nicholas Knize

        Activity

        Hide
        David Smiley added a comment -

        It's good to see this wonderful work graduating out of sandbox, Nick.

        This will allow support for experimental encoding methods with (minimal) backwards compatibility - something sandbox does not allow.

        Just curious but can you explain better? I'm unfamiliar with restrictions imposed by sandbox.

        Since GeoPoint classes are dependency free, all GeoPointField and support and utility classes currently in sandbox would be promoted to the spatial3d package. (possibly a separate issue to rename spatial3d to spatialcore or spatiallite?) Such that for basic lightweight Geo support one would only need a handful of lucene jars. By simply adding the lucene-spatial module and its dependency jars users can obtain more advanced geospatial support (heatmap facets, full shape relations, etc).

        I know spatial3d doesn't depend on Spatial4j (what I assume you imply by "dependency free") but I don't think that's what defines this module – it's the Geo3d code and hence it's name. So I don't think GeoPointField would make sense there at all. And I think wherever Geo3d lives, it should have a name reflective of what it is; "spatialcore" or some-such is IMO not a good name. I'd even like to see it not have any dependency whatsoever; the Lucene Field & Query adapters could go to the lucene-spatial module. This would make it easier for external (non-Lucene) consumption. It's very odd to me they even share the same Java package today; that used to not be the case.

        Much of the lucene-spatial module currently has a dependency on Spatial4j, and one part of it has a dependency on Geo3d/spatial3d (Geo3dShape.java) but I don't think that need be fundamental to the spatial module's future. In other words, just because there are parts of the lucene-spatial module that have certain dependencies doesn't mean there could/should be new things to add to the module that don't have those dependencies, and thus user's needn't add such dependencies to their classpath. From a maven standpoint, they could be marked "optional". This would be a bit of a renaissance to the module; a very welcome one IMO. What do you think?

        Show
        David Smiley added a comment - It's good to see this wonderful work graduating out of sandbox, Nick. This will allow support for experimental encoding methods with (minimal) backwards compatibility - something sandbox does not allow. Just curious but can you explain better? I'm unfamiliar with restrictions imposed by sandbox. Since GeoPoint classes are dependency free, all GeoPointField and support and utility classes currently in sandbox would be promoted to the spatial3d package. (possibly a separate issue to rename spatial3d to spatialcore or spatiallite?) Such that for basic lightweight Geo support one would only need a handful of lucene jars. By simply adding the lucene-spatial module and its dependency jars users can obtain more advanced geospatial support (heatmap facets, full shape relations, etc). I know spatial3d doesn't depend on Spatial4j (what I assume you imply by "dependency free") but I don't think that's what defines this module – it's the Geo3d code and hence it's name. So I don't think GeoPointField would make sense there at all. And I think wherever Geo3d lives, it should have a name reflective of what it is; "spatialcore" or some-such is IMO not a good name. I'd even like to see it not have any dependency whatsoever; the Lucene Field & Query adapters could go to the lucene-spatial module. This would make it easier for external (non-Lucene) consumption. It's very odd to me they even share the same Java package today; that used to not be the case. Much of the lucene-spatial module currently has a dependency on Spatial4j, and one part of it has a dependency on Geo3d/spatial3d (Geo3dShape.java) but I don't think that need be fundamental to the spatial module's future. In other words, just because there are parts of the lucene-spatial module that have certain dependencies doesn't mean there could/should be new things to add to the module that don't have those dependencies, and thus user's needn't add such dependencies to their classpath. From a maven standpoint, they could be marked "optional". This would be a bit of a renaissance to the module; a very welcome one IMO. What do you think?
        Hide
        Nicholas Knize added a comment -

        ...can you explain better? I'm unfamiliar with restrictions imposed by sandbox.

        I had the understanding the sandbox module provides no backwards compatibility. Not "it doesn't guarantee backwards compatibility" but there is "NO backwards compatibility". Other modules do not have this restriction. In this particular case the restriction makes it difficult/impossible to experiment and evolve alternative postings based encoding methods without adding some crazy abstraction layer (that just masks backcompat), or duplicating code and (like codecs) changing class names.

        ...doesn't depend on Spatial4j (what I assume you imply by "dependency free")

        Just no dependencies whatsoever, be them optional or not (to include JTS or future dependencies).

        I don't think that need be fundamental to the spatial module's future.

        I agree. While the GeoUtils API is really close to being able to remove the JTS dependency there are some Apache friendly libraries I think would really boost lucene-spatial's capabilities. And IMHO that's what defines the lucene-spatial module. A provider of the more advanced GIS capabilities with dependencies from only "friendly" licenses (I'm not a lawyer so I don't know what this list would include).

        From a maven standpoint, they could be marked "optional".

        I think that's part of the benefits for having a lightweight dependency free module. I was told (and did some quick research - a la elgoog) that optional dependencies are no longer going to be supported by Java 9? I'm sure someone far smarter than me on Java 9 can chime in but if class A depends on B which optionally depends on C you can no longer include B without C? Sounds like that will break the current Spatial4J / JTS model? Also Optional dependencies aren't well supported for build systems other than maven (e.g., gradle)? I'm certainly not a gradle expert (by any means) but I know I've seen this as enough of a headache in the past that gradle users try to avoid them unless necessary.

        To me these seem like compelling reasons to split the complexity of spatial into a lightweight dependency free core module that can be leveraged by modules that either do or do not have a dependency restriction.

        Show
        Nicholas Knize added a comment - ...can you explain better? I'm unfamiliar with restrictions imposed by sandbox. I had the understanding the sandbox module provides no backwards compatibility. Not "it doesn't guarantee backwards compatibility" but there is "NO backwards compatibility". Other modules do not have this restriction. In this particular case the restriction makes it difficult/impossible to experiment and evolve alternative postings based encoding methods without adding some crazy abstraction layer (that just masks backcompat), or duplicating code and (like codecs) changing class names. ...doesn't depend on Spatial4j (what I assume you imply by "dependency free") Just no dependencies whatsoever, be them optional or not (to include JTS or future dependencies). I don't think that need be fundamental to the spatial module's future. I agree. While the GeoUtils API is really close to being able to remove the JTS dependency there are some Apache friendly libraries I think would really boost lucene-spatial's capabilities. And IMHO that's what defines the lucene-spatial module. A provider of the more advanced GIS capabilities with dependencies from only "friendly" licenses (I'm not a lawyer so I don't know what this list would include). From a maven standpoint, they could be marked "optional". I think that's part of the benefits for having a lightweight dependency free module. I was told (and did some quick research - a la elgoog) that optional dependencies are no longer going to be supported by Java 9? I'm sure someone far smarter than me on Java 9 can chime in but if class A depends on B which optionally depends on C you can no longer include B without C? Sounds like that will break the current Spatial4J / JTS model? Also Optional dependencies aren't well supported for build systems other than maven (e.g., gradle)? I'm certainly not a gradle expert (by any means) but I know I've seen this as enough of a headache in the past that gradle users try to avoid them unless necessary. To me these seem like compelling reasons to split the complexity of spatial into a lightweight dependency free core module that can be leveraged by modules that either do or do not have a dependency restriction.
        Hide
        David Smiley added a comment -

        I agree. While the GeoUtils API is really close to being able to remove the JTS dependency ...

        What's missing, in your opinion? And did you actually mean Spatial4j? FYI JTS as of yesterday is licensed BSD.

        Uwe Schindler any thoughts on the impact of Java 9? Or might it be too early to made decisions based on that?

        RE the build system (e.g. proposed gradle, present ant, or whatever): I believe there's no impact; it's just a matter of modifying our maven pom template file with <optional>true</optional> so that consumers after release who use that pom (via Maven or...) don't transitively pick up Spatial4j. I could do that today and the build would succeed – it's just metadata.

        Show
        David Smiley added a comment - I agree. While the GeoUtils API is really close to being able to remove the JTS dependency ... What's missing, in your opinion? And did you actually mean Spatial4j? FYI JTS as of yesterday is licensed BSD. Uwe Schindler any thoughts on the impact of Java 9? Or might it be too early to made decisions based on that? RE the build system (e.g. proposed gradle, present ant, or whatever): I believe there's no impact; it's just a matter of modifying our maven pom template file with <optional>true</optional> so that consumers after release who use that pom (via Maven or...) don't transitively pick up Spatial4j. I could do that today and the build would succeed – it's just metadata.
        Hide
        Uwe Schindler added a comment -

        What does this have to do with Java 9?

        Show
        Uwe Schindler added a comment - What does this have to do with Java 9?
        Hide
        Uwe Schindler added a comment - - edited

        Optional dependencies in Maven have nothing to do with extends or like that. If you have a class that extends another class, the other class must be in classpath. There is no "optional" extends in Java.

        I think Maven "optional" dependencies are for uses cases where you need a class only at compile time (mandatory), but consumer (runtime) may not need it, e.g. if you have something like a implementation class in your JAR file that is needed to support some 3rd party thing. If it is clearly separated, one would only need the optional dependency if he uses this implementation class in his own code.

        But java always needs all classes that are referred from one class through super, interfaces, method signatures.

        Simple said: An optional dependency is not needed fro user, if the public api of your artifact does not expose any classes from this dependency through its signatures. If you have a factory class that just returns an abstract class, implemented by some code in your optional dependency, it is fine, if you have proper exception handling in your factory (e.g., optional dep not on classpath).

        Show
        Uwe Schindler added a comment - - edited Optional dependencies in Maven have nothing to do with extends or like that. If you have a class that extends another class, the other class must be in classpath. There is no "optional" extends in Java. I think Maven "optional" dependencies are for uses cases where you need a class only at compile time (mandatory), but consumer (runtime) may not need it, e.g. if you have something like a implementation class in your JAR file that is needed to support some 3rd party thing. If it is clearly separated, one would only need the optional dependency if he uses this implementation class in his own code. But java always needs all classes that are referred from one class through super, interfaces, method signatures. Simple said: An optional dependency is not needed fro user, if the public api of your artifact does not expose any classes from this dependency through its signatures. If you have a factory class that just returns an abstract class, implemented by some code in your optional dependency, it is fine, if you have proper exception handling in your factory (e.g., optional dep not on classpath).
        Hide
        Nicholas Knize added a comment - - edited

        What's missing, in your opinion? And did you actually mean Spatial4j? FYI JTS as of yesterday is licensed BSD.

        Hey wonderful! I actually meant JTS. Last conversation I had with Martin he wasn't confident in a friendly timeline for relicense. I think the larger discussion re: dependencies is still relevant? Unless there's a blacklist somewhere of licenses we will always reject.

        I think Maven "optional" dependencies are for uses cases where you need a class only at compile time (mandatory), but consumer (runtime) may not need it,

        Its likely I misunderstood this so this clarification will certainly help. I thought Java's module system is changing in Java 9 such that it needs all dependencies at runtime (even if the dependencies are not used)? That's the relevancy of Java 9 in the discussion.

        Show
        Nicholas Knize added a comment - - edited What's missing, in your opinion? And did you actually mean Spatial4j? FYI JTS as of yesterday is licensed BSD. Hey wonderful! I actually meant JTS. Last conversation I had with Martin he wasn't confident in a friendly timeline for relicense. I think the larger discussion re: dependencies is still relevant? Unless there's a blacklist somewhere of licenses we will always reject. I think Maven "optional" dependencies are for uses cases where you need a class only at compile time (mandatory), but consumer (runtime) may not need it, Its likely I misunderstood this so this clarification will certainly help. I thought Java's module system is changing in Java 9 such that it needs all dependencies at runtime (even if the dependencies are not used)? That's the relevancy of Java 9 in the discussion.
        Hide
        Uwe Schindler added a comment -

        I thought Java's module system is changing in Java 9 such that it needs all dependencies at runtime? That's the relevancy of Java 9 in the discussion.

        This is only relevant if we provide Lucene at some point as *.jmod files instead of JARs. JAR files and the classpath behaviour does not change in Java 9. Modules are only needed if you want to ship Lucene together with the JDK and you want to make use of Java's module system. But for this to happen, there needs to be support in Maven, too.

        Show
        Uwe Schindler added a comment - I thought Java's module system is changing in Java 9 such that it needs all dependencies at runtime? That's the relevancy of Java 9 in the discussion. This is only relevant if we provide Lucene at some point as *.jmod files instead of JARs. JAR files and the classpath behaviour does not change in Java 9. Modules are only needed if you want to ship Lucene together with the JDK and you want to make use of Java's module system. But for this to happen, there needs to be support in Maven, too.
        Hide
        Nicholas Knize added a comment -

        Thanks for clearing that up Uwe Schindler!

        Then it boils down to the value of having a spatial module for the simple use case that users don't have to worry about dependency management. If there's agreement in having that value then these classes should go into that package. If not, then either a. spatial3d could be refactored back to the spatial module (since JTS license is no longer an issue), or b. the GeoUtils classes go into spatial3d (renaming still seems appropriate) and the Lucene Query and Field classes go into the spatial module.

        I'm open to thoughts or other ideas.

        Show
        Nicholas Knize added a comment - Thanks for clearing that up Uwe Schindler ! Then it boils down to the value of having a spatial module for the simple use case that users don't have to worry about dependency management. If there's agreement in having that value then these classes should go into that package. If not, then either a. spatial3d could be refactored back to the spatial module (since JTS license is no longer an issue), or b. the GeoUtils classes go into spatial3d (renaming still seems appropriate) and the Lucene Query and Field classes go into the spatial module. I'm open to thoughts or other ideas.
        Hide
        Michael McCandless added a comment -

        +1 to graduate the postings based geo impls from sandbox.

        +1 to graduate into spatial3d and then rename spatial3d to spatial2 or spatiallite or something, but I think renaming can come later / be decoupled from graduating.

        It's wonderful JTS finally has a friendly license, but it's still an external dependency which makes spatial module heavy imo, and the numerous abstractions in spatial are also heavy. So far Nicholas Knize's geo sandbox impls have been kept relatively straightforward by comparison, and I think we should try to keep it that way.

        Show
        Michael McCandless added a comment - +1 to graduate the postings based geo impls from sandbox. +1 to graduate into spatial3d and then rename spatial3d to spatial2 or spatiallite or something, but I think renaming can come later / be decoupled from graduating. It's wonderful JTS finally has a friendly license, but it's still an external dependency which makes spatial module heavy imo, and the numerous abstractions in spatial are also heavy. So far Nicholas Knize 's geo sandbox impls have been kept relatively straightforward by comparison, and I think we should try to keep it that way.
        Hide
        David Smiley added a comment -

        It seems this has more to do with the presence of external dependencies than trying to define what "lite" is vs "heavy" (very contentious / subjective), and enumerating the number of abstractions. Geo3d has a ton of abstractions (I forget them often) yet we're not advocating moving them into lucene-spatial.

        Geo3d is a gem. By itself, it depends on nothing but Java; it's not a Lucene thing, its a math thing . So long as Geo3d stays with us hosted in our project, I really strongly want to see it remain in its own module – and it mostly is right now, notwithstanding some Lucene bits that IMO really ought to be separated either by package and module. This enables other projects out there to use it more easily by itself, and to some extent helps with establishing its identity. That identity would be lost if it was mixed into a jar file of unrelated stuff. As it is, it's jar/module is "spatial3d" yet has package (and original name) as "geo3d". That'll make finding it harder for people in the wild who are looking for it and not Lucene.

        Here's a compromise proposal with respect to non-Geo3d stuff. Perhaps have the current spatial module be renamed to "lucene-spatial-spatial4j" and then all the other cool spatial stuff you two have been working on goes into the name "lucene-spatial" (the current name). This frees us from subjective discussions about what constitutes "heavy" vs "light" or "advanced" vs "simple" – you're free to do whatever "advanced" (non-lite) stuff you want in the new lucene-spatial module. I do suggest we keep that module dependency-free (other than Lucene of course). That could be the main differentiator – dependencies or no dependencies. I'm not in love with the module name "lucene-spatial-spatial4j"... maybe "lucene-spatial-deps" or -s4j or I dunno. IMO the Lucene bits in the spatial3d module should go here too as I view Geo3d as a standalone thing and not Lucene. This module would depend on spatial3d. What do you think guys?

        Show
        David Smiley added a comment - It seems this has more to do with the presence of external dependencies than trying to define what "lite" is vs "heavy" (very contentious / subjective), and enumerating the number of abstractions. Geo3d has a ton of abstractions (I forget them often) yet we're not advocating moving them into lucene-spatial. Geo3d is a gem. By itself, it depends on nothing but Java; it's not a Lucene thing, its a math thing . So long as Geo3d stays with us hosted in our project, I really strongly want to see it remain in its own module – and it mostly is right now, notwithstanding some Lucene bits that IMO really ought to be separated either by package and module. This enables other projects out there to use it more easily by itself, and to some extent helps with establishing its identity. That identity would be lost if it was mixed into a jar file of unrelated stuff. As it is, it's jar/module is "spatial3d" yet has package (and original name) as "geo3d". That'll make finding it harder for people in the wild who are looking for it and not Lucene. Here's a compromise proposal with respect to non-Geo3d stuff. Perhaps have the current spatial module be renamed to "lucene-spatial-spatial4j" and then all the other cool spatial stuff you two have been working on goes into the name "lucene-spatial" (the current name). This frees us from subjective discussions about what constitutes "heavy" vs "light" or "advanced" vs "simple" – you're free to do whatever "advanced" (non-lite) stuff you want in the new lucene-spatial module. I do suggest we keep that module dependency-free (other than Lucene of course). That could be the main differentiator – dependencies or no dependencies. I'm not in love with the module name "lucene-spatial-spatial4j"... maybe "lucene-spatial-deps" or -s4j or I dunno. IMO the Lucene bits in the spatial3d module should go here too as I view Geo3d as a standalone thing and not Lucene. This module would depend on spatial3d. What do you think guys?
        Hide
        David Smiley added a comment -

        or "lucene-spatial-extras" maybe

        Show
        David Smiley added a comment - or "lucene-spatial-extras" maybe
        Hide
        Nicholas Knize added a comment -

        It seems this has more to do with the presence of external dependencies than trying to define what "lite" is vs "heavy"

        agree!

        I think this is a great compromise. Let me make sure I understand the changes (slightly different but same result):

        • rename spatial3d to lucene-spatial-extras (unless someone suggests a better name I'm good with this).
        • move existing s4j implementations to lucene-spatial-extras (which includes geo3d)
        • move sandbox spatial code to lucene-spatial and require it remain dependency free
        Show
        Nicholas Knize added a comment - It seems this has more to do with the presence of external dependencies than trying to define what "lite" is vs "heavy" agree! I think this is a great compromise. Let me make sure I understand the changes (slightly different but same result): rename spatial3d to lucene-spatial-extras (unless someone suggests a better name I'm good with this). move existing s4j implementations to lucene-spatial-extras (which includes geo3d) move sandbox spatial code to lucene-spatial and require it remain dependency free
        Hide
        David Smiley added a comment -

        The last 2 bullets are mostly what I meant but the first isn't. Let me re-state to clarify:

        • spatial3d stays put, for now/this-issue.
        • rename "lucene-spatial" module to "lucene-spatial-extras".
        • move sandbox spatial code to lucene-spatial and require it remain dependency free (can only depend on other Lucene modules(s) but not geo3d or lucene-spatial-extras)

        In a separate issue, I propose the Lucene parts of spatial3d (2 classes + applicable tests?) go to lucene-spatial-extras; probably with a package change. The pom for it would then have zero compile dependencies (not even Lucene!). It'd be good to revisit the name of the module as well and reconcile that with the package name which is currently inconsistent.

        Show
        David Smiley added a comment - The last 2 bullets are mostly what I meant but the first isn't. Let me re-state to clarify: spatial3d stays put, for now/this-issue . rename "lucene-spatial" module to "lucene-spatial-extras". move sandbox spatial code to lucene-spatial and require it remain dependency free (can only depend on other Lucene modules(s) but not geo3d or lucene-spatial-extras) In a separate issue, I propose the Lucene parts of spatial3d (2 classes + applicable tests?) go to lucene-spatial-extras; probably with a package change. The pom for it would then have zero compile dependencies (not even Lucene!). It'd be good to revisit the name of the module as well and reconcile that with the package name which is currently inconsistent.
        Hide
        Michael McCandless added a comment -

        +1 for this plan! I agree we should handle spatial3d in a separate issue ...

        Show
        Michael McCandless added a comment - +1 for this plan! I agree we should handle spatial3d in a separate issue ...
        Hide
        Nicholas Knize added a comment -

        +1 as well! A separate issue for geo3d sounds like the right move to me.

        I'd like to work this before committing LUCENE-6930, any objections?

        Show
        Nicholas Knize added a comment - +1 as well! A separate issue for geo3d sounds like the right move to me. I'd like to work this before committing LUCENE-6930 , any objections?
        Hide
        Nicholas Knize added a comment -

        Attached patch provides the following changes:

        • refactors spatial module to new spatial-extras module
        • refactors GeoPoint* classes from sandbox module to spatial module
        • adds intellij .iml files to dev-tools
        • updates ant build scripts, and maven pom files to build new module
        • updates sandbox build to depend on spatial module so that sandboxed BKD point classes and tests can remain sandboxed until they're ready to graduate

        A few notes:

        • I'm not an ant or maven expert, so I'll need someone to verify the changes. Steve Rowe? precommit generate-maven-poms idea all work
        • For collaboration I've created a new lucene-6997 feature branch that includes all of these changes.
        Show
        Nicholas Knize added a comment - Attached patch provides the following changes: refactors spatial module to new spatial-extras module refactors GeoPoint* classes from sandbox module to spatial module adds intellij .iml files to dev-tools updates ant build scripts, and maven pom files to build new module updates sandbox build to depend on spatial module so that sandboxed BKD point classes and tests can remain sandboxed until they're ready to graduate A few notes: I'm not an ant or maven expert, so I'll need someone to verify the changes. Steve Rowe ? precommit generate-maven-poms idea all work For collaboration I've created a new lucene-6997 feature branch that includes all of these changes.
        Hide
        Steve Rowe added a comment -

        Hi Nick, I skimmed the patch and noticed a few things:

        • You have some non-related whitespace changes in various files, very likely introduced by your IDE. Please change the appropriate setting so that it only auto-trims whitespace on changed lines.
        • In dev-tools/idea/lucene/spatial/spatial.iml, you removed scope="TEST" from the lucene-test-framework module dependency - why?:
          -    <orderEntry type="module" scope="TEST" module-name="lucene-test-framework" />
          [...]
          +    <orderEntry type="module" module-name="lucene-test-framework" />
          
        • AFAICT, the benchmark module no longer depends on the spatial module, so I think you can remove the jar-spatial target from the init target's list of dependencies In lucene/benchmark/build.xml?:
          -    <target name="init" depends="module-build.init,jar-memory,jar-highlighter,jar-analyzers-common,jar-queryparser,jar-facet,jar-spatial,jar-codecs,jar-join"/>
          +    <target name="init" depends="module-build.init,jar-memory,jar-highlighter,jar-analyzers-common,jar-queryparser,jar-facet,jar-spatial,jar-spatial-extras,jar-codecs,jar-join"/>
          
        • The spatial-compile-test target you added to lucene/module-build.xml is only used in the benchmark module, so maybe it could move there? That's the strategy used by the build for the dataimporthandler-extras module, which (alone) depends on the dataimporthandler module's tests. A nit: most other module-test-building targets (see solr/common-build.xml) are named compile-test-<module>, not <module>-compile-test:
          +  <target name="spatial-compile-test" depends="init" if="module.has.tests">
          +    <ant dir="${common.dir}/spatial" target="compile-test" inheritAll="false"/>
          +  </target>
          
        • Other than adding a POM template for the spatial-extras module, and including it in the Lucene parent POM's list of sub-modules to build, you didn't modify any inter-module dependencies in other POM templates, and I'm pretty sure there are several changes to make (looking at the Ant and IntelliJ build changes you made).

        Later today I'll try to run the IntelliJ and Maven builds.

        Show
        Steve Rowe added a comment - Hi Nick, I skimmed the patch and noticed a few things: You have some non-related whitespace changes in various files, very likely introduced by your IDE. Please change the appropriate setting so that it only auto-trims whitespace on changed lines. In dev-tools/idea/lucene/spatial/spatial.iml , you removed scope="TEST" from the lucene-test-framework module dependency - why?: - <orderEntry type="module" scope="TEST" module-name="lucene-test-framework" /> [...] + <orderEntry type="module" module-name="lucene-test-framework" /> AFAICT, the benchmark module no longer depends on the spatial module, so I think you can remove the jar-spatial target from the init target's list of dependencies In lucene/benchmark/build.xml ?: - <target name="init" depends="module-build.init,jar-memory,jar-highlighter,jar-analyzers-common,jar-queryparser,jar-facet,jar-spatial,jar-codecs,jar-join"/> + <target name="init" depends="module-build.init,jar-memory,jar-highlighter,jar-analyzers-common,jar-queryparser,jar-facet,jar-spatial,jar-spatial-extras,jar-codecs,jar-join"/> The spatial-compile-test target you added to lucene/module-build.xml is only used in the benchmark module, so maybe it could move there? That's the strategy used by the build for the dataimporthandler-extras module, which (alone) depends on the dataimporthandler module's tests. A nit: most other module-test-building targets (see solr/common-build.xml ) are named compile-test-<module> , not <module>-compile-test : + <target name="spatial-compile-test" depends="init" if="module.has.tests"> + <ant dir="${common.dir}/spatial" target="compile-test" inheritAll="false"/> + </target> Other than adding a POM template for the spatial-extras module, and including it in the Lucene parent POM's list of sub-modules to build, you didn't modify any inter-module dependencies in other POM templates, and I'm pretty sure there are several changes to make (looking at the Ant and IntelliJ build changes you made). Later today I'll try to run the IntelliJ and Maven builds.
        Hide
        David Smiley added a comment -

        This package reshuffling strikes me as a 6.0 feature; no? Do we really want to do this sorta thing in a 5.x release? I know you're anxious for it but 6.0 is right around the corner. In the interim (now; perhaps separate issue so it can be assigned fix-for 5.5), you could simply move & commit them to the spatial module. The only thing that would then change in 6.0 would be the decoupling of everything that existed before to go into a separate -extras module.

        Show
        David Smiley added a comment - This package reshuffling strikes me as a 6.0 feature; no? Do we really want to do this sorta thing in a 5.x release? I know you're anxious for it but 6.0 is right around the corner. In the interim (now; perhaps separate issue so it can be assigned fix-for 5.5), you could simply move & commit them to the spatial module. The only thing that would then change in 6.0 would be the decoupling of everything that existed before to go into a separate -extras module.
        Hide
        Nicholas Knize added a comment -

        I think that's a great suggestion:

        • (This issue / 5.5) Refactor GeoPoint* to existing spatial module
        • (New issue / 6.0) refactor non GeoPoint* to new spatial-extras module

        It makes it easier to manage and review the refactor that way.

        Show
        Nicholas Knize added a comment - I think that's a great suggestion: (This issue / 5.5) Refactor GeoPoint* to existing spatial module (New issue / 6.0) refactor non GeoPoint* to new spatial-extras module It makes it easier to manage and review the refactor that way.
        Hide
        David Smiley added a comment -

        +1 perfect.

        Show
        David Smiley added a comment - +1 perfect.
        Hide
        Michael McCandless added a comment -

        Patch looks nice: I like all the added javadocs, and the public -> private conversions.

        Can we keep this as a rote cutover and not do renames, e.g. I noticed GeoRect become GeoBoundingBox but I think we should keep GeoRect?

        Show
        Michael McCandless added a comment - Patch looks nice: I like all the added javadocs, and the public -> private conversions. Can we keep this as a rote cutover and not do renames, e.g. I noticed GeoRect become GeoBoundingBox but I think we should keep GeoRect ?
        Hide
        Nicholas Knize added a comment -

        Will do. Based on the discussion right above I created a new issue, LUCENE-7015, that will deal with the new module creation and refactoring for 6.0. I will update this patch to simply move GeoPoint* classes from sandbox to existing spatial module.

        Show
        Nicholas Knize added a comment - Will do. Based on the discussion right above I created a new issue, LUCENE-7015 , that will deal with the new module creation and refactoring for 6.0. I will update this patch to simply move GeoPoint* classes from sandbox to existing spatial module.
        Hide
        Nicholas Knize added a comment -

        Thanks Steve Rowe! This is very helpful.

        You have some non-related whitespace changes in various files, very likely introduced by your IDE. Please change the appropriate setting so that it only auto-trims whitespace on changed lines.

        These were actually deliberately changed tab characters that, I think, shouldn't have been there? I replaced them with spaces after they made git diff cranky.

        I also set my .gitconfig as follows

        git config --global core.whitespace trailing-space,space-before-tab
        git config --global apply.whitespace fix
        

        In dev-tools/idea/lucene/spatial/spatial.iml, you removed scope="TEST" from the lucene-test-framework module dependency - why?:

        that's a mistake. Thanks for catching I'll correct.

        ...you didn't modify any inter-module dependencies in other POM templates, and I'm pretty sure there are several changes to make (looking at the Ant and IntelliJ build changes you made).

        This (and ant) is where I'll likely need to most help. Re: maven I only ran ant generate-maven-artifacts and it was happy. But that doesn't mean I didn't miss other unhappy targets.

        Thanks for helping. I'll be making the bulk of these changes in LUCENE-7015 for 6.0.

        Show
        Nicholas Knize added a comment - Thanks Steve Rowe ! This is very helpful. You have some non-related whitespace changes in various files, very likely introduced by your IDE. Please change the appropriate setting so that it only auto-trims whitespace on changed lines. These were actually deliberately changed tab characters that, I think, shouldn't have been there? I replaced them with spaces after they made git diff cranky. I also set my .gitconfig as follows git config --global core.whitespace trailing-space,space-before-tab git config --global apply.whitespace fix In dev-tools/idea/lucene/spatial/spatial.iml, you removed scope="TEST" from the lucene-test-framework module dependency - why?: that's a mistake. Thanks for catching I'll correct. ...you didn't modify any inter-module dependencies in other POM templates, and I'm pretty sure there are several changes to make (looking at the Ant and IntelliJ build changes you made). This (and ant) is where I'll likely need to most help. Re: maven I only ran ant generate-maven-artifacts and it was happy. But that doesn't mean I didn't miss other unhappy targets. Thanks for helping. I'll be making the bulk of these changes in LUCENE-7015 for 6.0.
        Hide
        Uwe Schindler added a comment -

        These were actually deliberately changed tab characters that, I think, shouldn't have been there? I replaced them with spaces after they made git diff cranky.

        They should have failed precommit already! If you run ant validate-source-patterns, the tabs will fail the build.

        Show
        Uwe Schindler added a comment - These were actually deliberately changed tab characters that, I think, shouldn't have been there? I replaced them with spaces after they made git diff cranky. They should have failed precommit already! If you run ant validate-source-patterns , the tabs will fail the build.
        Hide
        Uwe Schindler added a comment -

        I know: Only in pom.xml.template files. They are not checked by precommit for tabs!

        Show
        Uwe Schindler added a comment - I know: Only in pom.xml.template files. They are not checked by precommit for tabs!
        Hide
        Steve Rowe added a comment -

        Okay, I'll watch LUCENE-7015.

        ...you didn't modify any inter-module dependencies in other POM templates, and I'm pretty sure there are several changes to make (looking at the Ant and IntelliJ build changes you made).

        This (and ant) is where I'll likely need to most help. Re: maven I only ran ant generate-maven-artifacts and it was happy. But that doesn't mean I didn't miss other unhappy targets.

        I forgot about the maven intermodule dependencies that these should be picked up automatically when the POM templates are filled out. There is one exception though: dependencies on other modules' tests is not checked for, so when modules need this, we modify the depended-on module's POM to build a test-jar, which is then manually added to the depending module. I'll take care of it.

        Show
        Steve Rowe added a comment - Okay, I'll watch LUCENE-7015 . ...you didn't modify any inter-module dependencies in other POM templates, and I'm pretty sure there are several changes to make (looking at the Ant and IntelliJ build changes you made). This (and ant) is where I'll likely need to most help. Re: maven I only ran ant generate-maven-artifacts and it was happy. But that doesn't mean I didn't miss other unhappy targets. I forgot about the maven intermodule dependencies that these should be picked up automatically when the POM templates are filled out. There is one exception though: dependencies on other modules' tests is not checked for, so when modules need this, we modify the depended-on module's POM to build a test-jar, which is then manually added to the depending module. I'll take care of it.
        Hide
        ASF subversion and git services added a comment -

        Commit 50a2f7549eb72be3a29cb2e391a9b4db63ee2e33 in lucene-solr's branch refs/heads/lucene-6997 from nknize
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50a2f75 ]

        LUCENE-6997: refactors lucene-spatial module to a new lucene-spatial-extras module, and refactors sandbox GeoPointField and queries to lucene-spatial module

        Show
        ASF subversion and git services added a comment - Commit 50a2f7549eb72be3a29cb2e391a9b4db63ee2e33 in lucene-solr's branch refs/heads/lucene-6997 from nknize [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50a2f75 ] LUCENE-6997 : refactors lucene-spatial module to a new lucene-spatial-extras module, and refactors sandbox GeoPointField and queries to lucene-spatial module
        Hide
        ASF subversion and git services added a comment -

        Commit 50a2f7549eb72be3a29cb2e391a9b4db63ee2e33 in lucene-solr's branch refs/heads/lucene-7015 from nknize
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50a2f75 ]

        LUCENE-6997: refactors lucene-spatial module to a new lucene-spatial-extras module, and refactors sandbox GeoPointField and queries to lucene-spatial module

        Show
        ASF subversion and git services added a comment - Commit 50a2f7549eb72be3a29cb2e391a9b4db63ee2e33 in lucene-solr's branch refs/heads/lucene-7015 from nknize [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=50a2f75 ] LUCENE-6997 : refactors lucene-spatial module to a new lucene-spatial-extras module, and refactors sandbox GeoPointField and queries to lucene-spatial module
        Hide
        ASF subversion and git services added a comment -

        Commit 665041c52f088310c5bd3414607aa683a1d8813b in lucene-solr's branch refs/heads/lucene-6997 from nknize
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=665041c ]

        LUCENE-6997: refactor sandboxed GeoPointField and query classes to lucene-spatial module

        Show
        ASF subversion and git services added a comment - Commit 665041c52f088310c5bd3414607aa683a1d8813b in lucene-solr's branch refs/heads/lucene-6997 from nknize [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=665041c ] LUCENE-6997 : refactor sandboxed GeoPointField and query classes to lucene-spatial module
        Hide
        Nicholas Knize added a comment -

        Updated patch:

        • Moves the creation and refactor of spatial-extras to LUCENE-7015
        • Only includes refactoring of GeoPoint* classes from sandbox to spatial module
        • Updates sandbox build and project iml file to depend on compile-test-spatial target
        • Updates and adds javadocs to pass documentation-lint target

        I think this is close, if not ready. The idea generate-maven-artifacts and precommit targets all pass. I also updated the feature branch.

        Show
        Nicholas Knize added a comment - Updated patch: Moves the creation and refactor of spatial-extras to LUCENE-7015 Only includes refactoring of GeoPoint* classes from sandbox to spatial module Updates sandbox build and project iml file to depend on compile-test-spatial target Updates and adds javadocs to pass documentation-lint target I think this is close, if not ready. The idea generate-maven-artifacts and precommit targets all pass. I also updated the feature branch.
        Hide
        David Smiley added a comment -

        +1 looks good to me.

        Show
        David Smiley added a comment - +1 looks good to me.
        Hide
        ASF subversion and git services added a comment -

        Commit 665041c52f088310c5bd3414607aa683a1d8813b in lucene-solr's branch refs/heads/master from nknize
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=665041c ]

        LUCENE-6997: refactor sandboxed GeoPointField and query classes to lucene-spatial module

        Show
        ASF subversion and git services added a comment - Commit 665041c52f088310c5bd3414607aa683a1d8813b in lucene-solr's branch refs/heads/master from nknize [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=665041c ] LUCENE-6997 : refactor sandboxed GeoPointField and query classes to lucene-spatial module
        Hide
        ASF subversion and git services added a comment -

        Commit 187fe8766a0d886499c0de5cbf61f459054002e9 in lucene-solr's branch refs/heads/branch_5x from nknize
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=187fe87 ]

        LUCENE-6997: refactor sandboxed GeoPointField and query classes to lucene-spatial module

        Show
        ASF subversion and git services added a comment - Commit 187fe8766a0d886499c0de5cbf61f459054002e9 in lucene-solr's branch refs/heads/branch_5x from nknize [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=187fe87 ] LUCENE-6997 : refactor sandboxed GeoPointField and query classes to lucene-spatial module
        Hide
        David Smiley added a comment -

        Nick, I have one parting thought before this gets cemented into 5.5. The package structure you've introduced here is:

        • org.apache.lucene.spatial.document (only public class is GeoPointField)
        • org.apache.lucene.spatial.search (lots of classes)

        If the only spatial indexing approach that were to exist were this one, then this is just fine. But there are in fact other approaches, and we are bound to come up with more. (BTW this is partly the motivation of the SpatialStrategy abstraction) With each approach, the Queries can only be used with a particular Field implementation. Imagine adding Geo3DPoint to your package structure above, and the Query side as well. It starts to become confusing for people to navigate and know what goes with what. I would certainly be confused; it all sounds so similar. Instead, I suggest a org.apache.lucene.spatial.geopoint package to put all the classes associated with this indexing approach. Perhaps you then feel it's useful to add a document and search sub-package; I don't but I leave that consideration to you. What do you think?

        Show
        David Smiley added a comment - Nick, I have one parting thought before this gets cemented into 5.5. The package structure you've introduced here is: org.apache.lucene.spatial.document (only public class is GeoPointField) org.apache.lucene.spatial.search (lots of classes) If the only spatial indexing approach that were to exist were this one, then this is just fine. But there are in fact other approaches, and we are bound to come up with more. (BTW this is partly the motivation of the SpatialStrategy abstraction) With each approach, the Queries can only be used with a particular Field implementation. Imagine adding Geo3DPoint to your package structure above, and the Query side as well. It starts to become confusing for people to navigate and know what goes with what. I would certainly be confused; it all sounds so similar. Instead, I suggest a org.apache.lucene.spatial.geopoint package to put all the classes associated with this indexing approach. Perhaps you then feel it's useful to add a document and search sub-package; I don't but I leave that consideration to you. What do you think?
        Hide
        Nicholas Knize added a comment -

        +1

        With each approach, the Queries can only be used with a particular Field implementation.

        This definitely warrants an organized package hierarchy. With the existing flat structure it implies queries can be used with any document.*Field implementation. Good idea, I'll make the change!

        Show
        Nicholas Knize added a comment - +1 With each approach, the Queries can only be used with a particular Field implementation. This definitely warrants an organized package hierarchy. With the existing flat structure it implies queries can be used with any document.*Field implementation. Good idea, I'll make the change!
        Hide
        Nicholas Knize added a comment - - edited

        Updated patch to refactor GeoPoint* classes from org.apache.lucene.spatial to org.apache.lucene.spatial.geopoint

        Show
        Nicholas Knize added a comment - - edited Updated patch to refactor GeoPoint* classes from org.apache.lucene.spatial to org.apache.lucene.spatial.geopoint
        Hide
        David Smiley added a comment -

        +1 to the patch. I looked it over quickly and it's of course very straight-forward.

        Show
        David Smiley added a comment - +1 to the patch. I looked it over quickly and it's of course very straight-forward.
        Hide
        ASF subversion and git services added a comment -

        Commit 7d8f0127b7e8514b19c2898dbaac2d1419964b1b in lucene-solr's branch refs/heads/master from nknize
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d8f012 ]

        LUCENE-6997: refactor GeoPointField and query classes from lucene.spatial to lucene.spatial.geopoint package

        Show
        ASF subversion and git services added a comment - Commit 7d8f0127b7e8514b19c2898dbaac2d1419964b1b in lucene-solr's branch refs/heads/master from nknize [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d8f012 ] LUCENE-6997 : refactor GeoPointField and query classes from lucene.spatial to lucene.spatial.geopoint package
        Hide
        ASF subversion and git services added a comment -

        Commit 2b2c516477f25effbd5fab39de4d33e576cb71d1 in lucene-solr's branch refs/heads/branch_5x from nknize
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2b2c516 ]

        LUCENE-6997: refactor GeoPointField and query classes from lucene.spatial to lucene.spatial.geopoint package

        Show
        ASF subversion and git services added a comment - Commit 2b2c516477f25effbd5fab39de4d33e576cb71d1 in lucene-solr's branch refs/heads/branch_5x from nknize [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2b2c516 ] LUCENE-6997 : refactor GeoPointField and query classes from lucene.spatial to lucene.spatial.geopoint package
        Hide
        ASF subversion and git services added a comment -

        Commit 8e0f7ff7bbe202e815a4c3f521871fef7f0e45b4 in lucene-solr's branch refs/heads/master from Steve Rowe
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8e0f7ff ]

        LUCENE-6997: Maven config: build a spatial module test-jar, and add a test dependency on it to the sandbox module

        Show
        ASF subversion and git services added a comment - Commit 8e0f7ff7bbe202e815a4c3f521871fef7f0e45b4 in lucene-solr's branch refs/heads/master from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8e0f7ff ] LUCENE-6997 : Maven config: build a spatial module test-jar, and add a test dependency on it to the sandbox module
        Hide
        ASF subversion and git services added a comment -

        Commit a900e5fdf49037fc409f032f8cb2c72f11eb41e9 in lucene-solr's branch refs/heads/branch_5x from Steve Rowe
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a900e5f ]

        LUCENE-6997: Maven config: build a spatial module test-jar, and add a test dependency on it to the sandbox module

        Show
        ASF subversion and git services added a comment - Commit a900e5fdf49037fc409f032f8cb2c72f11eb41e9 in lucene-solr's branch refs/heads/branch_5x from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a900e5f ] LUCENE-6997 : Maven config: build a spatial module test-jar, and add a test dependency on it to the sandbox module
        Hide
        ASF subversion and git services added a comment -

        Commit 02dee401cc14191a8b95bb644794a7d40c0163a4 in lucene-solr's branch refs/heads/branch_5_5 from Steve Rowe
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=02dee40 ]

        LUCENE-6997: Maven config: build a spatial module test-jar, and add a test dependency on it to the sandbox module

        Show
        ASF subversion and git services added a comment - Commit 02dee401cc14191a8b95bb644794a7d40c0163a4 in lucene-solr's branch refs/heads/branch_5_5 from Steve Rowe [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=02dee40 ] LUCENE-6997 : Maven config: build a spatial module test-jar, and add a test dependency on it to the sandbox module

          People

          • Assignee:
            Nicholas Knize
            Reporter:
            Nicholas Knize
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development