Skip to content
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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

bchapuis
Copy link
Member

@bchapuis bchapuis commented Feb 2, 2024

In JDBC adapter, add a PostGIS dialect so that ST functions can be pushed down

@bchapuis bchapuis force-pushed the 6239-postgis-dialect branch from 26be417 to 4869424 Compare February 2, 2024 23:49
@bchapuis
Copy link
Member Author

bchapuis commented Feb 3, 2024

@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.

@macroguo-ghy
Copy link
Contributor

@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.

@JiajunBernoulli
Copy link
Contributor

@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.

There are some integration tests for Druid: https://github.com/zabetak/calcite-druid-dataset
Here is CI config in calcite:

repository: zabetak/calcite-druid-dataset

@bchapuis
Copy link
Member Author

bchapuis commented Feb 5, 2024

Thanks a lot for the pointer, I will adopt the same approach and implement some integration tests in a third-party repository for now.

@bchapuis bchapuis force-pushed the 6239-postgis-dialect branch from 9b9f342 to 95c33c9 Compare February 7, 2024 11:10
@bchapuis bchapuis force-pushed the 6239-postgis-dialect branch from d4afb29 to 3b37b54 Compare February 8, 2024 16:37
Copy link
Contributor

@julianhyde julianhyde left a 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.

@@ -39,9 +39,11 @@
import com.google.common.collect.Interners;

import org.checkerframework.checker.nullness.qual.Nullable;
import org.locationtech.jts.geom.Geometry;
Copy link
Contributor

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?

Copy link
Member Author

@bchapuis bchapuis Feb 13, 2024

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?

Copy link
Member Author

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
  }
}

Copy link
Contributor

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.

Copy link
Member Author

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

core/src/main/java/org/apache/calcite/sql/SqlDialect.java Outdated Show resolved Hide resolved

@Override public void unparseCall(SqlWriter writer, SqlCall call, int leftPrec, int rightPrec) {
if (call.getOperator().getName().equals("ST_UNARYUNION")) {
RelToSqlConverterUtil.specialOperatorByName("ST_UNION")
Copy link
Contributor

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?

@bchapuis bchapuis force-pushed the 6239-postgis-dialect branch from c44f5c6 to 5fb31b8 Compare February 13, 2024 21:55
@bchapuis bchapuis force-pushed the 6239-postgis-dialect branch from 5e0a0c7 to 40d4418 Compare February 14, 2024 12:17
@bchapuis bchapuis force-pushed the 6239-postgis-dialect branch from 9aed8e5 to 75893e7 Compare February 14, 2024 13:28
Copy link

sonarcloud bot commented Feb 14, 2024

@@ -433,6 +433,17 @@ private static RelDataType sqlType(RelDataTypeFactory typeFactory, int dataType,
typeFactory.createSqlType(SqlTypeName.ANY), true);
}
return typeFactory.createArrayType(component, -1);
case GEOMETRY:
Copy link
Contributor

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) {
Copy link
Contributor

@YiwenWu YiwenWu Feb 22, 2024

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.

julianhyde and others added 4 commits March 7, 2024 23:13
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.
@mihaibudiu
Copy link
Contributor

Will this be ready for 1.37?

@bchapuis
Copy link
Member Author

bchapuis commented Apr 9, 2024

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.

https://issues.apache.org/jira/browse/CALCITE-6239

Copy link

github-actions bot commented Dec 6, 2024

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.

@github-actions github-actions bot added the stale label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants