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

Support reading geometries in EWKB format. #23894

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Conversation

umartin
Copy link
Contributor

@umartin umartin commented Oct 24, 2024

Description

Add support for parsing EKWB format in ST_GeomFromBinary

Additional context and related issues

EWKB is a popular extension to WKB.
Closes #23824

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: Add support for EWKB in ST_GeomFromBinary

Copy link

cla-bot bot commented Oct 24, 2024

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

@github-actions github-actions bot added the docs label Oct 24, 2024
Copy link

cla-bot bot commented Oct 24, 2024

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

cla-bot bot commented Oct 28, 2024

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 Nov 1, 2024
@wendigo
Copy link
Contributor

wendigo commented Nov 1, 2024

@umartin Instead of merge please rebase this PR

@ebyhr
Copy link
Member

ebyhr commented Nov 3, 2024

@cla-bot check

Copy link

cla-bot bot commented Nov 3, 2024

The cla-bot has been summoned, and re-checked this pull request!

@umartin umartin force-pushed the ewkb branch 2 times, most recently from 0e316f5 to 0ae8c45 Compare November 4, 2024 11:56
@wendigo wendigo merged commit f9b9049 into trinodb:master Nov 4, 2024
5 of 15 checks passed
@github-actions github-actions bot added this to the 465 milestone Nov 4, 2024
@wendigo
Copy link
Contributor

wendigo commented Nov 4, 2024

Thanks @umartin

@umartin
Copy link
Contributor Author

umartin commented Nov 4, 2024

Thank you @wendigo!

@umartin umartin deleted the ewkb branch November 5, 2024 07:20
@umartin
Copy link
Contributor Author

umartin commented Nov 6, 2024

Just to be cautious - is there a mechanism in Trino that prevents concurrent calls to geomFromBinary in the same JVM? Otherwise it is safer to create a new WKBReader instance for each call instead of a static instance.

"This class is designed to support reuse of a single instance to read multiple geometries. This class is not thread-safe; each thread should create its own instance."

https://locationtech.github.io/jts/javadoc/org/locationtech/jts/io/WKBReader.html

@martint
Copy link
Member

martint commented Nov 6, 2024

It's possible, but it requires functions to be implemented as stateful functions. See

for an example.

The implementation in this PR is, therefore, not thread-safe.

@umartin
Copy link
Contributor Author

umartin commented Nov 6, 2024

Got it, thanks! I'll open another PR removing the static field.

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.

Support reading geometries in EWKB format.
4 participants