[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144


##
site/_docs/howto.md:
##
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
   that the `META-INF` directory contains `LICENSE`,
   `NOTICE`
 * Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,

Review Comment:
   Yes, this is a good idea. Would adding something like `if ./gradlew 
dependencies | grep -q proj4j; then exit 1; else exit 0; fi` to the CI be 
sufficient? Should it be a dedicated github actions job?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144


##
site/_docs/howto.md:
##
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
   that the `META-INF` directory contains `LICENSE`,
   `NOTICE`
 * Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,

Review Comment:
   Yes, this is a good idea. Would adding something like `if ./gradlew 
dependencies | grep -q proj4j; then exit 1; else exit 0; fi` in a workflow be 
sufficient? Should it be a dedicated github actions job?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144


##
site/_docs/howto.md:
##
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
   that the `META-INF` directory contains `LICENSE`,
   `NOTICE`
 * Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,

Review Comment:
   Yes, this is a good idea. Would adding something like `if ./gradlew 
dependencies | grep -q proj4j; then exit 1; else exit 0; fi` in a workflow be 
sufficient? Should it be a dedicated job?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034935144


##
site/_docs/howto.md:
##
@@ -841,6 +841,8 @@ your key to the keyservers used by Nexus, see above.
   that the `META-INF` directory contains `LICENSE`,
   `NOTICE`
 * Check PGP, per [this](https://httpd.apache.org/dev/verification.html)
+* Check that Proj4J is not among the api and implementation dependencies,

Review Comment:
   Yes, this is a good idea. Would adding something like `if ./gradlew 
dependencies | grep -q proj4j; then exit 1; else exit 0; fi` in a workflow be 
sufficient?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034797722


##
core/build.gradle.kts:
##
@@ -57,6 +54,11 @@ dependencies {
 api("org.checkerframework:checker-qual")
 api("org.slf4j:slf4j-api")
 
+api("org.locationtech.jts:jts-core")
+api("org.locationtech.jts.io:jts-io-common")
+compileOnly("org.locationtech.proj4j:proj4j")
+testImplementation("org.locationtech.proj4j:proj4j")

Review Comment:
   > Maybe you did this already but if not it would be nice to create a very 
simple maven project that depends on `calcite-core` and uses the Spatial 
extension to verify that indeed you get the desired `ClassNotFoundException`.
   
   Good idea, I created a simple project, and it confirms that everything 
behave as expected. 
   
   https://github.com/bchapuis/calcite-proj4j
   
   Here is the dependency tree without the proj4j version 1.1.5 dependency:
   
   ```
   [INFO] org.example:calcite-proj4j:jar:1.0-SNAPSHOT
   [INFO] \- org.apache.calcite:calcite-core:jar:1.33.0-SNAPSHOT:compile
   [INFO]+- org.apache.calcite:calcite-linq4j:jar:1.33.0-SNAPSHOT:compile
   [INFO]+- 
com.fasterxml.jackson.core:jackson-annotations:jar:2.14.0:compile
   [INFO]+- com.google.errorprone:error_prone_annotations:jar:2.11.0:compile
   [INFO]+- com.google.guava:guava:jar:31.1-jre:compile
   [INFO]|  +- com.google.guava:failureaccess:jar:1.0.1:compile
   [INFO]|  +- 
com.google.guava:listenablefuture:jar:.0-empty-to-avoid-conflict-with-guava:compile
   [INFO]|  +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
   [INFO]|  \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
   [INFO]+- org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
   [INFO]|  +- org.apache.calcite.avatica:avatica-metrics:jar:1.22.0:compile
   [INFO]|  +- com.google.protobuf:protobuf-java:jar:3.17.1:compile
   [INFO]|  +- 
org.apache.httpcomponents.client5:httpclient5:jar:5.1.3:runtime
   [INFO]|  |  \- 
org.apache.httpcomponents.core5:httpcore5-h2:jar:5.1.3:runtime
   [INFO]|  \- org.apache.httpcomponents.core5:httpcore5:jar:5.1.3:runtime
   [INFO]+- org.apiguardian:apiguardian-api:jar:1.1.2:compile
   [INFO]+- org.checkerframework:checker-qual:jar:3.12.0:compile
   [INFO]+- org.slf4j:slf4j-api:jar:1.7.33:compile
   [INFO]+- org.locationtech.jts:jts-core:jar:1.19.0:compile
   [INFO]+- org.locationtech.jts.io:jts-io-common:jar:1.19.0:compile
   [INFO]|  \- com.googlecode.json-simple:json-simple:jar:1.1.1:compile
   [INFO]+- com.fasterxml.jackson.core:jackson-core:jar:2.14.0:compile
   [INFO]+- com.fasterxml.jackson.core:jackson-databind:jar:2.14.0:compile
   [INFO]+- 
com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.14.0:runtime
   [INFO]|  \- org.yaml:snakeyaml:jar:1.33:runtime
   [INFO]+- com.google.uzaygezen:uzaygezen-core:jar:0.2:runtime
   [INFO]+- com.jayway.jsonpath:json-path:jar:2.7.0:runtime
   [INFO]|  \- net.minidev:json-smart:jar:2.4.7:runtime
   [INFO]| \- net.minidev:accessors-smart:jar:2.4.7:runtime
   [INFO]|\- org.ow2.asm:asm:jar:9.1:runtime
   [INFO]+- com.yahoo.datasketches:sketches-core:jar:0.9.0:runtime
   [INFO]|  \- com.yahoo.datasketches:memory:jar:0.9.0:runtime
   [INFO]+- commons-codec:commons-codec:jar:1.13:runtime
   [INFO]+- net.hydromatic:aggdesigner-algorithm:jar:6.0:runtime
   [INFO]|  +- commons-lang:commons-lang:jar:2.4:runtime
   [INFO]|  \- commons-logging:commons-logging:jar:1.1.3:runtime
   [INFO]+- org.apache.commons:commons-dbcp2:jar:2.6.0:runtime
   [INFO]|  \- org.apache.commons:commons-pool2:jar:2.6.1:runtime
   [INFO]+- org.apache.commons:commons-lang3:jar:3.8:runtime
   [INFO]+- org.apache.commons:commons-math3:jar:3.6.1:runtime
   [INFO]+- commons-io:commons-io:jar:2.11.0:runtime
   [INFO]+- org.codehaus.janino:commons-compiler:jar:3.1.8:runtime
   [INFO]\- org.codehaus.janino:janino:jar:3.1.8:runtime
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034797722


##
core/build.gradle.kts:
##
@@ -57,6 +54,11 @@ dependencies {
 api("org.checkerframework:checker-qual")
 api("org.slf4j:slf4j-api")
 
+api("org.locationtech.jts:jts-core")
+api("org.locationtech.jts.io:jts-io-common")
+compileOnly("org.locationtech.proj4j:proj4j")
+testImplementation("org.locationtech.proj4j:proj4j")

Review Comment:
   > Maybe you did this already but if not it would be nice to create a very 
simple maven project that depends on `calcite-core` and uses the Spatial 
extension to verify that indeed you get the desired `ClassNotFoundException`.
   
   Good idea, I created a simple project and it confirms that everything behave 
as expected. 
   
   https://github.com/bchapuis/calcite-proj4j
   
   Here is the dependency tree without the proj4j version 1.1.5 dependency:
   
   ```
   [INFO] org.example:calcite-proj4j:jar:1.0-SNAPSHOT
   [INFO] \- org.apache.calcite:calcite-core:jar:1.33.0-SNAPSHOT:compile
   [INFO]+- org.apache.calcite:calcite-linq4j:jar:1.33.0-SNAPSHOT:compile
   [INFO]+- 
com.fasterxml.jackson.core:jackson-annotations:jar:2.14.0:compile
   [INFO]+- com.google.errorprone:error_prone_annotations:jar:2.11.0:compile
   [INFO]+- com.google.guava:guava:jar:31.1-jre:compile
   [INFO]|  +- com.google.guava:failureaccess:jar:1.0.1:compile
   [INFO]|  +- 
com.google.guava:listenablefuture:jar:.0-empty-to-avoid-conflict-with-guava:compile
   [INFO]|  +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
   [INFO]|  \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
   [INFO]+- org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
   [INFO]|  +- org.apache.calcite.avatica:avatica-metrics:jar:1.22.0:compile
   [INFO]|  +- com.google.protobuf:protobuf-java:jar:3.17.1:compile
   [INFO]|  +- 
org.apache.httpcomponents.client5:httpclient5:jar:5.1.3:runtime
   [INFO]|  |  \- 
org.apache.httpcomponents.core5:httpcore5-h2:jar:5.1.3:runtime
   [INFO]|  \- org.apache.httpcomponents.core5:httpcore5:jar:5.1.3:runtime
   [INFO]+- org.apiguardian:apiguardian-api:jar:1.1.2:compile
   [INFO]+- org.checkerframework:checker-qual:jar:3.12.0:compile
   [INFO]+- org.slf4j:slf4j-api:jar:1.7.33:compile
   [INFO]+- org.locationtech.jts:jts-core:jar:1.19.0:compile
   [INFO]+- org.locationtech.jts.io:jts-io-common:jar:1.19.0:compile
   [INFO]|  \- com.googlecode.json-simple:json-simple:jar:1.1.1:compile
   [INFO]+- com.fasterxml.jackson.core:jackson-core:jar:2.14.0:compile
   [INFO]+- com.fasterxml.jackson.core:jackson-databind:jar:2.14.0:compile
   [INFO]+- 
com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.14.0:runtime
   [INFO]|  \- org.yaml:snakeyaml:jar:1.33:runtime
   [INFO]+- com.google.uzaygezen:uzaygezen-core:jar:0.2:runtime
   [INFO]+- com.jayway.jsonpath:json-path:jar:2.7.0:runtime
   [INFO]|  \- net.minidev:json-smart:jar:2.4.7:runtime
   [INFO]| \- net.minidev:accessors-smart:jar:2.4.7:runtime
   [INFO]|\- org.ow2.asm:asm:jar:9.1:runtime
   [INFO]+- com.yahoo.datasketches:sketches-core:jar:0.9.0:runtime
   [INFO]|  \- com.yahoo.datasketches:memory:jar:0.9.0:runtime
   [INFO]+- commons-codec:commons-codec:jar:1.13:runtime
   [INFO]+- net.hydromatic:aggdesigner-algorithm:jar:6.0:runtime
   [INFO]|  +- commons-lang:commons-lang:jar:2.4:runtime
   [INFO]|  \- commons-logging:commons-logging:jar:1.1.3:runtime
   [INFO]+- org.apache.commons:commons-dbcp2:jar:2.6.0:runtime
   [INFO]|  \- org.apache.commons:commons-pool2:jar:2.6.1:runtime
   [INFO]+- org.apache.commons:commons-lang3:jar:3.8:runtime
   [INFO]+- org.apache.commons:commons-math3:jar:3.6.1:runtime
   [INFO]+- commons-io:commons-io:jar:2.11.0:runtime
   [INFO]+- org.codehaus.janino:commons-compiler:jar:3.1.8:runtime
   [INFO]\- org.codehaus.janino:janino:jar:3.1.8:runtime
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034797722


##
core/build.gradle.kts:
##
@@ -57,6 +54,11 @@ dependencies {
 api("org.checkerframework:checker-qual")
 api("org.slf4j:slf4j-api")
 
+api("org.locationtech.jts:jts-core")
+api("org.locationtech.jts.io:jts-io-common")
+compileOnly("org.locationtech.proj4j:proj4j")
+testImplementation("org.locationtech.proj4j:proj4j")

Review Comment:
   > Maybe you did this already but if not it would be nice to create a very 
simple maven project that depends on `calcite-core` and uses the Spatial 
extension to verify that indeed you get the desired `ClassNotFoundException`.
   
   Good idea, I create a simple project and it confirms that everything behave 
as expected. 
   
   https://github.com/bchapuis/calcite-proj4j
   
   Here is the dependency tree without the proj4j version 1.1.5 dependency:
   
   ```
   [INFO] org.example:calcite-proj4j:jar:1.0-SNAPSHOT
   [INFO] \- org.apache.calcite:calcite-core:jar:1.33.0-SNAPSHOT:compile
   [INFO]+- org.apache.calcite:calcite-linq4j:jar:1.33.0-SNAPSHOT:compile
   [INFO]+- 
com.fasterxml.jackson.core:jackson-annotations:jar:2.14.0:compile
   [INFO]+- com.google.errorprone:error_prone_annotations:jar:2.11.0:compile
   [INFO]+- com.google.guava:guava:jar:31.1-jre:compile
   [INFO]|  +- com.google.guava:failureaccess:jar:1.0.1:compile
   [INFO]|  +- 
com.google.guava:listenablefuture:jar:.0-empty-to-avoid-conflict-with-guava:compile
   [INFO]|  +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
   [INFO]|  \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
   [INFO]+- org.apache.calcite.avatica:avatica-core:jar:1.22.0:compile
   [INFO]|  +- org.apache.calcite.avatica:avatica-metrics:jar:1.22.0:compile
   [INFO]|  +- com.google.protobuf:protobuf-java:jar:3.17.1:compile
   [INFO]|  +- 
org.apache.httpcomponents.client5:httpclient5:jar:5.1.3:runtime
   [INFO]|  |  \- 
org.apache.httpcomponents.core5:httpcore5-h2:jar:5.1.3:runtime
   [INFO]|  \- org.apache.httpcomponents.core5:httpcore5:jar:5.1.3:runtime
   [INFO]+- org.apiguardian:apiguardian-api:jar:1.1.2:compile
   [INFO]+- org.checkerframework:checker-qual:jar:3.12.0:compile
   [INFO]+- org.slf4j:slf4j-api:jar:1.7.33:compile
   [INFO]+- org.locationtech.jts:jts-core:jar:1.19.0:compile
   [INFO]+- org.locationtech.jts.io:jts-io-common:jar:1.19.0:compile
   [INFO]|  \- com.googlecode.json-simple:json-simple:jar:1.1.1:compile
   [INFO]+- com.fasterxml.jackson.core:jackson-core:jar:2.14.0:compile
   [INFO]+- com.fasterxml.jackson.core:jackson-databind:jar:2.14.0:compile
   [INFO]+- 
com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.14.0:runtime
   [INFO]|  \- org.yaml:snakeyaml:jar:1.33:runtime
   [INFO]+- com.google.uzaygezen:uzaygezen-core:jar:0.2:runtime
   [INFO]+- com.jayway.jsonpath:json-path:jar:2.7.0:runtime
   [INFO]|  \- net.minidev:json-smart:jar:2.4.7:runtime
   [INFO]| \- net.minidev:accessors-smart:jar:2.4.7:runtime
   [INFO]|\- org.ow2.asm:asm:jar:9.1:runtime
   [INFO]+- com.yahoo.datasketches:sketches-core:jar:0.9.0:runtime
   [INFO]|  \- com.yahoo.datasketches:memory:jar:0.9.0:runtime
   [INFO]+- commons-codec:commons-codec:jar:1.13:runtime
   [INFO]+- net.hydromatic:aggdesigner-algorithm:jar:6.0:runtime
   [INFO]|  +- commons-lang:commons-lang:jar:2.4:runtime
   [INFO]|  \- commons-logging:commons-logging:jar:1.1.3:runtime
   [INFO]+- org.apache.commons:commons-dbcp2:jar:2.6.0:runtime
   [INFO]|  \- org.apache.commons:commons-pool2:jar:2.6.1:runtime
   [INFO]+- org.apache.commons:commons-lang3:jar:3.8:runtime
   [INFO]+- org.apache.commons:commons-math3:jar:3.6.1:runtime
   [INFO]+- commons-io:commons-io:jar:2.11.0:runtime
   [INFO]+- org.codehaus.janino:commons-compiler:jar:3.1.8:runtime
   [INFO]\- org.codehaus.janino:janino:jar:3.1.8:runtime
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034737271


##
core/build.gradle.kts:
##
@@ -57,6 +54,11 @@ dependencies {
 api("org.checkerframework:checker-qual")
 api("org.slf4j:slf4j-api")
 
+api("org.locationtech.jts:jts-core")
+api("org.locationtech.jts.io:jts-io-common")
+compileOnly("org.locationtech.proj4j:proj4j")
+testImplementation("org.locationtech.proj4j:proj4j")

Review Comment:
   I replaced `testImplementation` by `testRuntimeOnly`, however, builds fail 
without `compileOnly`. I also tried to improve grouping but failed at 
identifying a clear pattern (alphabetical order, etc.). I simply moved the 
spatial related dependencies at the end of each group.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [calcite] bchapuis commented on a diff in pull request #2988: [CALCITE-5399] Remove Proj4J from the api dependencies

2022-11-29 Thread GitBox 


bchapuis commented on code in PR #2988:
URL: https://github.com/apache/calcite/pull/2988#discussion_r1034723743


##
core/src/main/java/org/apache/calcite/runtime/ProjectionTransformer.java:
##
@@ -40,6 +40,14 @@
 
 /**
  * Transforms the projection of a geometry.
+ *
+ * This class uses Proj4J to transform the projection of a geometry
+ * and should not be used beyond the scope of the Spatial Type Extensions.
+ * Proj4J is released under the Apache License 2.0, however, it also uses the 
EPSG dataset,
+ * which has restricting https://epsg.org/terms-of-use.html";>terms of 
use.
+ * As a result, Proj4J is not suitable for inclusion in Apache Calcite
+ * and this class will throw {@code ClassNotFoundException}s
+ * if Proj4J is not added to the classpath by the user.

Review Comment:
   This is a good idea, I added notices in `reference.md` and `spatial.md`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@calcite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org