Skip to content

Add support for geometry type in mysql connector#24996

Merged
ZacBlanco merged 1 commit into
prestodb:masterfrom
Mariamalmesfer:support-geometry-mysqlconnector
May 22, 2025
Merged

Add support for geometry type in mysql connector#24996
ZacBlanco merged 1 commit into
prestodb:masterfrom
Mariamalmesfer:support-geometry-mysqlconnector

Conversation

@Mariamalmesfer

@Mariamalmesfer Mariamalmesfer commented Apr 28, 2025

Copy link
Copy Markdown
Contributor

Description

Added support for the GEOMETRY type in the MySQL connector.

Motivation and Context

To enable reading geometry data from MySQL tables using Presto.

Impact

Queries with GEOMETRY columns in MySQL will now be supported.

Test Plan

Covered in TestMySqlIntegrationSmokeTest with POINT, LINESTRING, and POLYGON.

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

MYSQL Connector Changes
* Add support for GEOMETRY type in the MySQL connector.

@prestodb-ci prestodb-ci added the from:IBM PR from IBM label Apr 28, 2025
@linux-foundation-easycla

linux-foundation-easycla Bot commented Apr 28, 2025

Copy link
Copy Markdown

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Mariamalmesfer / name: Mariam AlMesfer (62e6371)

@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch 5 times, most recently from a6417e3 to 0900b91 Compare April 30, 2025 08:27
@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch 3 times, most recently from 0b60212 to bbba4ee Compare May 6, 2025 10:29
@Mariamalmesfer Mariamalmesfer marked this pull request as ready for review May 6, 2025 12:22
@Mariamalmesfer Mariamalmesfer requested a review from a team as a code owner May 6, 2025 12:22
@Mariamalmesfer Mariamalmesfer requested a review from jaystarshot May 6, 2025 12:22
@prestodb-ci prestodb-ci requested review from a team, BryanCutler and nmahadevuni and removed request for a team May 6, 2025 12:22
@steveburnett

Copy link
Copy Markdown
Contributor

Do the Type Mapping tables in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/mysql.rst#type-mapping need to be updated with this added type?

@infvg infvg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work LGTM logic is great just some minor code style suggestions

Comment thread presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated
Comment thread presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated
Comment thread presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated
Comment thread presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated
Comment thread presto-postgresql/pom.xml Outdated
@prestodb-ci

Copy link
Copy Markdown
Contributor

@ethanyzhang imported this issue as lakehouse/tracker #24996

@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch from bbba4ee to 72ae512 Compare May 6, 2025 18:01
@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch from 72ae512 to 4c7408d Compare May 6, 2025 18:02
@Mariamalmesfer

Copy link
Copy Markdown
Contributor Author

Do the Type Mapping tables in https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/mysql.rst#type-mapping need to be updated with this added type?

Yes, I added the required change. The MySQL to PrestoDB type mapping table is updated to include GEOMETRY as VARBINARY

@steveburnett steveburnett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build, looks good.

This updates the table in MySQL to PrestoDB type mapping, but should a similar line be added to the table in PrestoDB to MySQL type mapping?

@Mariamalmesfer

Mariamalmesfer commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

LGTM! (docs)

Pull branch, local doc build, looks good.

This updates the table in MySQL to PrestoDB type mapping, but should a similar line be added to the table in PrestoDB to MySQL type mapping?

yes I only updated the MySQL to PrestoDB type mapping table. the change only adds read support for GEOMETRY. Since we’re reading it as VARBINARY, it’s not supported for writing

@steveburnett

Copy link
Copy Markdown
Contributor

yes I only updated the MySQL to PrestoDB type mapping table. the change only adds read support for GEOMETRY. Since we’re reading it as VARBINARY, it’s not supported for writing

Thanks for the explanation! Makes sense.

steveburnett
steveburnett previously approved these changes May 6, 2025

@steveburnett steveburnett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! (docs)

@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch 5 times, most recently from 18012ef to bb674b0 Compare May 16, 2025 06:28
Comment thread presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated
@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch 2 times, most recently from 37a1d69 to 58f2c78 Compare May 19, 2025 12:47
Comment thread presto-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlClient.java Outdated
Comment thread presto-mysql/src/test/java/com/facebook/presto/plugin/mysql/TestMySqlClient.java Outdated
@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch from 58f2c78 to 1d14a73 Compare May 19, 2025 20:27
Comment thread presto-mysql/pom.xml Outdated

@ZacBlanco ZacBlanco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also just realized you are using assertion functions from the assertj library. We use the assertion functions from org.testng. Please replace your usages of assertj with our standard assertion functions.

@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch from 1d14a73 to 2fcbfa3 Compare May 19, 2025 22:00
@Mariamalmesfer

Copy link
Copy Markdown
Contributor Author

I also just realized you are using assertion functions from the assertj library. We use the assertion functions from org.testng. Please replace your usages of assertj with our standard assertion functions.

Thanks for pointing that out. Done Ihave updated the assertions functions to use org.testng.Assert.

@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch 3 times, most recently from 9877f90 to d8ae77b Compare May 20, 2025 08:33
Comment thread presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated
@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch 4 times, most recently from 184a22f to a17e98c Compare May 21, 2025 17:04
Co-authored-by: pratyakshsharma  <pratyaksh13@gmail.com>3@gmail.com>
Co-authored-by: agrawalreetika <agrawal.reetika786@gmail.com>
@Mariamalmesfer Mariamalmesfer force-pushed the support-geometry-mysqlconnector branch from a17e98c to 62e6371 Compare May 21, 2025 18:58

@ZacBlanco ZacBlanco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@ZacBlanco ZacBlanco changed the title Added support for geometry type in mysql connector Add support for geometry type in mysql connector May 22, 2025
@ZacBlanco ZacBlanco merged commit 274e05a into prestodb:master May 22, 2025
99 checks passed
@ZacBlanco ZacBlanco mentioned this pull request May 29, 2025
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:IBM PR from IBM

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants