Skip to content

Port 'geometry_nearest_points' UDF#8280

Merged
martint merged 1 commit intotrinodb:masterfrom
art4ul:geometry_nearest_points
Jun 28, 2021
Merged

Port 'geometry_nearest_points' UDF#8280
martint merged 1 commit intotrinodb:masterfrom
art4ul:geometry_nearest_points

Conversation

@art4ul
Copy link
Contributor

@art4ul art4ul commented Jun 15, 2021

The port of the 'geometry_nearest_points' UDF from PrestoDB

The 'geometry_nearest_points' function returns the points on each geometry nearest the other. If either geometry is empty, return NULL. Otherwise, return an array of two Points that have the minimum distance of any two points on the geometries. The first Point will be from the first Geometry argument, the second from the second Geometry argument. If there are multiple pairs with the minimum distance, one pair is chosen arbitrarily.

Extracted-From: https://github.com/prestodb/presto/pull/14923

@cla-bot
Copy link

cla-bot bot commented Jun 15, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

Copy link
Member

Choose a reason for hiding this comment

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

Since the function always returns two points, it would be more natural to return a row(geometry, geometry) (with each element being a point) instead of an array containing two elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martint Good point. But it’s a port of the existing functionality and I tried to keep the original code of the author as much as it possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martint I've changed the function in order to return a row(geometry, geometry) . Please check it out :)

@cla-bot
Copy link

cla-bot bot commented Jun 16, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@art4ul art4ul force-pushed the geometry_nearest_points branch from 4dc437a to 613a893 Compare June 16, 2021 13:54
@cla-bot
Copy link

cla-bot bot commented Jun 16, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@art4ul
Copy link
Contributor Author

art4ul commented Jun 16, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

I've already sent the email with signed CLA. Please check it out.

@art4ul art4ul force-pushed the geometry_nearest_points branch from 613a893 to 5174f9e Compare June 16, 2021 14:19
@cla-bot
Copy link

cla-bot bot commented Jun 16, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@art4ul art4ul requested a review from martint June 16, 2021 14:37
@martint
Copy link
Member

martint commented Jun 18, 2021

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Jun 18, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@cla-bot
Copy link

cla-bot bot commented Jun 18, 2021

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

Copy link
Member

Choose a reason for hiding this comment

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

This should go in the "Operations" section above. Also, make sure to place in the correct alphabetical position.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 1200 to 1201
Copy link
Member

Choose a reason for hiding this comment

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

Move this below the if (leftGeometry.isEmpty() || rightGeometry.isEmpty()) { check below to avoid constructing these objects in the case of empty input geometries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Fixed.

@cla-bot
Copy link

cla-bot bot commented Jun 22, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

@art4ul
Copy link
Contributor Author

art4ul commented Jun 22, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

Seems my CLA still not accepted .

@art4ul art4ul requested a review from martint June 22, 2021 11:26
@martint
Copy link
Member

martint commented Jun 22, 2021

Seems my CLA still not accepted .

That's because the author of one of the commits is not in the CLA database. Ignore that for now. We've received your CLA and have added it to the tooling.

@art4ul
Copy link
Contributor Author

art4ul commented Jun 24, 2021

Seems my CLA still not accepted .

That's because the author of one of the commits is not in the CLA database. Ignore that for now. We've received your CLA and have added it to the tooling.

@martint As far as I propose the port of the existed in PrestoDB functionality, the author of the one of commits is the original author of the prestodb/presto#14923

I've made following commit in order to save the name of the original author :

git commit --amend --author 'James Gill <jagill@fb.com>' -m 'Add geometry_nearest_points function
Extracted-From: https://github.com/prestodb/presto/pull/14923'

@JamesRTaylor
Copy link

Ping @martint. Any more feedback for @art4ul?

@martint martint force-pushed the geometry_nearest_points branch from c333676 to 835c681 Compare June 28, 2021 18:34
@cla-bot cla-bot bot added the cla-signed label Jun 28, 2021
@martint martint self-assigned this Jun 28, 2021
@martint martint merged commit d56a6d9 into trinodb:master Jun 28, 2021
@martint martint added this to the 359 milestone Jun 28, 2021
@martint martint mentioned this pull request Jun 28, 2021
13 tasks
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.

3 participants