Skip to content

make Geometry comparable#25225

Merged
martint merged 1 commit into
trinodb:masterfrom
justin2004:geometry-comparable
Apr 24, 2025
Merged

make Geometry comparable#25225
martint merged 1 commit into
trinodb:masterfrom
justin2004:geometry-comparable

Conversation

@justin2004
Copy link
Copy Markdown
Member

@justin2004 justin2004 commented Mar 5, 2025

In PostGIS, instances of Geometry are comparable (that is, you can run select distinct on those columns).
This PR allows Trino to do the same and addresses #24961.

e.g. with the patch applied:

trino> describe postgis.public.movement;                            
     Column      |     Type     | Extra | Comment 
-----------------+--------------+-------+---------
 id              | integer      |       |         
 thing_id        | integer      |       |         
 geometry        | Geometry     |       |         
 geometry_string | varchar(50)  |       |         
 dt              | timestamp(6) |       |          
 speed_mph       | integer      |       |         
(6 rows)                                                           

Query 20250305_174904_00016_abpcz, FINISHED, 1 node                                                                                    
Splits: 19 total, 19 done (100.00%)
0.40 [6 rows, 364B] [14 rows/s, 901B/s]
trino> select geometry from postgis.public.movement;
      geometry       
---------------------
 POINT (-90 38.5)    
 POINT (-90 38.501)  
 POINT (-90 38.5)    
 POINT (-90.1 38.58) 
(4 rows)

Query 20250305_174913_00017_abpcz, FINISHED, 1 node
Splits: 1 total, 1 done (100.00%)
0.23 [4 rows, 0B] [17 rows/s, 0B/s]

trino> select distinct geometry from postgis.public.movement;
      geometry       
---------------------
 POINT (-90 38.501)  
 POINT (-90 38.5)    
 POINT (-90.1 38.58) 
(3 rows)

Query 20250305_174919_00018_abpcz, FINISHED, 1 node
Splits: 1 total, 1 done (100.00%)
0.26 [3 rows, 0B] [11 rows/s, 0B/s]

EDIT:
I got the formatter to run (using intellij).

Release notes

# General
* Add support for comparing (`=`, `<>`) values of type `geometry`. ({issue}`25225`)

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 5, 2025

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Justin Dowdy.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 5, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@justin2004 justin2004 force-pushed the geometry-comparable branch from f4ee92f to 1417e95 Compare March 5, 2025 21:43
@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 5, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@justin2004
Copy link
Copy Markdown
Member Author

I've emailed the signed CLA.

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 5, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be

private static final TypeOperatorDeclaration TYPE_OPERATOR_DECLARATION = extractOperatorDeclaration(GeometryType.class, lookup(), Slice.class);

See UuidType for an example.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this but then the test failed:

[ERROR] Tests run: 4, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 34.16 s <<< FAILURE! -- in io.trino.plugin.geospatial.TestGeoSp
atialQueries                                                                                                                           
[ERROR] io.trino.plugin.geospatial.TestGeoSpatialQueries.testDistinctGeometry -- Time elapsed: 9.717 s <<< ERROR!                      
io.trino.testing.QueryFailedException: io.trino.spi.TrinoException: Geometry READ_VALUE operator can not be adapted to convention (([FL
AT])FAIL_ON_NULL). Available implementations: [([BLOCK_POSITION_NOT_NULL])FAIL_ON_NULL, ([NEVER_NULL])BLOCK_BUILDER]                   
        at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:138)                                    
        at io.trino.testing.DistributedQueryRunner.executeInternal(DistributedQueryRunner.java:565)                                    
        at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:548)                                            
        at io.trino.sql.query.QueryAssertions$QueryAssert.lambda$new$1(QueryAssertions.java:317)                                       
        at com.google.common.base.Suppliers$NonSerializableMemoizingSupplier.get(Suppliers.java:200)                                   
        at io.trino.sql.query.QueryAssertions$QueryAssert.result(QueryAssertions.java:436)                                             
        at io.trino.plugin.geospatial.TestGeoSpatialQueries.testDistinctGeometry(TestGeoSpatialQueries.java:58) 

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could implement isOrderable() = true as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you don't mind i'd like to do orderability in a follow on PR.

@electrum
Copy link
Copy Markdown
Member

electrum commented Mar 6, 2025

Can you add tests for this in TestGeoSpatialQueries?

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 6, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

1 similar comment
@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 6, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@justin2004
Copy link
Copy Markdown
Member Author

justin2004 commented Mar 6, 2025

Can you add tests for this in TestGeoSpatialQueries?

@electrum, done.

before this change the test says:

[ERROR]   TestGeoSpatialQueries.testDistinctGeometry:55 ? QueryFailed line 1:1: DISTINCT can only be applied to comparable types (actua
l: Geometry): geom

with this PR the test passes.

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 6, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

1 similar comment
@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented Mar 7, 2025

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@cla-bot cla-bot Bot added the cla-signed label Mar 17, 2025
@justin2004 justin2004 force-pushed the geometry-comparable branch from 8035f44 to aa71422 Compare March 17, 2025 18:35
@justin2004
Copy link
Copy Markdown
Member Author

@electrum , looks like all checks have passed

@justin2004 justin2004 requested a review from electrum March 17, 2025 22:05
@justin2004
Copy link
Copy Markdown
Member Author

hey @wendigo , anything else i need to do for this PR?
thanks

@wendigo
Copy link
Copy Markdown
Contributor

wendigo commented Mar 24, 2025

@justin2004 I wasn't involved in this review so I'd prefer to defer it to @electrum :)

@justin2004
Copy link
Copy Markdown
Member Author

hey @electrum , anything else you'd like me to do for this PR?

@github-actions github-actions Bot added release-notes docs ui Web UI jdbc Relates to Trino JDBC driver hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector mongodb MongoDB connector labels Apr 2, 2025
@ebyhr ebyhr removed jdbc Relates to Trino JDBC driver hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector bigquery BigQuery connector mongodb MongoDB connector snowflake Snowflake connector cassandra Cassandra connector blackhole Blackhole connector clickhouse ClickHouse connector druid Druid connector duckdb DuckDB connector elasticsearch Elasticsearch connector exasol Exasol connector faker Faker connector google-sheets Google Sheets connector ignite Ignite connector kafka Kafka connector loki Loki connector mariadb MariaDB connector memory Memory connector mysql MySQL connector opensearch OpenSearch connector labels Apr 21, 2025
@justin2004
Copy link
Copy Markdown
Member Author

hey @wendigo , would you be willing to take this PR review over?
it would be nice to not have to run a private fork of trino (with these changes) in the project i am working on.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering doesn't make sense for geometries.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, @martint . addressed.

Copy link
Copy Markdown
Member

@martint martint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I just have a couple of minor formatting-related comments.

Also, capitalize the commit message: "Make Geometry comparable"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the arguments to the previous line.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalize the SQL keywords. Also, use VALUES instead of UNION ALL between two queries to synthesize the base data, and format it as follows for readability:

assertThat(query("""
                 SELECT DISTINCT ST_GeometryFromText(point)
                 FROM (VALUES 'POINT (-90 38.99)', 'POINT (-90 38.99)') t(point)
                 """))
        .result().matches(MaterializedResult.resultBuilder(getSession(), GEOMETRY)
                .row("POINT (-90 38.99)")
                .build());

@justin2004
Copy link
Copy Markdown
Member Author

hey @martint , I made the changes as you requested!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants