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