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