server icon indicating copy to clipboard operation
server copied to clipboard

MDEV-17398 Implement ST_LatFromGeoHash and ST_LongFromGeoHash

Open 3abqreno opened this issue 10 months ago • 6 comments

Porting ST_LatFromGeoHash and ST_LongFromGeoHash from MySQL

  • [x] The Jira issue number for this PR is: MDEV-17398

Description

This PR ports two functions ST_LatFromGeoHash and ST_LongFromGeoHash from MySQL, the implementation was taken from item_geofunc.cc and item_geofunc.h, there were some changes to make the code work with MariaDB codebase.

How can this PR be tested?

Ported tests from MySQL from geohash_functions.test and geohash_functions.result.

the new tests were added to a file called gis_geohash_functions.test and gis_geohash_functions.result.

Basing the PR against the correct MariaDB version

  • [x] This is a new feature and the PR is based against the latest MariaDB development branch.

PR quality check

  • [x] I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • [x] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

3abqreno avatar Apr 01 '24 21:04 3abqreno

related #2570.

Would be good to attribute original authors of code base and reference the MySQL commits in the commit message.

grooverdan avatar Apr 02 '24 06:04 grooverdan

@grooverdan I have looked into the other PR and tried to and tried to add the original author to the commit but I couldn't find their email in github, you can find the original implementation PR here, I have added their name and commit hash to the commit message is that enough?

3abqreno avatar Apr 03 '24 21:04 3abqreno

Regarding the author attribution, https://github.com/mysql/mysql-server/commit/ad34780e.patch shows [email protected] I would suggest adding the email and full link to the commit.

robinnewhouse avatar Apr 03 '24 21:04 robinnewhouse

@HugoWenTD Thanks for noticing the typo.

@robinnewhouse Thank you I didn't know about the .patch thing it's very useful, I have made him the author as suggested in the other geohash function PR and I am now co-author. I have also added the original commit link in the commit message.

3abqreno avatar Apr 04 '24 14:04 3abqreno

@grooverdan I have done what people asked in the code review, should I wait for another review? I also have another question about the failing test, the 11.5 branch itself fails some tests so should I work on making them work? If I have to fix them how should I do that since they are on other distributions and on my PC the tests pass.

3abqreno avatar Apr 07 '24 20:04 3abqreno

I also have another question about the failing test, the 11.5 branch itself fails some tests so should I work on making them work? If I have to fix them how should I do that since they are on other distributions and on my PC the tests pass.

There are known issues here that are being worked on. I don't think you have triggered these, although this can be confirmed the next time we review it.

LinuxJedi avatar Apr 11 '24 12:04 LinuxJedi