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