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