You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "bchapuis (via GitHub)" <gi...@apache.org> on 2024/02/02 22:58:52 UTC
[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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1934228556
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [59.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943689440
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [84.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943793419
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [14 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [84.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
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} function. */
+ ST_ISRECTANGLE,
+
+ /** The {@code ST_IsRing} function. */
+ ST_ISRING,
+
+ /** The {@code ST_IsSimple} function. */
+ ST_ISSIMPLE,
+
+ /** The {@code ST_IsValid} function. */
+ ST_ISVALID,
+
+ /** The {@code ST_Length} function. */
+ ST_LENGTH,
+
+ /** The {@code ST_LineFromText} function. */
+ ST_LINEFROMTEXT,
+
+ /** The {@code ST_LineFromWKB} function. */
+ ST_LINEFROMWKB,
+
+ /** The {@code ST_LineMerge} function. */
+ ST_LINEMERGE,
+
+ /** The {@code ST_LocateAlong} function. */
+ ST_LOCATEALONG,
+
+ /** The {@code ST_LongestLine} function. */
+ ST_LONGESTLINE,
+
+ /** The {@code ST_MLineFromText} function. */
+ ST_MLINEFROMTEXT,
+
+ /** The {@code ST_MPointFromText} function. */
+ ST_MPOINTFROMTEXT,
+
+ /** The {@code ST_MPolyFromText} function. */
+ ST_MPOLYFROMTEXT,
+
+ /** The {@code ST_MakeEllipse} function. */
+ ST_MAKEELLIPSE,
+
+ /** The {@code ST_MakeEnvelope} function. */
+ ST_MAKEENVELOPE,
+
+ /** The {@code ST_MakeGrid} function. */
+ ST_MAKEGRID,
+
+ /** The {@code ST_MakeGridPoints} function. */
+ ST_MAKEGRIDPOINTS,
+
+ /** The {@code ST_MakeLine} function. */
+ ST_MAKELINE,
+
+ /** The {@code ST_MakePoint} function. */
+ ST_MAKEPOINT,
+
+ /** The {@code ST_MakePolygon} function. */
+ ST_MAKEPOLYGON,
+
+ /** The {@code ST_MakeValid} function. */
+ ST_MAKEVALID,
+
+ /** The {@code ST_MaxDistance} function. */
+ ST_MAXDISTANCE,
+
+ /** The {@code ST_MinimumDiameter} function. */
+ ST_MINIMUMDIAMETER,
+
+ /** The {@code ST_MinimumRectangle} function. */
+ ST_MINIMUMRECTANGLE,
+
+ /** The {@code ST_NPoints} function. */
+ ST_NPOINTS,
+
+ /** The {@code ST_Normalize} function. */
+ ST_NORMALIZE,
+
+ /** The {@code ST_NumGeometries} function. */
+ ST_NUMGEOMETRIES,
+
+ /** The {@code ST_NumInteriorRing} function. */
+ ST_NUMINTERIORRING,
+
+ /** The {@code ST_NumInteriorRings} function. */
+ ST_NUMINTERIORRINGS,
+
+ /** The {@code ST_NumPoints} function. */
+ ST_NUMPOINTS,
+
+ /** The {@code ST_OctagonalEnvelope} function. */
+ ST_OCTAGONALENVELOPE,
+
+ /** The {@code ST_OffsetCurve} function. */
+ ST_OFFSETCURVE,
+
+ /** The {@code ST_OrderingEquals} function. */
+ ST_ORDERINGEQUALS,
+
+ /** The {@code ST_Overlaps} function. */
+ ST_OVERLAPS,
+
+ /** The {@code ST_Perimeter} function. */
+ ST_PERIMETER,
+
/** The {@code ST_Point} function. */
ST_POINT,
- /** The {@code ST_Point} function that makes a 3D point. */
- ST_POINT3,
+ /** The {@code ST_PointFromText} function. */
+ ST_POINTFROMTEXT,
- /** The {@code ST_MakeLine} function that makes a line. */
- ST_MAKE_LINE,
+ /** The {@code ST_PointFromWKB} function. */
+ ST_POINTFROMWKB,
- /** The {@code ST_Contains} function that tests whether one geometry contains
- * another. */
- ST_CONTAINS,
+ /** The {@code ST_PointN} function. */
+ ST_POINTN,
+
+ /** The {@code ST_PointOnSurface} function. */
+ ST_POINTONSURFACE,
+
+ /** The {@code ST_PolyFromText} function. */
+ ST_POLYFROMTEXT,
+
+ /** The {@code ST_PolyFromWKB} function. */
+ ST_POLYFROMWKB,
+
+ /** The {@code ST_Polygonize} function. */
+ ST_POLYGONIZE,
+
+ /** The {@code ST_PrecisionReducer} function. */
+ ST_PRECISIONREDUCER,
+
+ /** The {@code ST_ProjectPoint} function. */
+ ST_PROJECTPOINT,
+
+ /** The {@code ST_ReducePrecision} function. */
+ ST_REDUCEPRECISION,
+
+ /** The {@code ST_Relate} function. */
+ ST_RELATE,
+
+ /** The {@code ST_RemoveHoles} function. */
+ ST_REMOVEHOLES,
+
+ /** The {@code ST_RemovePoint} function. */
+ ST_REMOVEPOINT,
+
+ /** The {@code ST_RemoveRepeatedPoints} function. */
+ ST_REMOVEREPEATEDPOINTS,
+
+ /** The {@code ST_Reverse} function. */
+ ST_REVERSE,
+
+ /** The {@code ST_Rotate} function. */
+ ST_ROTATE,
+
+ /** The {@code ST_SRID} function. */
+ ST_SRID,
+
+ /** The {@code ST_Scale} function. */
+ ST_SCALE,
+
+ /** The {@code ST_SetSRID} function. */
+ ST_SETSRID,
+
+ /** The {@code ST_Simplify} function. */
+ ST_SIMPLIFY,
+
+ /** The {@code ST_SimplifyPreserveTopology} function. */
+ ST_SIMPLIFYPRESERVETOPOLOGY,
+
+ /** The {@code ST_Snap} function. */
+ ST_SNAP,
+
+ /** The {@code ST_Split} function. */
+ ST_SPLIT,
+
+ /** The {@code ST_StartPoint} function. */
+ ST_STARTPOINT,
+
+ /** The {@code ST_SymDifference} function. */
+ ST_SYMDIFFERENCE,
+
+ /** The {@code ST_ToMultiLine} function. */
+ ST_TOMULTILINE,
+
+ /** The {@code ST_ToMultiPoint} function. */
+ ST_TOMULTIPOINT,
+
+ /** The {@code ST_ToMultiSegments} function. */
+ ST_TOMULTISEGMENTS,
+
+ /** The {@code ST_Touches} function. */
+ ST_TOUCHES,
+
+ /** The {@code ST_Transform} function. */
+ ST_TRANSFORM,
+
+ /** The {@code ST_Translate} function. */
+ ST_TRANSLATE,
+
+ /** The {@code ST_UnaryUnion} function. */
+ ST_UNARYUNION,
+
+ /** The {@code ST_UnaryUnion} function. */
+ ST_UNION,
+
+ /** The {@code ST_Within} function. */
+ ST_WITHIN,
+
+ /** The {@code ST_X} function. */
+ ST_X,
+
+ /** The {@code ST_XMax} function. */
+ ST_XMAX,
+
+ /** The {@code ST_XMin} function. */
+ ST_XMIN,
+
+ /** The {@code ST_Y} function. */
+ ST_Y,
+
+ /** The {@code ST_YMax} function. */
+ ST_YMAX,
+
+ /** The {@code ST_YMin} function. */
+ ST_YMIN,
+
+ /** The {@code ST_Z} function. */
+ ST_Z,
+
+ /** The {@code ST_ZMax} function. */
+ ST_ZMAX,
+
+ /** The {@code ST_ZMin} function. */
+ ST_ZMIN,
/** The {@code Hilbert} function that converts (x, y) to a position on a
* Hilbert space-filling curve. */
HILBERT,
+ ST_MAKE_LINE,
Review Comment:
Are these obsolete or not?
##########
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:
huh?
##########
core/src/main/java/org/apache/calcite/sql/SqlDialect.java:
##########
@@ -1382,6 +1384,7 @@ public enum DatabaseProduct {
LUCIDDB("LucidDB", "\"", NullCollation.HIGH),
INTERBASE("Interbase", null, NullCollation.HIGH),
PHOENIX("Phoenix", "\"", NullCollation.HIGH),
+ POSTGIS("Postgis", "\"", NullCollation.HIGH),
Review Comment:
Usual capitalization is PostGIS, I believe
##########
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:
Do we really want this dependency here?
##########
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java:
##########
@@ -283,6 +284,15 @@ private static void generateGet(EnumerableRelImplementor implementor,
java.sql.Array.class);
source = Expressions.call(BuiltInMethod.JDBC_ARRAY_TO_LIST.method, x);
break;
+ case GEOMETRY:
+ source =
+ Expressions.call(
Review Comment:
Add methods to BuiltInMethod. We don't want reflection errors at runtime.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeName.java:
##########
@@ -264,6 +264,8 @@ public enum SqlTypeName {
.put(Types.DISTINCT, DISTINCT)
.put(Types.STRUCT, STRUCTURED)
.put(Types.ARRAY, ARRAY)
+ .put(Types.OTHER, OTHER)
+
Review Comment:
remove blank line
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1488,6 +1505,7 @@ public static SqlNode toSql(RexLiteral literal) {
default:
break;
}
+
Review Comment:
remove blank line
##########
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.validate.SqlConformanceEnum;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A <code>SqlDialect</code> implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT = SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withConformance(SqlConformanceEnum.LENIENT)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgresqlSqlDialect. */
Review Comment:
a few places you wrote 'Postgresql' where you meant 'PostGIS' or 'Postgis'.
##########
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:
On most databases the type string will be upper-case, "GEOMETRY".
##########
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.validate.SqlConformanceEnum;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A <code>SqlDialect</code> implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT = SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withConformance(SqlConformanceEnum.LENIENT)
+ .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) {
+ if (call.getOperator().getName().equals("ST_UNARYUNION")) {
+ RelToSqlConverterUtil.specialOperatorByName("ST_UNION")
Review Comment:
use SqlKind rather than name?
##########
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} function. */
+ ST_ISRECTANGLE,
+
+ /** The {@code ST_IsRing} function. */
+ ST_ISRING,
+
+ /** The {@code ST_IsSimple} function. */
+ ST_ISSIMPLE,
+
+ /** The {@code ST_IsValid} function. */
+ ST_ISVALID,
+
+ /** The {@code ST_Length} function. */
+ ST_LENGTH,
+
+ /** The {@code ST_LineFromText} function. */
+ ST_LINEFROMTEXT,
+
+ /** The {@code ST_LineFromWKB} function. */
+ ST_LINEFROMWKB,
+
+ /** The {@code ST_LineMerge} function. */
+ ST_LINEMERGE,
+
+ /** The {@code ST_LocateAlong} function. */
+ ST_LOCATEALONG,
+
+ /** The {@code ST_LongestLine} function. */
+ ST_LONGESTLINE,
+
+ /** The {@code ST_MLineFromText} function. */
+ ST_MLINEFROMTEXT,
+
+ /** The {@code ST_MPointFromText} function. */
+ ST_MPOINTFROMTEXT,
+
+ /** The {@code ST_MPolyFromText} function. */
+ ST_MPOLYFROMTEXT,
+
+ /** The {@code ST_MakeEllipse} function. */
+ ST_MAKEELLIPSE,
+
+ /** The {@code ST_MakeEnvelope} function. */
+ ST_MAKEENVELOPE,
+
+ /** The {@code ST_MakeGrid} function. */
+ ST_MAKEGRID,
+
+ /** The {@code ST_MakeGridPoints} function. */
+ ST_MAKEGRIDPOINTS,
+
+ /** The {@code ST_MakeLine} function. */
+ ST_MAKELINE,
+
+ /** The {@code ST_MakePoint} function. */
+ ST_MAKEPOINT,
+
+ /** The {@code ST_MakePolygon} function. */
+ ST_MAKEPOLYGON,
+
+ /** The {@code ST_MakeValid} function. */
+ ST_MAKEVALID,
+
+ /** The {@code ST_MaxDistance} function. */
+ ST_MAXDISTANCE,
+
+ /** The {@code ST_MinimumDiameter} function. */
+ ST_MINIMUMDIAMETER,
+
+ /** The {@code ST_MinimumRectangle} function. */
+ ST_MINIMUMRECTANGLE,
+
+ /** The {@code ST_NPoints} function. */
+ ST_NPOINTS,
+
+ /** The {@code ST_Normalize} function. */
+ ST_NORMALIZE,
+
+ /** The {@code ST_NumGeometries} function. */
+ ST_NUMGEOMETRIES,
+
+ /** The {@code ST_NumInteriorRing} function. */
+ ST_NUMINTERIORRING,
+
+ /** The {@code ST_NumInteriorRings} function. */
+ ST_NUMINTERIORRINGS,
+
+ /** The {@code ST_NumPoints} function. */
+ ST_NUMPOINTS,
+
+ /** The {@code ST_OctagonalEnvelope} function. */
+ ST_OCTAGONALENVELOPE,
+
+ /** The {@code ST_OffsetCurve} function. */
+ ST_OFFSETCURVE,
+
+ /** The {@code ST_OrderingEquals} function. */
+ ST_ORDERINGEQUALS,
+
+ /** The {@code ST_Overlaps} function. */
+ ST_OVERLAPS,
+
+ /** The {@code ST_Perimeter} function. */
+ ST_PERIMETER,
+
/** The {@code ST_Point} function. */
ST_POINT,
- /** The {@code ST_Point} function that makes a 3D point. */
- ST_POINT3,
+ /** The {@code ST_PointFromText} function. */
+ ST_POINTFROMTEXT,
- /** The {@code ST_MakeLine} function that makes a line. */
- ST_MAKE_LINE,
+ /** The {@code ST_PointFromWKB} function. */
+ ST_POINTFROMWKB,
- /** The {@code ST_Contains} function that tests whether one geometry contains
- * another. */
- ST_CONTAINS,
+ /** The {@code ST_PointN} function. */
+ ST_POINTN,
+
+ /** The {@code ST_PointOnSurface} function. */
+ ST_POINTONSURFACE,
+
+ /** The {@code ST_PolyFromText} function. */
+ ST_POLYFROMTEXT,
+
+ /** The {@code ST_PolyFromWKB} function. */
+ ST_POLYFROMWKB,
+
+ /** The {@code ST_Polygonize} function. */
+ ST_POLYGONIZE,
+
+ /** The {@code ST_PrecisionReducer} function. */
+ ST_PRECISIONREDUCER,
+
+ /** The {@code ST_ProjectPoint} function. */
+ ST_PROJECTPOINT,
+
+ /** The {@code ST_ReducePrecision} function. */
+ ST_REDUCEPRECISION,
+
+ /** The {@code ST_Relate} function. */
+ ST_RELATE,
+
+ /** The {@code ST_RemoveHoles} function. */
+ ST_REMOVEHOLES,
+
+ /** The {@code ST_RemovePoint} function. */
+ ST_REMOVEPOINT,
+
+ /** The {@code ST_RemoveRepeatedPoints} function. */
+ ST_REMOVEREPEATEDPOINTS,
+
+ /** The {@code ST_Reverse} function. */
+ ST_REVERSE,
+
+ /** The {@code ST_Rotate} function. */
+ ST_ROTATE,
+
+ /** The {@code ST_SRID} function. */
+ ST_SRID,
+
+ /** The {@code ST_Scale} function. */
+ ST_SCALE,
+
+ /** The {@code ST_SetSRID} function. */
+ ST_SETSRID,
+
+ /** The {@code ST_Simplify} function. */
+ ST_SIMPLIFY,
+
+ /** The {@code ST_SimplifyPreserveTopology} function. */
+ ST_SIMPLIFYPRESERVETOPOLOGY,
+
+ /** The {@code ST_Snap} function. */
+ ST_SNAP,
+
+ /** The {@code ST_Split} function. */
+ ST_SPLIT,
+
+ /** The {@code ST_StartPoint} function. */
+ ST_STARTPOINT,
+
+ /** The {@code ST_SymDifference} function. */
+ ST_SYMDIFFERENCE,
+
+ /** The {@code ST_ToMultiLine} function. */
+ ST_TOMULTILINE,
+
+ /** The {@code ST_ToMultiPoint} function. */
+ ST_TOMULTIPOINT,
+
+ /** The {@code ST_ToMultiSegments} function. */
+ ST_TOMULTISEGMENTS,
+
+ /** The {@code ST_Touches} function. */
+ ST_TOUCHES,
+
+ /** The {@code ST_Transform} function. */
+ ST_TRANSFORM,
+
+ /** The {@code ST_Translate} function. */
+ ST_TRANSLATE,
+
+ /** The {@code ST_UnaryUnion} function. */
+ ST_UNARYUNION,
+
+ /** The {@code ST_UnaryUnion} function. */
+ ST_UNION,
Review Comment:
javadoc should be ST_Union
##########
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.validate.SqlConformanceEnum;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A <code>SqlDialect</code> implementation for the PostgreSQL database.
+ */
Review Comment:
explain why it is a subclass of PostgresSQLDialect
##########
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:
This class seems like a utility class, not appropriate for the org.apache.calcite.sql.dialect package. I didn't see any unit tests, but it could use a few.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-3138036354
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [147 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [83.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-2916049277
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [158 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2135328409
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1511,6 +1513,21 @@ public static SqlNode toSql(RexLiteral literal) {
// Create a string without specifying a charset
return SqlLiteral.createCharString((String) castNonNull(literal.getValue2()), POS);
}
+ case GEOMETRY: {
+ Geometry geom = castNonNull(literal.getValueAs(Geometry.class));
+ switch (typeName) {
+ case CHAR:
+ case VARCHAR:
+ return SqlLiteral.createCharString(
+ SpatialTypeUtils.asEwkt(geom), POS);
+ case BINARY:
+ case VARBINARY:
+ return SqlLiteral.createBinaryString(
+ SpatialTypeUtils.asWkbArray(geom), POS);
+ default:
+ return SqlLiteral.createNull(POS);
Review Comment:
why is this null?
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
Review Comment:
type families are a very confusing concept in Calcite, I think they should be deprecated.
##########
core/src/test/java/org/apache/calcite/util/PostgisGeometryDecoderTest.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.util;
+
+import org.junit.jupiter.api.Test;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.io.WKTWriter;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for the {@link PostgisGeometryDecoder} class. These tests are based
+ * on the values retrieved from PostGIS using the JDBC driver for PostgreSQL.
+ */
+class PostgisGeometryDecoderTest {
+
+ private static final String POINT = "0101000020E6100000000000000000F03F000000"
+ + "0000000040";
+
+ private static final String LINESTRING = "0102000020E610000003000000000000000"
+ + "00000000000000000000000000000000000F03F000000000000F03F000000000000004"
+ + "00000000000000040";
+
+ private static final String POLYGON = "0103000020E610000002000000050000000000"
+ + "0000000000000000000000000000000000000000F03F00000000000000000000000000"
+ + "00F03F000000000000F03F0000000000000000000000000000F03F0000000000000000"
+ + "000000000000000005000000000000000000E03F000000000000E03F000000000000E0"
+ + "3F333333333333E33F333333333333E33F333333333333E33F333333333333E33F0000"
+ + "00000000E03F000000000000E03F000000000000E03F";
+
+ private static final String MULTIPOINT = "0104000020E610000002000000010100000"
+ + "0000000000000000000000000000000000101000000000000000000F03F00000000000"
+ + "0F03F";
+
+ private static final String MULTILINESTRING = "0105000020E6100000020000000102"
+ + "0000000200000000000000000000000000000000000000000000000000F03F00000000"
+ + "0000F03F01020000000200000000000000000000400000000000000040000000000000"
+ + "08400000000000000840";
+
+ private static final String MULTIPOLYGON = "0106000020E6100000020000000103000"
+ + "000010000000500000000000000000000000000000000000000000000000000F03F000"
+ + "0000000000000000000000000F03F000000000000F03F0000000000000000000000000"
+ + "000F03F000000000000000000000000000000000103000000010000000500000000000"
+ + "0000000004000000000000000400000000000000840000000000000004000000000000"
+ + "0084000000000000008400000000000000040000000000000084000000000000000400"
+ + "000000000000040";
+
+ private static final String GEOMETRYCOLLECTION = "0107000020E6100000020000000"
+ + "1010000000000000000000000000000000000000001020000000200000000000000000"
+ + "0F03F000000000000F03F00000000000000400000000000000040";
+
+ @Test void decodeNull() {
+ Geometry geometry = PostgisGeometryDecoder.decode((String) null);
+ assertEquals(null, geometry);
+ }
+
+ @Test void decodePoint() {
+ Geometry geometry = PostgisGeometryDecoder.decode(POINT);
+ assertTrue(geometry instanceof Point);
+ assertEquals("POINT (1 2)", new WKTWriter().write(geometry));
+ }
+
+ @Test void decodeLineString() {
+ Geometry geometry = PostgisGeometryDecoder.decode(LINESTRING);
+ assertTrue(geometry instanceof org.locationtech.jts.geom.LineString);
+ assertEquals("LINESTRING (0 0, 1 1, 2 2)", new WKTWriter().write(geometry));
+ }
+
+ @Test void decodePolygon() {
+ Geometry geometry = PostgisGeometryDecoder.decode(POLYGON);
+ assertTrue(geometry instanceof org.locationtech.jts.geom.Polygon);
+ assertEquals(
+ "POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0), (0.5 0.5, 0.5 0.6, 0.6 0.6, 0.6 0.5, 0.5 0.5))",
+ new WKTWriter().write(geometry));
+ }
+
+ @Test void decodeMultiPoint() {
+ Geometry geometry = PostgisGeometryDecoder.decode(MULTIPOINT);
+ assertTrue(geometry instanceof org.locationtech.jts.geom.MultiPoint);
+ assertEquals(
+ "MULTIPOINT ((0 0), (1 1))",
+ new WKTWriter().write(geometry));
+ }
+
+ @Test void decodeMultiLineString() {
+ Geometry geometry = PostgisGeometryDecoder.decode(MULTILINESTRING);
+ assertTrue(geometry instanceof org.locationtech.jts.geom.MultiLineString);
+ assertEquals(
+ "MULTILINESTRING ((0 0, 1 1), (2 2, 3 3))",
+ new WKTWriter().write(geometry));
+ }
+
+ @Test void decodeMultiPolygon() {
+ Geometry geometry = PostgisGeometryDecoder.decode(MULTIPOLYGON);
+ assertTrue(geometry instanceof org.locationtech.jts.geom.MultiPolygon);
+ assertEquals(
+ "MULTIPOLYGON (((0 0, 1 0, 1 1, 0 1, 0 0)), ((2 2, 3 2, 3 3, 2 3, 2 2)))",
+ new WKTWriter().write(geometry));
+ }
+
+ @Test void decodeGeometryCollection() {
Review Comment:
how about a few negative tests, where the decoding cannot happen?
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
Review Comment:
but perhaps this PR is not the right place to have this conversation.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java:
##########
@@ -193,6 +193,8 @@ private SqlTypeAssignmentRule(
rule.add(SqlTypeName.GEOMETRY);
rule.add(SqlTypeName.CHAR);
rule.add(SqlTypeName.VARCHAR);
+ rule.add(SqlTypeName.BINARY);
Review Comment:
these (and a few other similar changes) look like bug fixes that would be nice to have even before we can merge the full 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1925255637
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[96.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1928140652
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[93.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1931862374
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[60.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2243855299
##########
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java:
##########
@@ -323,6 +323,14 @@ private static void generateGet(EnumerableRelImplementor implementor,
java.sql.Array.class);
source = Expressions.call(BuiltInMethod.JDBC_ARRAY_TO_LIST.method, x);
break;
+ case GEOMETRY:
Review Comment:
My integration tests are located here: https://github.com/bchapuis/calcite-postgis-tests
I looked into how Calcite uses Testcontainers, but it seems the support is currently limited to a few Redis tests. The JDBC tests don’t appear to leverage Testcontainers yet, which is why I set up external integration tests. Did I overlook anything?
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943793834
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [84.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1942774265
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [85.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
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 <code>SqlDialect</code> 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1925252773
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[96.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "asuriy (via GitHub)" <gi...@apache.org>.
asuriy commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-3553324054
Hi there,
I am hoping to be able to integrate the postgis functions added in this PR in one of my projects in future, so I was wondering if this PR is planned to be a part of the next release? Thanks for your hard work!
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2422694547
##########
core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java:
##########
@@ -837,7 +837,7 @@ private static ColumnMetaData.AvaticaType avaticaType(JavaTypeFactory typeFactor
}
return ColumnMetaData.struct(columns);
case ExtraSqlTypes.GEOMETRY:
- typeOrdinal = Types.VARCHAR;
+ typeOrdinal = Types.OTHER;
Review Comment:
@bchapuis usually in cases like this the PR modifies the commented out part at the top of history.md corresponding to the next release, adding a line on the "Breaking Changes" section so that, when preparing the release, the RM will not forget about mentioning this breaking 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2424642150
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
I understand the concerns regarding the coupling to JTS. There is no problem in posponing this for 1.42. I suggested to include it in this release to unstall the discussion and try to move things forward. It would be great to improve geospatial support 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "jnh5y (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2143309805
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1511,6 +1513,21 @@ public static SqlNode toSql(RexLiteral literal) {
// Create a string without specifying a charset
return SqlLiteral.createCharString((String) castNonNull(literal.getValue2()), POS);
}
+ case GEOMETRY: {
+ Geometry geom = castNonNull(literal.getValueAs(Geometry.class));
+ switch (typeName) {
+ case CHAR:
+ case VARCHAR:
+ return SqlLiteral.createCharString(
+ SpatialTypeUtils.asEwkt(geom), POS);
+ case BINARY:
+ case VARBINARY:
+ return SqlLiteral.createBinaryString(
+ SpatialTypeUtils.asWkbArray(geom), POS);
+ default:
+ return SqlLiteral.createNull(POS);
Review Comment:
sorry, I wasn't sure what to return in that case. Would the following be better?
```java
SqlLiteral.createUnknown(POS);
```
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##########
@@ -644,15 +658,27 @@ public class JavaType extends RelDataTypeImpl {
private final boolean nullable;
private final @Nullable SqlCollation collation;
private final @Nullable Charset charset;
+ private final @Nullable RelDataTypeFamily family;
Review Comment:
The JavaType(Class clazz) constructor is called in many places, and I was worried to break things by specifying a RelDataTypeFamily everywhere. Is this something I should do?
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -116,6 +117,7 @@
import org.checkerframework.checker.initialization.qual.UnknownInitialization;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
I'm not sure how to do it as well. We can probably try to abstract JTS, but I'm not sure the effort is worth it as JTS almost became a defacto standard in Java. Additionaly, I think this coupling is internal as the WKT strings and WKB binaries outputed here can be read by other libraries if needed.
##########
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:
I logged a jira issue for the introduction of this functions:
https://issues.apache.org/jira/browse/CALCITE-6264
##########
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.validate.SqlConformanceEnum;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A <code>SqlDialect</code> implementation for the PostgreSQL database.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT = SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withConformance(SqlConformanceEnum.LENIENT)
+ .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) {
+ if (call.getOperator().getName().equals("ST_UNARYUNION")) {
+ RelToSqlConverterUtil.specialOperatorByName("ST_UNION")
Review Comment:
Thanks, I fixed it.
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
@zabetak This relates to my interpretation of the architectural comments made in [CALCITE-6263](https://issues.apache.org/jira/browse/CALCITE-6263): `RelDataTypeFactoryImpl` is a core class, and the coupling with JTS should preferably occur in a child class. `JavaTypeFactoryImpl` looked to me like a good place to check if a class is a JTS Geometry.
##########
core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java:
##########
@@ -837,7 +837,7 @@ private static ColumnMetaData.AvaticaType avaticaType(JavaTypeFactory typeFactor
}
return ColumnMetaData.struct(columns);
case ExtraSqlTypes.GEOMETRY:
- typeOrdinal = Types.VARCHAR;
+ typeOrdinal = Types.OTHER;
Review Comment:
Yes, this is probably a breaking change, but I don't think VARCHAR is appropriate here.
I think GEOMETRY is clearly database specific as indicated here:
https://docs.oracle.com/en/java/javase/11/docs/api/java.sql/java/sql/Types.html#OTHER
This choice also correspond to what is implemented in the postgresql jdbc driver.
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
This modification was suggested in the following commit and seemed necessary decouple `RelDataTypeFactoryImpl` from JTS:
https://github.com/apache/calcite/pull/3668/commits/da8494213c3810584a75064d5fadf52ac1f95d12
> A related change is to add a 'family' parameter to
the `TypeFactory.createJavaType` method. It is nullable, and
if null, the type becomes its own family. In JavaType, we
should add a `family` field.
> When this change is complete we should be able to remove JTS
Geometry from RelDataTypeFactoryImpl.CLASS_FAMILIES.
##########
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 believe this is necessary when geometries are represented as string in the query with WKT.
##########
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:
I'm not sure about this. There is no type name for geometry in java.sql.Types, but there is an `ExtraSqlTypes.GEOMETRY = 2015` defined in Calcite. I have now registered it in `SqlTypeName#JDBC_TYPE_TO_NAME`.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeFamily.java:
##########
@@ -76,7 +76,7 @@ public enum SqlTypeFamily implements RelDataTypeFamily {
ANY,
CURSOR,
COLUMN_LIST,
- GEO,
+ GEOMETRY,
Review Comment:
Maybe, I can revert it.
##########
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.SqlKind;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.validate.SqlConformanceEnum;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A <code>SqlDialect</code> implementation for the PostGIS database that extends
+ * the PostgreSQL dialect. As PostGIS is an extension of PostgreSQL that must be
+ * installed separately, it makes sense to extend the PostgreSQL dialect. Having
+ * a separate dialect allows for the possibility of adding PostGIS-specific features
+ * in the future. It also isolates PostGIS-specific behavior from the PostgreSQL dialect.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT = SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withConformance(SqlConformanceEnum.LENIENT)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgisSqlDialect. */
+ public PostgisSqlDialect(Context context) {
+ super(context);
+ }
+
+ @Override public void unparseCall(SqlWriter writer, SqlCall call, int leftPrec, int rightPrec) {
+ if (call.getOperator().getName().equals(SqlKind.ST_UNARYUNION.name())) {
+ RelToSqlConverterUtil.specialOperatorByName(SqlKind.ST_UNION.name())
+ .unparse(writer, call, 0, 0);
Review Comment:
I think this is somewhat related to https://issues.apache.org/jira/browse/CALCITE-5280
##########
linq4j/src/main/java/org/apache/calcite/linq4j/function/Parameter.java:
##########
@@ -81,4 +82,10 @@
* is NULL, and therefore optional parameters must be nullable.
*/
boolean optional() default false;
+
+ /** Returns the SQL type code.
+ *
+ * <p>Values are typically from {@link java.sql.Types}, for example
+ * {@link java.sql.Types#INTEGER}. */
+ int sqlType() default Types.OTHER;
Review Comment:
This was introduced in the following commit:
https://github.com/apache/calcite/pull/3668/commits/da8494213c3810584a75064d5fadf52ac1f95d12
> Add annotation Parameter.sqlType
> The annotation allows you to define the SQL type of a
parameter, for cases where it cannot be deduced automatically
from the Java type. This is especially useful for GEOMETRY
values, for which Calcite has no built-in Java type. (JTS
Geometry is widely used in Calcite, but it is an
implementation, and we do not want it to become the
specification.)
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##########
@@ -680,6 +717,9 @@ public Class getJavaClass() {
}
@Override public RelDataTypeFamily getFamily() {
+ if (this.family != null) {
+ return this.family;
+ }
RelDataTypeFamily family = CLASS_FAMILIES.get(clazz);
return family != null ? family : this;
Review Comment:
Same question as before, isn't it a risky breaking change.
##########
core/src/main/java/org/apache/calcite/config/Lex.java:
##########
@@ -63,6 +63,13 @@ public enum Lex {
MYSQL_ANSI(Quoting.DOUBLE_QUOTE, Casing.UNCHANGED, Casing.UNCHANGED, false,
CharLiteralStyle.STANDARD),
+ /** Lexical policy similar to PostgreSQL. Quoted identifiers are preserved;
+ * Unquoted identifiers are converted to lower-case; after which, identifiers
+ * are matched case-sensitively.
+ */
+ POSTGRESQL(Quoting.DOUBLE_QUOTE, Casing.TO_LOWER, Casing.UNCHANGED, false,
Review Comment:
I will look into this. I believe it is covered by the integration tests I implemented with testcontainers in a separate repository. I'm not sure how to test it without starting a postgis container.
##########
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java:
##########
@@ -323,6 +323,14 @@ private static void generateGet(EnumerableRelImplementor implementor,
java.sql.Array.class);
source = Expressions.call(BuiltInMethod.JDBC_ARRAY_TO_LIST.method, x);
break;
+ case GEOMETRY:
Review Comment:
I will look into this. I believe it is covered by the integration tests I implemented with testcontainers in a separate repository.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2242758075
##########
core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java:
##########
@@ -837,7 +837,7 @@ private static ColumnMetaData.AvaticaType avaticaType(JavaTypeFactory typeFactor
}
return ColumnMetaData.struct(columns);
case ExtraSqlTypes.GEOMETRY:
- typeOrdinal = Types.VARCHAR;
+ typeOrdinal = Types.OTHER;
Review Comment:
I checked the history.md file. Is it modified by the release manager? I'm tempted to say that this change is closer to a bugfix and has a relatively low impact as it matches what is done in other databases and drivers.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2244430654
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java:
##########
@@ -193,6 +193,8 @@ private SqlTypeAssignmentRule(
rule.add(SqlTypeName.GEOMETRY);
rule.add(SqlTypeName.CHAR);
rule.add(SqlTypeName.VARCHAR);
+ rule.add(SqlTypeName.BINARY);
Review Comment:
I think some of these bug fixes have been addressed separately by Lucas Brenner in CALCITE-7073. I had a few merge conflicts when rebasing the current branch.
I'm not completely sure what else could be extracted into smaller PR/bug fixes and would prefer to squash merge this PR to move on ;)
At this point I'm not completely sure what else could be extracted into smaller PR/bug fixes. To keep things moving, I’d prefer to squash-merge, unless anyone has other suggestions.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2242669517
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1511,6 +1513,21 @@ public static SqlNode toSql(RexLiteral literal) {
// Create a string without specifying a charset
return SqlLiteral.createCharString((String) castNonNull(literal.getValue2()), POS);
}
+ case GEOMETRY: {
+ Geometry geom = castNonNull(literal.getValueAs(Geometry.class));
+ switch (typeName) {
+ case CHAR:
+ case VARCHAR:
+ return SqlLiteral.createCharString(
+ SpatialTypeUtils.asEwkt(geom), POS);
+ case BINARY:
+ case VARBINARY:
+ return SqlLiteral.createBinaryString(
+ SpatialTypeUtils.asWkbArray(geom), POS);
+ default:
+ return SqlLiteral.createNull(POS);
Review Comment:
I used createUnknown 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2423071993
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
My understanding of @zabetak 's remark was the other way around.
IIANM your proposal would mean that all consumers implementing this (widely used) interface will break upon upgrade, since they'd need to implement the new method `createJavaType(Class clazz, @Nullable RelDataTypeFamily family)` ; whereas having a default implementation for it delegating to `createJavaType(Class clazz)` (as I, and I think @zabetak, proposed) would mean that these consumers shall continue working as before upon upgrade.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2423457982
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
This change comes from a suggestion made by @julianhyde in https://github.com/apache/calcite/commit/da8494213c3810584a75064d5fadf52ac1f95d12
> Add annotation Parameter.sqlType
The annotation allows you to define the SQL type of a
parameter, for cases where it cannot be deduced automatically
from the Java type. This is especially useful for GEOMETRY
values, for which Calcite has no built-in Java type. (JTS
Geometry is widely used in Calcite, but it is an
implementation, and we do not want it to become the
specification.)
>
> This commit is incomplete, but illustrates the general idea.
>
> A related change is to add a 'family' parameter to
the `TypeFactory.createJavaType` method. It is nullable, and
if null, the type becomes its own family. In JavaType, we
should add a `family` field.
>
> When this change is complete we should be able to remove JTS
Geometry from RelDataTypeFactoryImpl.CLASS_FAMILIES.
In my understanding this proposition was an API change (possibly major) that would allow to define the SQL type of a parameter more explicitly. As the family parameter is nullable, it effectively allows the cosumers to continue working as before. I acknoledge that I probably don't get all the possible ramifications of this.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2139649176
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -116,6 +117,7 @@
import org.checkerframework.checker.initialization.qual.UnknownInitialization;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
Additionally, it seems that the new code in this class is not covered by existing 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-2914438602
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [158 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1934535856
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [59.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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} function. */
+ ST_ISRECTANGLE,
+
+ /** The {@code ST_IsRing} function. */
+ ST_ISRING,
+
+ /** The {@code ST_IsSimple} function. */
+ ST_ISSIMPLE,
+
+ /** The {@code ST_IsValid} function. */
+ ST_ISVALID,
+
+ /** The {@code ST_Length} function. */
+ ST_LENGTH,
+
+ /** The {@code ST_LineFromText} function. */
+ ST_LINEFROMTEXT,
+
+ /** The {@code ST_LineFromWKB} function. */
+ ST_LINEFROMWKB,
+
+ /** The {@code ST_LineMerge} function. */
+ ST_LINEMERGE,
+
+ /** The {@code ST_LocateAlong} function. */
+ ST_LOCATEALONG,
+
+ /** The {@code ST_LongestLine} function. */
+ ST_LONGESTLINE,
+
+ /** The {@code ST_MLineFromText} function. */
+ ST_MLINEFROMTEXT,
+
+ /** The {@code ST_MPointFromText} function. */
+ ST_MPOINTFROMTEXT,
+
+ /** The {@code ST_MPolyFromText} function. */
+ ST_MPOLYFROMTEXT,
+
+ /** The {@code ST_MakeEllipse} function. */
+ ST_MAKEELLIPSE,
+
+ /** The {@code ST_MakeEnvelope} function. */
+ ST_MAKEENVELOPE,
+
+ /** The {@code ST_MakeGrid} function. */
+ ST_MAKEGRID,
+
+ /** The {@code ST_MakeGridPoints} function. */
+ ST_MAKEGRIDPOINTS,
+
+ /** The {@code ST_MakeLine} function. */
+ ST_MAKELINE,
+
+ /** The {@code ST_MakePoint} function. */
+ ST_MAKEPOINT,
+
+ /** The {@code ST_MakePolygon} function. */
+ ST_MAKEPOLYGON,
+
+ /** The {@code ST_MakeValid} function. */
+ ST_MAKEVALID,
+
+ /** The {@code ST_MaxDistance} function. */
+ ST_MAXDISTANCE,
+
+ /** The {@code ST_MinimumDiameter} function. */
+ ST_MINIMUMDIAMETER,
+
+ /** The {@code ST_MinimumRectangle} function. */
+ ST_MINIMUMRECTANGLE,
+
+ /** The {@code ST_NPoints} function. */
+ ST_NPOINTS,
+
+ /** The {@code ST_Normalize} function. */
+ ST_NORMALIZE,
+
+ /** The {@code ST_NumGeometries} function. */
+ ST_NUMGEOMETRIES,
+
+ /** The {@code ST_NumInteriorRing} function. */
+ ST_NUMINTERIORRING,
+
+ /** The {@code ST_NumInteriorRings} function. */
+ ST_NUMINTERIORRINGS,
+
+ /** The {@code ST_NumPoints} function. */
+ ST_NUMPOINTS,
+
+ /** The {@code ST_OctagonalEnvelope} function. */
+ ST_OCTAGONALENVELOPE,
+
+ /** The {@code ST_OffsetCurve} function. */
+ ST_OFFSETCURVE,
+
+ /** The {@code ST_OrderingEquals} function. */
+ ST_ORDERINGEQUALS,
+
+ /** The {@code ST_Overlaps} function. */
+ ST_OVERLAPS,
+
+ /** The {@code ST_Perimeter} function. */
+ ST_PERIMETER,
+
/** The {@code ST_Point} function. */
ST_POINT,
- /** The {@code ST_Point} function that makes a 3D point. */
- ST_POINT3,
+ /** The {@code ST_PointFromText} function. */
+ ST_POINTFROMTEXT,
- /** The {@code ST_MakeLine} function that makes a line. */
- ST_MAKE_LINE,
+ /** The {@code ST_PointFromWKB} function. */
+ ST_POINTFROMWKB,
- /** The {@code ST_Contains} function that tests whether one geometry contains
- * another. */
- ST_CONTAINS,
+ /** The {@code ST_PointN} function. */
+ ST_POINTN,
+
+ /** The {@code ST_PointOnSurface} function. */
+ ST_POINTONSURFACE,
+
+ /** The {@code ST_PolyFromText} function. */
+ ST_POLYFROMTEXT,
+
+ /** The {@code ST_PolyFromWKB} function. */
+ ST_POLYFROMWKB,
+
+ /** The {@code ST_Polygonize} function. */
+ ST_POLYGONIZE,
+
+ /** The {@code ST_PrecisionReducer} function. */
+ ST_PRECISIONREDUCER,
+
+ /** The {@code ST_ProjectPoint} function. */
+ ST_PROJECTPOINT,
+
+ /** The {@code ST_ReducePrecision} function. */
+ ST_REDUCEPRECISION,
+
+ /** The {@code ST_Relate} function. */
+ ST_RELATE,
+
+ /** The {@code ST_RemoveHoles} function. */
+ ST_REMOVEHOLES,
+
+ /** The {@code ST_RemovePoint} function. */
+ ST_REMOVEPOINT,
+
+ /** The {@code ST_RemoveRepeatedPoints} function. */
+ ST_REMOVEREPEATEDPOINTS,
+
+ /** The {@code ST_Reverse} function. */
+ ST_REVERSE,
+
+ /** The {@code ST_Rotate} function. */
+ ST_ROTATE,
+
+ /** The {@code ST_SRID} function. */
+ ST_SRID,
+
+ /** The {@code ST_Scale} function. */
+ ST_SCALE,
+
+ /** The {@code ST_SetSRID} function. */
+ ST_SETSRID,
+
+ /** The {@code ST_Simplify} function. */
+ ST_SIMPLIFY,
+
+ /** The {@code ST_SimplifyPreserveTopology} function. */
+ ST_SIMPLIFYPRESERVETOPOLOGY,
+
+ /** The {@code ST_Snap} function. */
+ ST_SNAP,
+
+ /** The {@code ST_Split} function. */
+ ST_SPLIT,
+
+ /** The {@code ST_StartPoint} function. */
+ ST_STARTPOINT,
+
+ /** The {@code ST_SymDifference} function. */
+ ST_SYMDIFFERENCE,
+
+ /** The {@code ST_ToMultiLine} function. */
+ ST_TOMULTILINE,
+
+ /** The {@code ST_ToMultiPoint} function. */
+ ST_TOMULTIPOINT,
+
+ /** The {@code ST_ToMultiSegments} function. */
+ ST_TOMULTISEGMENTS,
+
+ /** The {@code ST_Touches} function. */
+ ST_TOUCHES,
+
+ /** The {@code ST_Transform} function. */
+ ST_TRANSFORM,
+
+ /** The {@code ST_Translate} function. */
+ ST_TRANSLATE,
+
+ /** The {@code ST_UnaryUnion} function. */
+ ST_UNARYUNION,
+
+ /** The {@code ST_UnaryUnion} function. */
+ ST_UNION,
+
+ /** The {@code ST_Within} function. */
+ ST_WITHIN,
+
+ /** The {@code ST_X} function. */
+ ST_X,
+
+ /** The {@code ST_XMax} function. */
+ ST_XMAX,
+
+ /** The {@code ST_XMin} function. */
+ ST_XMIN,
+
+ /** The {@code ST_Y} function. */
+ ST_Y,
+
+ /** The {@code ST_YMax} function. */
+ ST_YMAX,
+
+ /** The {@code ST_YMin} function. */
+ ST_YMIN,
+
+ /** The {@code ST_Z} function. */
+ ST_Z,
+
+ /** The {@code ST_ZMax} function. */
+ ST_ZMAX,
+
+ /** The {@code ST_ZMin} function. */
+ ST_ZMIN,
/** The {@code Hilbert} function that converts (x, y) to a position on a
* Hilbert space-filling curve. */
HILBERT,
+ ST_MAKE_LINE,
Review Comment:
I believe we can remove them. ST_MAKE_LINE is ST_MAKELINE and all results returned by search engines for ST_POINT3 are related to ST_POINT.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2429590986
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeName.java:
##########
@@ -287,6 +287,9 @@ public enum SqlTypeName {
.put(Types.DISTINCT, DISTINCT)
.put(Types.STRUCT, STRUCTURED)
.put(Types.ARRAY, ARRAY)
+ .put(Types.OTHER, OTHER)
Review Comment:
Why does OTHER come into the picture now?
##########
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java:
##########
@@ -323,6 +323,14 @@ private static void generateGet(EnumerableRelImplementor implementor,
java.sql.Array.class);
source = Expressions.call(BuiltInMethod.JDBC_ARRAY_TO_LIST.method, x);
break;
+ case GEOMETRY:
Review Comment:
If the only dependency is Testcontainers (that we already use in Calcite) then you could bring in the tests in the calcite code base (assuming that there are no big data files involved). They don't necessarily need to be under the `JdbcToEnumerableConverter` class; you can create a completely new one especially for `PostGIS`.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeName.java:
##########
@@ -264,6 +264,8 @@ public enum SqlTypeName {
.put(Types.DISTINCT, DISTINCT)
.put(Types.STRUCT, STRUCTURED)
.put(Types.ARRAY, ARRAY)
+ .put(Types.OTHER, OTHER)
+
Review Comment:
Still relevant
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -198,6 +199,7 @@ private static Map<SqlDialect, DatabaseProduct> dialects() {
.put(DatabaseProduct.MYSQL.getDialect(), DatabaseProduct.MYSQL)
.put(mySqlDialect(NullCollation.HIGH), DatabaseProduct.MYSQL)
.put(DatabaseProduct.ORACLE.getDialect(), DatabaseProduct.ORACLE)
+ .put(DatabaseProduct.POSTGIS.getDialect(), DatabaseProduct.POSTGIS)
Review Comment:
How about testing the rest of POSTGIS functions here? Can't we have a single test that builds different queries by iterating through all supported functions?
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
I will leave out the discussion about the general coupling with JTS and will try to focus on more pragmatic stuff.
There is a new API in RelDataTypeFactory and this interface has various implementations both in Calcite code base and elsewhere. The change here implies that only the `JavaTypeFactoryImpl` is able to handle properly GEOMETRY types. If we accept that Geometry types are now part of the core type system shouldn't the other implementations handle them as well?
If we leave the change only here does it mean that all support for PostGIS dialect is possible only if we are using the `JavaTypeFactoryImpl`?
##########
core/src/test/java/org/apache/calcite/sql/validate/LexCaseSensitiveTest.java:
##########
@@ -123,6 +123,18 @@ private static void runProjectQueryWithLex(Lex lex, String sql)
runProjectQueryWithLex(Lex.MYSQL_ANSI, sql);
}
+ @Test void testCalciteCasePostgresql() throws Exception {
+ String sql = "select empid as EMPID, empid from (\n"
+ + " select empid from emps order by EMPS.DEPTNO)";
+ runProjectQueryWithLex(Lex.POSTGRESQL, sql);
+ }
+
+ @Test void testCalciteCasePostgresqlNoException() throws Exception {
+ String sql = "select EMPID, empid from\n"
+ + " (select empid from emps order by emps.deptno)";
+ runProjectQueryWithLex(Lex.POSTGRESQL, sql);
+ }
+
Review Comment:
Tests do not seem relevant to PostGIS dialect support.
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
My suggestion to avoid breakage is the following:
```java
RelDataType createJavaType(Class clazz);
default RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family) {
return createJavaType(clazz);
}
```
Moreover, I don't fully understand what the new API is doing so I would suggest to add unit tests for the most common implementations (`SqlTypeFactoryImpl` and `JavaTypeFactoryImpl`) especially covering cases where we use `Geometry` types that is the focus of the PR.
##########
core/src/test/java/org/apache/calcite/sql/test/SqlTypeNameTest.java:
##########
@@ -202,7 +203,7 @@ class SqlTypeNameTest {
@Test void testOther() {
SqlTypeName tn =
SqlTypeName.getNameForJdbcType(Types.OTHER);
- assertThat("OTHER did not map to null", tn, nullValue());
+ assertThat("OTHER did not map to OTHER", tn, is(OTHER));
Review Comment:
Why there was a test explicitly testing for null?
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-4095395378
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943465882
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [84.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1930960796
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
[60.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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 <code>SqlDialect</code> 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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, RelDataTypeFamily> CLASS_FAMILIES =
ImmutableMap.<Class, RelDataTypeFamily>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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2242750524
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
I abandoned the idea of introducing the wrappers described in CALCITE-6263 after the addition of the @SqlType annotation by @jhyde in [7124c3a](https://github.com/apache/calcite/commit/7124c3ae3e5001b85dc6617554c170be49476f75), as it allows Java types to explicitly declare their corresponding SQL type.
I’m still trying to wrap my head around the best solution 😅, but since spatial support is still evolving in Calcite (and JTS is almost a de facto standard for Java) moving this to `JavaTypeFactoryImpl` felt like a practical fit for now.
Let me know if you’d like to discuss broader architectural directions. I’m happy to revisit it (ideally in another PR), but I hope this strikes a reasonable balance in the meantime.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-1943428367
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [12 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&resolved=false&inNewCodePeriod=true)
 [84.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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, RelDataTypeFamily> CLASS_FAMILIES =
ImmutableMap.<Class, RelDataTypeFamily>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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2152558142
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
I don't see much benefit between doing the change here or in `RelDataTypeFactoryImpl` since both are widely used. My understanding from CALCITE-6263 is that we would have our own `org.apache.calcite.xxx.Geometry` interface and this is what would appear in `RelDataTypeFactoryImpl`. I haven't followed every discussion so maybe my understanding is not correct.
Another side concern with this implementation is that the caller is supposed to pass explicitly the family to use but it seems that for the `Geometry` instances it is not possible. This behavior seems to be inconsistent with the API specification.
##########
core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java:
##########
@@ -837,7 +837,7 @@ private static ColumnMetaData.AvaticaType avaticaType(JavaTypeFactory typeFactor
}
return ColumnMetaData.struct(columns);
case ExtraSqlTypes.GEOMETRY:
- typeOrdinal = Types.VARCHAR;
+ typeOrdinal = Types.OTHER;
Review Comment:
Sounds good but in general breaking changes should be mentioned in history.md unless they are clearly bug fixes.
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##########
@@ -644,15 +658,27 @@ public class JavaType extends RelDataTypeImpl {
private final boolean nullable;
private final @Nullable SqlCollation collation;
private final @Nullable Charset charset;
+ private final @Nullable RelDataTypeFamily family;
Review Comment:
From modifying the constructors I got the impression that we were always passing a family. It seems that it is not true so feel free to ignore this comment.
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
Still you can add a default implementation to avoid the breakage.
##########
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java:
##########
@@ -323,6 +323,14 @@ private static void generateGet(EnumerableRelImplementor implementor,
java.sql.Array.class);
source = Expressions.call(BuiltInMethod.JDBC_ARRAY_TO_LIST.method, x);
break;
+ case GEOMETRY:
Review Comment:
We have usages of testcontainers in Calcite. Is there something that prevents the integration tests to be committed as part of this PR?
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##########
@@ -680,6 +717,9 @@ public Class getJavaClass() {
}
@Override public RelDataTypeFamily getFamily() {
+ if (this.family != null) {
+ return this.family;
+ }
RelDataTypeFamily family = CLASS_FAMILIES.get(clazz);
return family != null ? family : this;
Review Comment:
You are right, let's avoid that especially in the content of this PR.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -116,6 +117,7 @@
import org.checkerframework.checker.initialization.qual.UnknownInitialization;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
From my point of view this also increases the coupling of Calcite with JTS and my understanding from CALCITE-6263 is that we were trying to avoid that. `SqlImplementor` is not as important as the `RelDataTypeFactory` but still it is a class that is used in many projects and so far it didn't have any non `org.apache.calcite` import.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-2924804331
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [158 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-2916171285
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [158 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [82.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-2522057061
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2243848001
##########
core/src/main/java/org/apache/calcite/config/Lex.java:
##########
@@ -63,6 +63,13 @@ public enum Lex {
MYSQL_ANSI(Quoting.DOUBLE_QUOTE, Casing.UNCHANGED, Casing.UNCHANGED, false,
CharLiteralStyle.STANDARD),
+ /** Lexical policy similar to PostgreSQL. Quoted identifiers are preserved;
+ * Unquoted identifiers are converted to lower-case; after which, identifiers
+ * are matched case-sensitively.
+ */
+ POSTGRESQL(Quoting.DOUBLE_QUOTE, Casing.TO_LOWER, Casing.UNCHANGED, false,
Review Comment:
I added a test cases for this in `LexCaseSensitiveTest` and in `LexEscapeTest`.
My integration tests are located here: https://github.com/bchapuis/calcite-postgis-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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2243913748
##########
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##########
@@ -1242,22 +1242,439 @@ 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");
+ // });
Review Comment:
I used this as a convenience to generate the list of function but have now removed it.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2243914254
##########
core/src/main/java/org/apache/calcite/runtime/SpatialTypeFunctions.java:
##########
@@ -22,6 +22,7 @@
import org.apache.calcite.linq4j.function.Deterministic;
import org.apache.calcite.linq4j.function.Experimental;
import org.apache.calcite.linq4j.function.Hints;
+import org.apache.calcite.linq4j.function.Parameter;
Review Comment:
This is related to the approach introduced here:
https://github.com/apache/calcite/commit/7124c3ae3e5001b85dc6617554c170be49476f75
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2137382909
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
This is a public API so adding a new method in the interface can break many consumers. Since family is nullable I guess we can add a default implementation that delegates to `createJavaType(Class)` to avoid that.
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
Why should we do this change here and not in `RelDataTypeFactoryImpl`?
##########
core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java:
##########
@@ -837,7 +837,7 @@ private static ColumnMetaData.AvaticaType avaticaType(JavaTypeFactory typeFactor
}
return ColumnMetaData.struct(columns);
case ExtraSqlTypes.GEOMETRY:
- typeOrdinal = Types.VARCHAR;
+ typeOrdinal = Types.OTHER;
Review Comment:
breaking change?
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeFamily.java:
##########
@@ -76,7 +76,7 @@ public enum SqlTypeFamily implements RelDataTypeFamily {
ANY,
CURSOR,
COLUMN_LIST,
- GEO,
+ GEOMETRY,
Review Comment:
breaking change?
##########
core/src/main/java/org/apache/calcite/runtime/SpatialTypeFunctions.java:
##########
@@ -22,6 +22,7 @@
import org.apache.calcite.linq4j.function.Deterministic;
import org.apache.calcite.linq4j.function.Experimental;
import org.apache.calcite.linq4j.function.Hints;
+import org.apache.calcite.linq4j.function.Parameter;
Review Comment:
Why do we need this annotation now?
##########
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.SqlKind;
+import org.apache.calcite.sql.SqlWriter;
+import org.apache.calcite.sql.validate.SqlConformanceEnum;
+import org.apache.calcite.util.RelToSqlConverterUtil;
+
+/**
+ * A <code>SqlDialect</code> implementation for the PostGIS database that extends
+ * the PostgreSQL dialect. As PostGIS is an extension of PostgreSQL that must be
+ * installed separately, it makes sense to extend the PostgreSQL dialect. Having
+ * a separate dialect allows for the possibility of adding PostGIS-specific features
+ * in the future. It also isolates PostGIS-specific behavior from the PostgreSQL dialect.
+ */
+public class PostgisSqlDialect extends PostgresqlSqlDialect {
+
+ public static final SqlDialect.Context DEFAULT_CONTEXT = SqlDialect.EMPTY_CONTEXT
+ .withDatabaseProduct(DatabaseProduct.POSTGIS)
+ .withConformance(SqlConformanceEnum.LENIENT)
+ .withIdentifierQuoteString("\"")
+ .withUnquotedCasing(Casing.TO_LOWER)
+ .withDataTypeSystem(POSTGRESQL_TYPE_SYSTEM);
+
+ public static final SqlDialect DEFAULT = new PostgisSqlDialect(DEFAULT_CONTEXT);
+
+ /** Creates a PostgisSqlDialect. */
+ public PostgisSqlDialect(Context context) {
+ super(context);
+ }
+
+ @Override public void unparseCall(SqlWriter writer, SqlCall call, int leftPrec, int rightPrec) {
+ if (call.getOperator().getName().equals(SqlKind.ST_UNARYUNION.name())) {
+ RelToSqlConverterUtil.specialOperatorByName(SqlKind.ST_UNION.name())
+ .unparse(writer, call, 0, 0);
Review Comment:
Is this unparse specificity Postgres specific or operator specific? I am just trying to understand why we didn't opt to override the unparse method of the respective operator and we opted to do it at the dialect level.
##########
core/src/test/java/org/apache/calcite/util/PostgisGeometryDecoderTest.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.util;
+
+import org.junit.jupiter.api.Test;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.io.WKTWriter;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for the {@link PostgisGeometryDecoder} class. These tests are based
+ * on the values retrieved from PostGIS using the JDBC driver for PostgreSQL.
+ */
+class PostgisGeometryDecoderTest {
+
+ private static final String POINT = "0101000020E6100000000000000000F03F000000"
+ + "0000000040";
+
+ private static final String LINESTRING = "0102000020E610000003000000000000000"
+ + "00000000000000000000000000000000000F03F000000000000F03F000000000000004"
+ + "00000000000000040";
+
+ private static final String POLYGON = "0103000020E610000002000000050000000000"
+ + "0000000000000000000000000000000000000000F03F00000000000000000000000000"
+ + "00F03F000000000000F03F0000000000000000000000000000F03F0000000000000000"
+ + "000000000000000005000000000000000000E03F000000000000E03F000000000000E0"
+ + "3F333333333333E33F333333333333E33F333333333333E33F333333333333E33F0000"
+ + "00000000E03F000000000000E03F000000000000E03F";
+
+ private static final String MULTIPOINT = "0104000020E610000002000000010100000"
+ + "0000000000000000000000000000000000101000000000000000000F03F00000000000"
+ + "0F03F";
+
+ private static final String MULTILINESTRING = "0105000020E6100000020000000102"
+ + "0000000200000000000000000000000000000000000000000000000000F03F00000000"
+ + "0000F03F01020000000200000000000000000000400000000000000040000000000000"
+ + "08400000000000000840";
+
+ private static final String MULTIPOLYGON = "0106000020E6100000020000000103000"
+ + "000010000000500000000000000000000000000000000000000000000000000F03F000"
+ + "0000000000000000000000000F03F000000000000F03F0000000000000000000000000"
+ + "000F03F000000000000000000000000000000000103000000010000000500000000000"
+ + "0000000004000000000000000400000000000000840000000000000004000000000000"
+ + "0084000000000000008400000000000000040000000000000084000000000000000400"
+ + "000000000000040";
+
+ private static final String GEOMETRYCOLLECTION = "0107000020E6100000020000000"
+ + "1010000000000000000000000000000000000000001020000000200000000000000000"
+ + "0F03F000000000000F03F00000000000000400000000000000040";
+
+ @Test void decodeNull() {
+ Geometry geometry = PostgisGeometryDecoder.decode((String) null);
+ assertEquals(null, geometry);
Review Comment:
Use `assertNull` ?
##########
linq4j/src/main/java/org/apache/calcite/linq4j/function/Parameter.java:
##########
@@ -81,4 +82,10 @@
* is NULL, and therefore optional parameters must be nullable.
*/
boolean optional() default false;
+
+ /** Returns the SQL type code.
+ *
+ * <p>Values are typically from {@link java.sql.Types}, for example
+ * {@link java.sql.Types#INTEGER}. */
+ int sqlType() default Types.OTHER;
Review Comment:
This is another improvement that could potentially stand on its own independent of PostGIS. Also I would be curious to understand why we need this now and we didn't before?
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -116,6 +117,7 @@
import org.checkerframework.checker.initialization.qual.UnknownInitialization;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
It seems that now the RelToSql conversion becomes coupled with `org.locationtech` library. I am wondering if there is a way to avoid it.
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeName.java:
##########
@@ -264,6 +264,8 @@ public enum SqlTypeName {
.put(Types.DISTINCT, DISTINCT)
.put(Types.STRUCT, STRUCTURED)
.put(Types.ARRAY, ARRAY)
+ .put(Types.OTHER, OTHER)
+
Review Comment:
I still see the blank lines here.
##########
testkit/src/main/java/org/apache/calcite/test/CalciteAssert.java:
##########
@@ -888,6 +889,7 @@ static SchemaPlus addSchema_(SchemaPlus rootSchema, SchemaSpec schema) {
case CLONE_FOODMART:
foodmart = addSchemaIfNotExists(rootSchema, SchemaSpec.JDBC_FOODMART);
return rootSchema.add("foodmart2", new CloneSchema(foodmart));
+
Review Comment:
nit: Irrelevant diff
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##########
@@ -644,15 +658,27 @@ public class JavaType extends RelDataTypeImpl {
private final boolean nullable;
private final @Nullable SqlCollation collation;
private final @Nullable Charset charset;
+ private final @Nullable RelDataTypeFamily family;
Review Comment:
Should this really be nullable? I get the impression that we are always gonna set a family either explicit or implicit.
##########
core/src/main/java/org/apache/calcite/sql/validate/implicit/AbstractTypeCoercion.java:
##########
@@ -905,13 +905,20 @@ boolean canImplicitTypeCast(List<RelDataType> types, List<SqlTypeFamily> familie
return expected.getDefaultConcreteType(factory);
}
// CHAR -> GEOMETRY
- if (SqlTypeUtil.isCharacter(in) && expected == SqlTypeFamily.GEO) {
+ if (SqlTypeUtil.isCharacter(in) && expected == SqlTypeFamily.GEOMETRY) {
return expected.getDefaultConcreteType(factory);
}
+
+ // BINARY -> GEOMETRY
+ if (SqlTypeUtil.isBinary(in) && expected == SqlTypeFamily.GEOMETRY) {
+ return expected.getDefaultConcreteType(factory);
+ }
+
if ((SqlTypeUtil.isCharacter(in) || SqlTypeUtil.isBinary(in))
&& expected == SqlTypeFamily.UUID) {
return expected.getDefaultConcreteType(factory);
}
+
Review Comment:
(nit) Remove blank line. The intention is to minimize diff and keep history clean by avoiding unecessary changes.
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java:
##########
@@ -680,6 +717,9 @@ public Class getJavaClass() {
}
@Override public RelDataTypeFamily getFamily() {
+ if (this.family != null) {
+ return this.family;
+ }
RelDataTypeFamily family = CLASS_FAMILIES.get(clazz);
return family != null ? family : this;
Review Comment:
It seems that after the changes in this PR we always gonna set a family during construction so maybe the method could be simplified to just `return this.family`.
##########
testkit/src/main/java/org/apache/calcite/test/CalciteAssert.java:
##########
@@ -917,7 +920,6 @@ static SchemaPlus addSchema_(SchemaPlus rootSchema, SchemaSpec schema) {
ViewTable.viewMacro(rootSchema, sql2,
ImmutableList.of("GEO"), emptyPath, false);
s.add("states", viewMacro2);
-
Review Comment:
nit: Irrelevant diff
##########
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##########
@@ -1242,22 +1242,439 @@ 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");
+ // });
Review Comment:
Not sure why need to put this codeblock here. If we want to ensure that SqlKind entries are generated in a certain way we should better put this code in a unit test.
Other than that, I am not sure why we opted to add all these use-case specific functions in this enum. Where do we use them? Where are they tested?
##########
core/src/main/java/org/apache/calcite/config/Lex.java:
##########
@@ -63,6 +63,13 @@ public enum Lex {
MYSQL_ANSI(Quoting.DOUBLE_QUOTE, Casing.UNCHANGED, Casing.UNCHANGED, false,
CharLiteralStyle.STANDARD),
+ /** Lexical policy similar to PostgreSQL. Quoted identifiers are preserved;
+ * Unquoted identifiers are converted to lower-case; after which, identifiers
+ * are matched case-sensitively.
+ */
+ POSTGRESQL(Quoting.DOUBLE_QUOTE, Casing.TO_LOWER, Casing.UNCHANGED, false,
Review Comment:
Where do we use this? How is it tested?
##########
core/src/test/java/org/apache/calcite/util/PostgisGeometryDecoderTest.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.util;
+
+import org.junit.jupiter.api.Test;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.io.WKTWriter;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for the {@link PostgisGeometryDecoder} class. These tests are based
+ * on the values retrieved from PostGIS using the JDBC driver for PostgreSQL.
+ */
+class PostgisGeometryDecoderTest {
Review Comment:
It seems that this class is not only about the `PostgisGeometryDecoder` but depends also on `WKTWriter`. I am wondering if it makes sense to test the decoder independently by explicitly creating the expected Point, Linestring, etc., object and rely on equality comparison between objects.
##########
core/src/test/java/org/apache/calcite/util/PostgisGeometryDecoderTest.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.util;
+
+import org.junit.jupiter.api.Test;
+import org.locationtech.jts.geom.Geometry;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.io.WKTWriter;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for the {@link PostgisGeometryDecoder} class. These tests are based
+ * on the values retrieved from PostGIS using the JDBC driver for PostgreSQL.
+ */
+class PostgisGeometryDecoderTest {
+
+ private static final String POINT = "0101000020E6100000000000000000F03F000000"
+ + "0000000040";
+
+ private static final String LINESTRING = "0102000020E610000003000000000000000"
+ + "00000000000000000000000000000000000F03F000000000000F03F000000000000004"
+ + "00000000000000040";
+
+ private static final String POLYGON = "0103000020E610000002000000050000000000"
+ + "0000000000000000000000000000000000000000F03F00000000000000000000000000"
+ + "00F03F000000000000F03F0000000000000000000000000000F03F0000000000000000"
+ + "000000000000000005000000000000000000E03F000000000000E03F000000000000E0"
+ + "3F333333333333E33F333333333333E33F333333333333E33F333333333333E33F0000"
+ + "00000000E03F000000000000E03F000000000000E03F";
+
+ private static final String MULTIPOINT = "0104000020E610000002000000010100000"
+ + "0000000000000000000000000000000000101000000000000000000F03F00000000000"
+ + "0F03F";
+
+ private static final String MULTILINESTRING = "0105000020E6100000020000000102"
+ + "0000000200000000000000000000000000000000000000000000000000F03F00000000"
+ + "0000F03F01020000000200000000000000000000400000000000000040000000000000"
+ + "08400000000000000840";
+
+ private static final String MULTIPOLYGON = "0106000020E6100000020000000103000"
+ + "000010000000500000000000000000000000000000000000000000000000000F03F000"
+ + "0000000000000000000000000F03F000000000000F03F0000000000000000000000000"
+ + "000F03F000000000000000000000000000000000103000000010000000500000000000"
+ + "0000000004000000000000000400000000000000840000000000000004000000000000"
+ + "0084000000000000008400000000000000040000000000000084000000000000000400"
+ + "000000000000040";
+
+ private static final String GEOMETRYCOLLECTION = "0107000020E6100000020000000"
+ + "1010000000000000000000000000000000000000001020000000200000000000000000"
+ + "0F03F000000000000F03F00000000000000400000000000000040";
+
+ @Test void decodeNull() {
+ Geometry geometry = PostgisGeometryDecoder.decode((String) null);
+ assertEquals(null, geometry);
+ }
+
+ @Test void decodePoint() {
+ Geometry geometry = PostgisGeometryDecoder.decode(POINT);
+ assertTrue(geometry instanceof Point);
+ assertEquals("POINT (1 2)", new WKTWriter().write(geometry));
Review Comment:
It seems that we don't need both assertions since if equality succeeds it implies that its a point. SImilar for the other test cases.
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1511,6 +1513,21 @@ public static SqlNode toSql(RexLiteral literal) {
// Create a string without specifying a charset
return SqlLiteral.createCharString((String) castNonNull(literal.getValue2()), POS);
}
+ case GEOMETRY: {
+ Geometry geom = castNonNull(literal.getValueAs(Geometry.class));
+ switch (typeName) {
+ case CHAR:
+ case VARCHAR:
+ return SqlLiteral.createCharString(
+ SpatialTypeUtils.asEwkt(geom), POS);
+ case BINARY:
+ case VARBINARY:
+ return SqlLiteral.createBinaryString(
+ SpatialTypeUtils.asWkbArray(geom), POS);
+ default:
+ return SqlLiteral.createNull(POS);
Review Comment:
Indeed it is not clear why we return null here. It doesn't feel right.
##########
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java:
##########
@@ -323,6 +323,14 @@ private static void generateGet(EnumerableRelImplementor implementor,
java.sql.Array.class);
source = Expressions.call(BuiltInMethod.JDBC_ARRAY_TO_LIST.method, x);
break;
+ case GEOMETRY:
Review Comment:
Are there tests that cover this path?
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeAssignmentRule.java:
##########
@@ -193,6 +193,8 @@ private SqlTypeAssignmentRule(
rule.add(SqlTypeName.GEOMETRY);
rule.add(SqlTypeName.CHAR);
rule.add(SqlTypeName.VARCHAR);
+ rule.add(SqlTypeName.BINARY);
Review Comment:
I kind of agree with Mihai. Some changes in this PR such as those around binary types seem independent to to PostGIS. Would it be possible to isolate them in separate JIRA/PRs?
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
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 <code>SqlDialect</code> 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2422749674
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
Maybe I missundersood the originial comment. However, since my PR introduced a new createJavaType method with two arguments, we wanted to keep the existing single-argument createJavaType method to avoid breaking consumers.
If I recall correctly, my fix (on 30.07.2025) involved adding a default implementation for the single-argument version, as shown below:
```java
default RelDataType createJavaType(Class clazz) {
return createJavaType(clazz, null);
}
RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-3382629065
@zabetak do you think you'd have a bit of time to take a look at the latest changes after your remarks please?
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2422692135
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
@bchapuis I cannot see the default implementation suggested by @zabetak to avoid breaking consumers.
IIANM the proposal to maintain compatibility was:
```
RelDataType createJavaType(Class clazz);
default RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family) {
return createJavaType(clazz);
}
```
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2423457982
##########
core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java:
##########
@@ -65,6 +65,15 @@ public interface RelDataTypeFactory {
*/
RelDataType createJavaType(Class clazz);
+ /**
+ * Creates a type that corresponds to a Java class, with a given family.
+ *
+ * @param clazz the Java class used to define the type
+ * @param family The family of the type, or null to infer
+ * @return canonical Java type descriptor
+ */
+ RelDataType createJavaType(Class clazz, @Nullable RelDataTypeFamily family);
Review Comment:
This change comes from a suggestion made by @julianhyde in https://github.com/apache/calcite/commit/da8494213c3810584a75064d5fadf52ac1f95d12
> Add annotation Parameter.sqlType
The annotation allows you to define the SQL type of a
parameter, for cases where it cannot be deduced automatically
from the Java type. This is especially useful for GEOMETRY
values, for which Calcite has no built-in Java type. (JTS
Geometry is widely used in Calcite, but it is an
implementation, and we do not want it to become the
specification.)
>
> This commit is incomplete, but illustrates the general idea.
>
> A related change is to add a 'family' parameter to
the `TypeFactory.createJavaType` method. It is nullable, and
if null, the type becomes its own family. In JavaType, we
should add a `family` field.
>
> When this change is complete we should be able to remove JTS
Geometry from RelDataTypeFactoryImpl.CLASS_FAMILIES.
In my understanding this proposition was an API change (possibly major) that would allow to define the SQL type of a parameter more explicitly. As the family parameter is nullable, it effectively allows the cosumers to continue working as before. I acknoledge that I probably don't get all the possible ramifications of this 😅
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2423090722
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
I appreciate your work @bchapuis .
Regarding `JavaTypeFactoryImpl` vs `RelDataTypeFactoryImpl` discussion, a small note that may be worth considering is that `JavaTypeFactoryImpl` is marked as "experimental and subject to change/removal without notice", so it might seem a better fit for experimental things that we may need to revisits in the future.
I admit that I haven't followed all the discussions involving this issue, but I can see there are still some valid open points, like RelToSql becoming coupled with `org.locationtech`, test coverage for the new conversion paths, the default implementation of the new method in `RelDataTypeFactory`, and where to include our actual GEO implementation (`JavaTypeFactoryImpl` vs `RelDataTypeFactoryImpl`).
Also there's this comment by @zabetak :
> Another side concern with this implementation is that the caller is supposed to pass explicitly the family to use but it seems that for the Geometry instances it is not possible. This behavior seems to be inconsistent with the API specification.
I think we can try our best to not let this PR stale and finalize it asap, but probably the timing would be better for 1.42.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2243938634
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -116,6 +117,7 @@
import org.checkerframework.checker.initialization.qual.UnknownInitialization;
import org.checkerframework.checker.nullness.qual.Nullable;
+import org.locationtech.jts.geom.Geometry;
Review Comment:
Would this imply adding a wrapper in Calcite to reduce coupling to JTS? I’m still not convinced that the added complexity is worth it. JTS is almost a standard for spatial functions and operators. GEOS, which is used in Postgis, is actually a C++ port of JTS, and their communities overlap significantly. To put it another way, I think JTS is almost to geometry what Guava is to immutable maps and lists ;)
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-3138017007
@zabetak Thanks a lot for your review. I think most of the comments have been addressed. Please let me know if you want to further discuss de decoupling of JTS.
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2422752859
##########
core/src/main/java/org/apache/calcite/jdbc/JavaTypeFactoryImpl.java:
##########
@@ -440,4 +442,17 @@ private static class RecordFieldImpl implements Types.RecordField {
return syntheticType;
}
}
+
+ @Override public RelDataType createJavaType(Class clazz) {
+ return createJavaType(clazz, null);
+ }
+
+ @Override public RelDataType createJavaType(
+ Class clazz,
+ @Nullable RelDataTypeFamily family) {
+ if (Geometry.class.isAssignableFrom(clazz)) {
+ return canonize(new JavaType(clazz, SqlTypeFamily.GEOMETRY));
+ }
+ return super.createJavaType(clazz, family);
+ }
Review Comment:
@rubenada @zabetak Let me revive this thread, as I believe it’s important to decide whether to merge the PR or give it another round of modifications. Please note that I don’t want to block the current release cycle, but I’d really appreciate some hints on how to implement this 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239, CALCITE-6263] Add PostGIS Dialect to Apache Calcite [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
bchapuis commented on code in PR #3668:
URL: https://github.com/apache/calcite/pull/3668#discussion_r2422787534
##########
core/src/main/java/org/apache/calcite/prepare/CalcitePrepareImpl.java:
##########
@@ -837,7 +837,7 @@ private static ColumnMetaData.AvaticaType avaticaType(JavaTypeFactory typeFactor
}
return ColumnMetaData.struct(columns);
case ExtraSqlTypes.GEOMETRY:
- typeOrdinal = Types.VARCHAR;
+ typeOrdinal = Types.OTHER;
Review Comment:
Thanks for the clarification, I added a note in history.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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #3668:
URL: https://github.com/apache/calcite/pull/3668#issuecomment-2914394881
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3668) **Quality Gate passed**
Issues
 [158 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3668&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3668&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3668&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "YiwenWu (via GitHub)" <gi...@apache.org>.
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 <code>SqlDialect</code> 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "macroguo-ghy (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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 <code>SqlDialect</code> 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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6239] Add a postgis dialect that supports ST functions [calcite]
Posted by "bchapuis (via GitHub)" <gi...@apache.org>.
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org