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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
