-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CALCITE-6239] Add a postgis dialect that supports ST functions #3668
base: main
Are you sure you want to change the base?
Conversation
26be417
to
4869424
Compare
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java
Show resolved
Hide resolved
@YiwenWu @JiajunBernoulli What would be the best way to test these changes against a postgis database? Ideally, I'd like to add an integration test that executes queries against postgis (e.g. with testcontainers). But I havn't been able to find such tests in calcite. |
I remember that testContainer has been used in redis apapter test. |
There are some integration tests for Druid: https://github.com/zabetak/calcite-druid-dataset calcite/.github/workflows/main.yml Line 440 in 2aabf21
|
Thanks a lot for the pointer, I will adopt the same approach and implement some integration tests in a third-party repository for now. |
9b9f342
to
95c33c9
Compare
d4afb29
to
3b37b54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the summary to "In JDBC adapter, add a PostGIS dialect so that ST_ functions can be pushed down". It will make the release notes easier to understand.
core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcToEnumerableConverter.java
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java
Outdated
Show resolved
Hide resolved
@@ -39,9 +39,11 @@ | |||
import com.google.common.collect.Interners; | |||
|
|||
import org.checkerframework.checker.nullness.qual.Nullable; | |||
import org.locationtech.jts.geom.Geometry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want this dependency here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to wrap my head around this. From what I understand, calling a method of RelDataTypeFactoryImpl without JTS would trigger a class not found exception. As the use of JTS is currently circumscribed to a few spatial type classes, calcite works just fine without adding a dependency to JTS. Is that correct?
Given the popularity of JTS in the geospatial world, I wouldn't try to abstract it completely in JTS. An option could be to use reflection to dynamically load the Geometry class and provide some sort of fallback if the class is not in the class path.
public static class GeometryFallback { }
static Class<?> loadGeometryClass() {
try {
return Class.forName("org.locationtech.jts.geom.Geometry");
} catch (ClassNotFoundException e) {
return GeometryFallback.class;
}
}
private static final Map<Class, RelDataTypeFamily> CLASS_FAMILIES =
ImmutableMap.<Class, RelDataTypeFamily>builder()
.put(String.class, SqlTypeFamily.CHARACTER)
// ...
.put(loadGeometryClass(), SqlTypeFamily.GEO)
.build();
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option could be to add this type only when JTS is in the classpath.
static {
try {
CLASS_FAMILIES.put(Class.forName("org.locationtech.jts.geom.Geometry"), SqlTypeFamily.GEO);
} catch (ClassNotFoundException e) {
// JTS is not available in the classpath
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we discuss in jira? Increasing the coupling to JTS is an architectural change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, here is the jira issue I just created:
https://issues.apache.org/jira/browse/CALCITE-6263
|
||
@Override public void unparseCall(SqlWriter writer, SqlCall call, int leftPrec, int rightPrec) { | ||
if (call.getOperator().getName().equals("ST_UNARYUNION")) { | ||
RelToSqlConverterUtil.specialOperatorByName("ST_UNION") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use SqlKind rather than name?
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/sql/dialect/PostgisSqlDialect.java
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/sql/dialect/PostgisGeometryDecoder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java
Show resolved
Hide resolved
This prevent reflection errors at runtime.
For now, let's assume that most databases behave as postgis.
c44f5c6
to
5fb31b8
Compare
5e0a0c7
to
40d4418
Compare
9aed8e5
to
75893e7
Compare
Quality Gate passedIssues Measures |
@@ -433,6 +433,17 @@ private static RelDataType sqlType(RelDataTypeFactory typeFactory, int dataType, | |||
typeFactory.createSqlType(SqlTypeName.ANY), true); | |||
} | |||
return typeFactory.createArrayType(component, -1); | |||
case GEOMETRY: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the elements in SqlTypeName#JDBC_TYPE_TO_NAME
, whether there are any cases that can be matched GEOMETRY
?
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that typeName CHAR
or VARCHAR
can be obtained directly under GEO
family.
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.) This commit is incomplete, but illustrates the general idea. 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.
Will this be ready for 1.37? |
No, I hadn't enought time to work on this and the decoupling of the GEOMETRY type from the JTS Geometry class will require additional efforts. |
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
In JDBC adapter, add a PostGIS dialect so that ST functions can be pushed down