Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-2044360172 No, I hadn't enought time to work on this and the decoupling of the GEOMETRY type from the JTS Geometry class will require additional efforts. https://issues.apache.org/jira/browse/CALCITE-6239 -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
mihaibudiu commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-2043741736 Will this be ready for 1.37? -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
YiwenWu commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1498492864
##
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcSchema.java:
##
@@ -433,6 +433,17 @@ private static RelDataType sqlType(RelDataTypeFactory
typeFactory, int dataType,
typeFactory.createSqlType(SqlTypeName.ANY), true);
}
return typeFactory.createArrayType(component, -1);
+case GEOMETRY:
Review Comment:
According to the elements in `SqlTypeName#JDBC_TYPE_TO_NAME` , whether there
are any cases that can be matched `GEOMETRY`?
##
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##
@@ -1445,6 +1447,21 @@ public static SqlNode toSql(RexLiteral literal) {
// Create a string without specifying a charset
return SqlLiteral.createCharString((String)
castNonNull(literal.getValue2()), POS);
}
+case GEO: {
+ Geometry geom = castNonNull(literal.getValueAs(Geometry.class));
+ switch (typeName) {
Review Comment:
I am not sure that typeName `CHAR` or `VARCHAR` can be obtained under `GEO`
family.
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
YiwenWu commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1498501193
##
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##
@@ -1445,6 +1447,21 @@ public static SqlNode toSql(RexLiteral literal) {
// Create a string without specifying a charset
return SqlLiteral.createCharString((String)
castNonNull(literal.getValue2()), POS);
}
+case GEO: {
+ Geometry geom = castNonNull(literal.getValueAs(Geometry.class));
+ switch (typeName) {
Review Comment:
I am not sure that typeName `CHAR` or `VARCHAR` can be obtained directly
under `GEO` family.
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943793419 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [14 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [84.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943793834 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [84.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943689440 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [84.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943465882 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [84.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943428367 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [84.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668: URL: https://github.com/apache/calcite/pull/3668#discussion_r1489118497 ## core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java: ## @@ -39,9 +39,11 @@ import com.google.common.collect.Interners; import org.checkerframework.checker.nullness.qual.Nullable; +import org.locationtech.jts.geom.Geometry; Review Comment: Yes, here is the jira issue I just created: https://issues.apache.org/jira/browse/CALCITE-6263 -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1942774265 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [85.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
julianhyde commented on code in PR #3668: URL: https://github.com/apache/calcite/pull/3668#discussion_r1488656193 ## core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java: ## @@ -39,9 +39,11 @@ import com.google.common.collect.Interners; import org.checkerframework.checker.nullness.qual.Nullable; +import org.locationtech.jts.geom.Geometry; Review Comment: Can we discuss in jira? Increasing the coupling to JTS is an architectural change. -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1488560237
##
core/src/test/java/org/apache/calcite/sql/test/SqlTypeNameTest.java:
##
@@ -172,7 +173,7 @@ class SqlTypeNameTest {
@Test void testOther() {
SqlTypeName tn =
SqlTypeName.getNameForJdbcType(Types.OTHER);
-assertEquals(null, tn, "OTHER did not map to null");
+assertEquals(OTHER, tn, "OTHER did not map to null");
Review Comment:
I remember modifying this line and found the test cases unclear (and I
forgot to adapt the error message). The javadoc of getNameForJdbcType states:
"corresponding SqlTypeName, or null if the type is not known". As other is kind
of a known type, does it makes sense to adapt the test accordingly? I will
commit this fix for now.
```java
@Test void testOther() {
SqlTypeName tn =
SqlTypeName.getNameForJdbcType(Types.OTHER);
assertEquals(OTHER, tn, "OTHER did not map to OTHER");
}
```
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1488530763
##
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##
@@ -1140,27 +1140,447 @@ public enum SqlKind {
// Spatial functions. They are registered as "user-defined functions" but it
// is convenient to have a "kind" so that we can quickly match them in
planner
// rules.
+ //
+ // One can generate the list of spatial functions using the following
snippet:
+ //
+ // Method[] methods = SpatialTypeFunctions .class.getDeclaredMethods();
+ // Arrays.stream(methods)
+ //.map(java.lang.reflect.Method::getName)
+ //.filter(method -> method.startsWith("ST_"))
+ //.distinct().sorted()
+ //.forEach(method -> {
+ // System.out.println("/** The {@code " + method + "} function. */");
+ // System.out.print(method.toUpperCase());
+ // System.out.print(",\n\n");
+ //});
- /** The {@code ST_DWithin} geo-spatial function. */
+ /** The {@code ST_AddPoint} function. */
+ ST_ADDPOINT,
+
+ /** The {@code ST_AddZ} function. */
+ ST_ADDZ,
+
+ /** The {@code ST_Area} function. */
+ ST_AREA,
+
+ /** The {@code ST_AsBinary} function. */
+ ST_ASBINARY,
+
+ /** The {@code ST_AsEWKB} function. */
+ ST_ASEWKB,
+
+ /** The {@code ST_AsEWKT} function. */
+ ST_ASEWKT,
+
+ /** The {@code ST_AsGML} function. */
+ ST_ASGML,
+
+ /** The {@code ST_AsGeoJSON} function. */
+ ST_ASGEOJSON,
+
+ /** The {@code ST_AsText} function. */
+ ST_ASTEXT,
+
+ /** The {@code ST_AsWKB} function. */
+ ST_ASWKB,
+
+ /** The {@code ST_AsWKT} function. */
+ ST_ASWKT,
+
+ /** The {@code ST_Boundary} function. */
+ ST_BOUNDARY,
+
+ /** The {@code ST_BoundingCircle} function. */
+ ST_BOUNDINGCIRCLE,
+
+ /** The {@code ST_Buffer} function. */
+ ST_BUFFER,
+
+ /** The {@code ST_Centroid} function. */
+ ST_CENTROID,
+
+ /** The {@code ST_ClosestCoordinate} function. */
+ ST_CLOSESTCOORDINATE,
+
+ /** The {@code ST_ClosestPoint} function. */
+ ST_CLOSESTPOINT,
+
+ /** The {@code ST_ConstrainedDelaunay} function. */
+ ST_CONSTRAINEDDELAUNAY,
+
+ /** The {@code ST_Contains} function. */
+ ST_CONTAINS,
+
+ /** The {@code ST_ContainsProperly} function. */
+ ST_CONTAINSPROPERLY,
+
+ /** The {@code ST_ConvexHull} function. */
+ ST_CONVEXHULL,
+
+ /** The {@code ST_CoordDim} function. */
+ ST_COORDDIM,
+
+ /** The {@code ST_CoveredBy} function. */
+ ST_COVEREDBY,
+
+ /** The {@code ST_Covers} function. */
+ ST_COVERS,
+
+ /** The {@code ST_Crosses} function. */
+ ST_CROSSES,
+
+ /** The {@code ST_DWithin} function. */
ST_DWITHIN,
+ /** The {@code ST_Delaunay} function. */
+ ST_DELAUNAY,
+
+ /** The {@code ST_Densify} function. */
+ ST_DENSIFY,
+
+ /** The {@code ST_Difference} function. */
+ ST_DIFFERENCE,
+
+ /** The {@code ST_Dimension} function. */
+ ST_DIMENSION,
+
+ /** The {@code ST_Disjoint} function. */
+ ST_DISJOINT,
+
+ /** The {@code ST_Distance} function. */
+ ST_DISTANCE,
+
+ /** The {@code ST_EndPoint} function. */
+ ST_ENDPOINT,
+
+ /** The {@code ST_Envelope} function. */
+ ST_ENVELOPE,
+
+ /** The {@code ST_EnvelopesIntersect} function. */
+ ST_ENVELOPESINTERSECT,
+
+ /** The {@code ST_Equals} function. */
+ ST_EQUALS,
+
+ /** The {@code ST_Expand} function. */
+ ST_EXPAND,
+
+ /** The {@code ST_Explode} function. */
+ ST_EXPLODE,
+
+ /** The {@code ST_Extent} function. */
+ ST_EXTENT,
+
+ /** The {@code ST_ExteriorRing} function. */
+ ST_EXTERIORRING,
+
+ /** The {@code ST_FlipCoordinates} function. */
+ ST_FLIPCOORDINATES,
+
+ /** The {@code ST_Force2D} function. */
+ ST_FORCE2D,
+
+ /** The {@code ST_Force3D} function. */
+ ST_FORCE3D,
+
+ /** The {@code ST_FurthestCoordinate} function. */
+ ST_FURTHESTCOORDINATE,
+
+ /** The {@code ST_GeomFromEWKB} function. */
+ ST_GEOMFROMEWKB,
+
+ /** The {@code ST_GeomFromEWKT} function. */
+ ST_GEOMFROMEWKT,
+
+ /** The {@code ST_GeomFromGML} function. */
+ ST_GEOMFROMGML,
+
+ /** The {@code ST_GeomFromGeoJSON} function. */
+ ST_GEOMFROMGEOJSON,
+
+ /** The {@code ST_GeomFromText} function. */
+ ST_GEOMFROMTEXT,
+
+ /** The {@code ST_GeomFromWKB} function. */
+ ST_GEOMFROMWKB,
+
+ /** The {@code ST_GeomFromWKT} function. */
+ ST_GEOMFROMWKT,
+
+ /** The {@code ST_GeometryN} function. */
+ ST_GEOMETRYN,
+
+ /** The {@code ST_GeometryType} function. */
+ ST_GEOMETRYTYPE,
+
+ /** The {@code ST_GeometryTypeCode} function. */
+ ST_GEOMETRYTYPECODE,
+
+ /** The {@code ST_Holes} function. */
+ ST_HOLES,
+
+ /** The {@code ST_InteriorRing} function. */
+ ST_INTERIORRING,
+
+ /** The {@code ST_Intersection} function. */
+ ST_INTERSECTION,
+
+ /** The {@code ST_Intersects} function. */
+ ST_INTERSECTS,
+
+ /** The {@code ST_Is3D} function. */
+ ST_IS3D,
+
+ /** The {@code ST_IsClosed} function. */
+ ST_ISCLOSED,
+
+ /** The {@code ST_IsEmpty} function. */
+ ST_ISEMPTY,
+
+ /** The {@code ST_IsRectangle}
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1487873628
##
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##
@@ -39,9 +39,11 @@
import com.google.common.collect.Interners;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
Another option could be to add this type only when JTS is in the classpath.
```java
static {
try {
CLASS_FAMILIES.put(Class.forName("org.locationtech.jts.geom.Geometry"),
SqlTypeFamily.GEO);
} catch (ClassNotFoundException e) {
// JTS is not available in the classpath
}
}
```
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1487852604
##
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##
@@ -39,9 +39,11 @@
import com.google.common.collect.Interners;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
I'm trying to wrap my head around this. From what I understand, calling a
method of RelDataTypeFactoryImpl without JTS would trigger a class not found
exception. As the use of JTS is currently circumscribed to a few spatial type
classes, calcite works just fine without adding a dependency to JTS. Is that
correct?
Given the popularity of JTS in the geospatial world, I wouldn't try to
abstract it completely in JTS. An option could be to use reflection to
dynamically load the Geometry class and provide some sort of fallback if the
class is not in the class path.
```java
public static class GeometryFallback { }
static Class loadGeometryClass() {
try {
return Class.forName("org.locationtech.jts.geom.Geometry");
} catch (ClassNotFoundException e) {
return GeometryFallback.class;
}
}
private static final Map CLASS_FAMILIES =
ImmutableMap.builder()
.put(String.class, SqlTypeFamily.CHARACTER)
// ...
.put(loadGeometryClass(), SqlTypeFamily.GEO)
.build();
```
What do you think?
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668: URL: https://github.com/apache/calcite/pull/3668#discussion_r1487860243 ## core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java: ## @@ -312,7 +312,9 @@ private Expression getConvertExpression( case CHAR: case VARCHAR: return Expressions.call(BuiltInMethod.ST_GEOM_FROM_EWKT.method, operand); - + case BINARY: + case VARBINARY: +return Expressions.call(BuiltInMethod.ST_GEOM_FROM_EWKB.method, operand); Review Comment: Thanks a lot for the pointer, it would definitely make sense to implement these functions: https://postgis.net/docs/ST_AsTWKB.html -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668: URL: https://github.com/apache/calcite/pull/3668#discussion_r1487856703 ## core/src/main/java/org/apache/calcite/sql/dialect/PostgisGeometryDecoder.java: ## @@ -0,0 +1,191 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.calcite.sql.dialect; + +import org.apache.commons.codec.DecoderException; +import org.apache.commons.codec.binary.Hex; + +import org.locationtech.jts.geom.Coordinate; +import org.locationtech.jts.geom.Geometry; +import org.locationtech.jts.geom.GeometryCollection; +import org.locationtech.jts.geom.GeometryFactory; +import org.locationtech.jts.geom.LineString; +import org.locationtech.jts.geom.LinearRing; +import org.locationtech.jts.geom.MultiLineString; +import org.locationtech.jts.geom.MultiPoint; +import org.locationtech.jts.geom.MultiPolygon; +import org.locationtech.jts.geom.Point; +import org.locationtech.jts.geom.Polygon; + +import java.lang.reflect.Array; +import java.nio.ByteBuffer; +import java.nio.ByteOrder; +import java.util.Arrays; + +/** + * A decoder for the PostGIS geometry format. Review Comment: ok, I will place it in org.apache.calcite.util and add some unit tests. -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1487852604
##
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##
@@ -39,9 +39,11 @@
import com.google.common.collect.Interners;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
I'm trying to wrap my head around this. From what I understand, calling a
method of RelDataTypeFactoryImpl without JTS would trigger a class not found
exception. As the use of JTS is currently circumscribed to a few spatial type
classes, calcite works just fine without adding a dependency to JTS. Is that
correct?
Given the popularity of JTS in the geospatial world, I wouldn't try to
abstract it completely in JTS. An option could be to use reflection to
dynamically load the Geometry class and provide some sort of fallback if the
class is not in the class path.
```java
public static class Geometry { }
static Class loadGeometryClass() {
try {
return Class.forName("org.locationtech.jts.geom.Geometry");
} catch (ClassNotFoundException e) {
return Geometry.class;
}
}
private static final Map CLASS_FAMILIES =
ImmutableMap.builder()
.put(String.class, SqlTypeFamily.CHARACTER)
// ...
.put(loadGeometryClass(), SqlTypeFamily.GEO)
.build();
```
What do you think?
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668: URL: https://github.com/apache/calcite/pull/3668#discussion_r1487541098 ## core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcSchema.java: ## @@ -433,6 +433,12 @@ private static RelDataType sqlType(RelDataTypeFactory typeFactory, int dataType, typeFactory.createSqlType(SqlTypeName.ANY), true); } return typeFactory.createArrayType(component, -1); +case OTHER: Review Comment: Yes, the use of OTHER with a "geometry" type name seems specific to the postgresql jdbc driver and postgis. I guess they did this to distinguish postgis geometry types from the regular geometry types implemented in postgresql. https://www.postgresql.org/docs/current/datatype-geometric.html I will clarify this with a comment and try to investigate regular geometry data types a bit more. -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
jnh5y commented on code in PR #3668: URL: https://github.com/apache/calcite/pull/3668#discussion_r1486959295 ## core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java: ## @@ -312,7 +312,9 @@ private Expression getConvertExpression( case CHAR: case VARCHAR: return Expressions.call(BuiltInMethod.ST_GEOM_FROM_EWKT.method, operand); - + case BINARY: + case VARBINARY: +return Expressions.call(BuiltInMethod.ST_GEOM_FROM_EWKB.method, operand); Review Comment: As a future consideration, there are other binary formats like Tiny WKB. https://github.com/TWKB/Specification/blob/master/twkb.md I suppose that may be handled by a specific cast. -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
julianhyde commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1486928091
##
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##
@@ -1140,27 +1140,447 @@ public enum SqlKind {
// Spatial functions. They are registered as "user-defined functions" but it
// is convenient to have a "kind" so that we can quickly match them in
planner
// rules.
+ //
+ // One can generate the list of spatial functions using the following
snippet:
+ //
+ // Method[] methods = SpatialTypeFunctions .class.getDeclaredMethods();
+ // Arrays.stream(methods)
+ //.map(java.lang.reflect.Method::getName)
+ //.filter(method -> method.startsWith("ST_"))
+ //.distinct().sorted()
+ //.forEach(method -> {
+ // System.out.println("/** The {@code " + method + "} function. */");
+ // System.out.print(method.toUpperCase());
+ // System.out.print(",\n\n");
+ //});
- /** The {@code ST_DWithin} geo-spatial function. */
+ /** The {@code ST_AddPoint} function. */
+ ST_ADDPOINT,
+
+ /** The {@code ST_AddZ} function. */
+ ST_ADDZ,
+
+ /** The {@code ST_Area} function. */
+ ST_AREA,
+
+ /** The {@code ST_AsBinary} function. */
+ ST_ASBINARY,
+
+ /** The {@code ST_AsEWKB} function. */
+ ST_ASEWKB,
+
+ /** The {@code ST_AsEWKT} function. */
+ ST_ASEWKT,
+
+ /** The {@code ST_AsGML} function. */
+ ST_ASGML,
+
+ /** The {@code ST_AsGeoJSON} function. */
+ ST_ASGEOJSON,
+
+ /** The {@code ST_AsText} function. */
+ ST_ASTEXT,
+
+ /** The {@code ST_AsWKB} function. */
+ ST_ASWKB,
+
+ /** The {@code ST_AsWKT} function. */
+ ST_ASWKT,
+
+ /** The {@code ST_Boundary} function. */
+ ST_BOUNDARY,
+
+ /** The {@code ST_BoundingCircle} function. */
+ ST_BOUNDINGCIRCLE,
+
+ /** The {@code ST_Buffer} function. */
+ ST_BUFFER,
+
+ /** The {@code ST_Centroid} function. */
+ ST_CENTROID,
+
+ /** The {@code ST_ClosestCoordinate} function. */
+ ST_CLOSESTCOORDINATE,
+
+ /** The {@code ST_ClosestPoint} function. */
+ ST_CLOSESTPOINT,
+
+ /** The {@code ST_ConstrainedDelaunay} function. */
+ ST_CONSTRAINEDDELAUNAY,
+
+ /** The {@code ST_Contains} function. */
+ ST_CONTAINS,
+
+ /** The {@code ST_ContainsProperly} function. */
+ ST_CONTAINSPROPERLY,
+
+ /** The {@code ST_ConvexHull} function. */
+ ST_CONVEXHULL,
+
+ /** The {@code ST_CoordDim} function. */
+ ST_COORDDIM,
+
+ /** The {@code ST_CoveredBy} function. */
+ ST_COVEREDBY,
+
+ /** The {@code ST_Covers} function. */
+ ST_COVERS,
+
+ /** The {@code ST_Crosses} function. */
+ ST_CROSSES,
+
+ /** The {@code ST_DWithin} function. */
ST_DWITHIN,
+ /** The {@code ST_Delaunay} function. */
+ ST_DELAUNAY,
+
+ /** The {@code ST_Densify} function. */
+ ST_DENSIFY,
+
+ /** The {@code ST_Difference} function. */
+ ST_DIFFERENCE,
+
+ /** The {@code ST_Dimension} function. */
+ ST_DIMENSION,
+
+ /** The {@code ST_Disjoint} function. */
+ ST_DISJOINT,
+
+ /** The {@code ST_Distance} function. */
+ ST_DISTANCE,
+
+ /** The {@code ST_EndPoint} function. */
+ ST_ENDPOINT,
+
+ /** The {@code ST_Envelope} function. */
+ ST_ENVELOPE,
+
+ /** The {@code ST_EnvelopesIntersect} function. */
+ ST_ENVELOPESINTERSECT,
+
+ /** The {@code ST_Equals} function. */
+ ST_EQUALS,
+
+ /** The {@code ST_Expand} function. */
+ ST_EXPAND,
+
+ /** The {@code ST_Explode} function. */
+ ST_EXPLODE,
+
+ /** The {@code ST_Extent} function. */
+ ST_EXTENT,
+
+ /** The {@code ST_ExteriorRing} function. */
+ ST_EXTERIORRING,
+
+ /** The {@code ST_FlipCoordinates} function. */
+ ST_FLIPCOORDINATES,
+
+ /** The {@code ST_Force2D} function. */
+ ST_FORCE2D,
+
+ /** The {@code ST_Force3D} function. */
+ ST_FORCE3D,
+
+ /** The {@code ST_FurthestCoordinate} function. */
+ ST_FURTHESTCOORDINATE,
+
+ /** The {@code ST_GeomFromEWKB} function. */
+ ST_GEOMFROMEWKB,
+
+ /** The {@code ST_GeomFromEWKT} function. */
+ ST_GEOMFROMEWKT,
+
+ /** The {@code ST_GeomFromGML} function. */
+ ST_GEOMFROMGML,
+
+ /** The {@code ST_GeomFromGeoJSON} function. */
+ ST_GEOMFROMGEOJSON,
+
+ /** The {@code ST_GeomFromText} function. */
+ ST_GEOMFROMTEXT,
+
+ /** The {@code ST_GeomFromWKB} function. */
+ ST_GEOMFROMWKB,
+
+ /** The {@code ST_GeomFromWKT} function. */
+ ST_GEOMFROMWKT,
+
+ /** The {@code ST_GeometryN} function. */
+ ST_GEOMETRYN,
+
+ /** The {@code ST_GeometryType} function. */
+ ST_GEOMETRYTYPE,
+
+ /** The {@code ST_GeometryTypeCode} function. */
+ ST_GEOMETRYTYPECODE,
+
+ /** The {@code ST_Holes} function. */
+ ST_HOLES,
+
+ /** The {@code ST_InteriorRing} function. */
+ ST_INTERIORRING,
+
+ /** The {@code ST_Intersection} function. */
+ ST_INTERSECTION,
+
+ /** The {@code ST_Intersects} function. */
+ ST_INTERSECTS,
+
+ /** The {@code ST_Is3D} function. */
+ ST_IS3D,
+
+ /** The {@code ST_IsClosed} function. */
+ ST_ISCLOSED,
+
+ /** The {@code ST_IsEmpty} function. */
+ ST_ISEMPTY,
+
+ /** The {@code ST_IsRectangle}
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
julianhyde commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1486918013
##
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.sql.dialect;
+
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A SqlDialect implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT =
SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new
PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgresqlSqlDialect. */
+ public PostgisSqlDialect(Context context) {
+super(context);
+ }
+
+ @Override public void unparseCall(SqlWriter writer, SqlCall call, int
leftPrec, int rightPrec) {
Review Comment:
Simplest thing seems to be to make Postgis dialect a subclass of Postgres
dialect. There are many similarities, but also many differences.
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1934535856 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [59.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1934228556 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** Issues  [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true)  [59.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1931862374 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true) [60.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1930960796 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [6 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true) [60.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1928140652 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true) [93.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1926614766 Thanks a lot for the pointer, I will adopt the same approach and implement some integration tests in a third-party repository for now. -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
JiajunBernoulli commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1926081457 > @YiwenWu @JiajunBernoulli What would be the best way to test these changes against a postgis database? Ideally, I'd like to add an integration test that executes queries against postgis (e.g. with testcontainers). But I havn't been able to find such tests in calcite. There are some integration tests for Druid: https://github.com/zabetak/calcite-druid-dataset Here is CI config in calcite: https://github.com/apache/calcite/blob/2aabf210dc1918c6ca20e63b39661ff445535eb8/.github/workflows/main.yml#L440 -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1477257234
##
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.sql.dialect;
+
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A SqlDialect implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT =
SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new
PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgresqlSqlDialect. */
+ public PostgisSqlDialect(Context context) {
+super(context);
+ }
+
+ @Override public void unparseCall(SqlWriter writer, SqlCall call, int
leftPrec, int rightPrec) {
Review Comment:
ST_UNION is the main case I had in mind that required to introduce a new
function name (ST_UNARYUNION) in calcite. However, some other functions, such
as ST_DUMP, may require a similar treatment. I guess that the list will grow as
the dialect gets more systematically tested against postgis.
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
macroguo-ghy commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1925587529 > @YiwenWu @JiajunBernoulli What would be the best way to test these changes against a postgis database? Ideally, I'd like to add an integration test that executes queries against postgis (e.g. with testcontainers). But I havn't been able to find such tests in calcite. I remember that testContainer has been used in redis apapter test. -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
YiwenWu commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1477158273
##
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.sql.dialect;
+
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A SqlDialect implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT =
SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new
PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgresqlSqlDialect. */
+ public PostgisSqlDialect(Context context) {
+super(context);
+ }
+
+ @Override public void unparseCall(SqlWriter writer, SqlCall call, int
leftPrec, int rightPrec) {
Review Comment:
It would be clearer to have an independent Dialect, but I am worried that it
will introduce a maintenance burden.
At present, PostGIS does not have many unique functions.
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1925277348 What would be the best way to test these changes against a postgis database? Ideally, I'd like to add an integration test that executes queries against postgis (e.g. with testcontainers). But I havn't been able to find such tests in calcite. -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1925255637 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true) [96.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
sonarcloud[bot] commented on PR #3668: URL: https://github.com/apache/calcite/pull/3668#issuecomment-1925252773 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3668) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite=3668=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3668=false=true) [96.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3668=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3668) -- 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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
JiajunBernoulli commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1477029166
##
core/src/main/java/org/apache/calcite/sql/SqlDialectFactoryImpl.java:
##
@@ -222,6 +223,8 @@ public class SqlDialectFactoryImpl implements
SqlDialectFactory {
return PhoenixSqlDialect.DEFAULT;
case POSTGRESQL:
return PostgresqlSqlDialect.DEFAULT;
+case POSTGIS:
Review Comment:
Is `POSTGIS` before `POSTGRESQL`?
> I is before R
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1477026643
##
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##
@@ -7876,6 +7878,26 @@ private void checkLiteral2(String expression, String
expected) {
.withSpark().ok(sparkExpected);
}
+ /**
+ * Test case for ST_SRID function.
+ * All the spatial functions where the arguments are the same type should
behave similarly.
+ */
+ @Test void testPostgisStSrid() {
+String query = "select st_srid(\"point\") FROM \"points\"";
+String expectedPostgis = "SELECT \"ST_SRID\"(\"point\")\nFROM
\"GEO\".\"points\"";
Review Comment:
I should probably have created a draft PR.
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1477026388
##
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.sql.dialect;
+
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A SqlDialect implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT =
SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new
PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgresqlSqlDialect. */
+ public PostgisSqlDialect(Context context) {
+super(context);
+ }
+
+ @Override public void unparseCall(SqlWriter writer, SqlCall call, int
leftPrec, int rightPrec) {
Review Comment:
Yes they could, I also had mixed feeling about extending the postgresql
dialect. My current reasoning comes from the fact that postgis is an extension
that must be installed on the server and then activated in the database with
`CREATE EXTENSION postgis;`. Having a separate dialect makes things more
explicit in calcite and isolates postgis specific customizations in a dedicated
`unparseCall` method.
--
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
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
YiwenWu commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r1476931157
##
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.sql.dialect;
+
+import org.apache.calcite.avatica.util.Casing;
+import org.apache.calcite.sql.SqlCall;
+import org.apache.calcite.sql.SqlDialect;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A SqlDialect implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT =
SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new
PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgresqlSqlDialect. */
+ public PostgisSqlDialect(Context context) {
+super(context);
+ }
+
+ @Override public void unparseCall(SqlWriter writer, SqlCall call, int
leftPrec, int rightPrec) {
Review Comment:
Since PostGIS is an extension of PostgreSQL, Can these changes be processed
directly in PostgresqlSqlDialect?
##
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##
@@ -7876,6 +7878,26 @@ private void checkLiteral2(String expression, String
expected) {
.withSpark().ok(sparkExpected);
}
+ /**
+ * Test case for ST_SRID function.
+ * All the spatial functions where the arguments are the same type should
behave similarly.
+ */
+ @Test void testPostgisStSrid() {
+String query = "select st_srid(\"point\") FROM \"points\"";
+String expectedPostgis = "SELECT \"ST_SRID\"(\"point\")\nFROM
\"GEO\".\"points\"";
Review Comment:
Some UT tests failed
--
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
[PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
bchapuis opened a new pull request, #3668: URL: https://github.com/apache/calcite/pull/3668 (no comment) -- 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
