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

feat(query): implement ST_YMAX/ST_XMIN/ST_YMIN #15594

Merged
merged 2 commits into from
May 27, 2024

Conversation

kkk25641463
Copy link
Contributor

@kkk25641463 kkk25641463 commented May 21, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

feat: Implement ST_YMAX/ST_XMIN/ST_YMIN with issues

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label May 21, 2024
@b41sh b41sh self-requested a review May 21, 2024 07:32
src/query/functions/src/scalars/geometry.rs Outdated Show resolved Hide resolved
src/query/functions/src/scalars/geometry.rs Outdated Show resolved Hide resolved
src/query/functions/src/scalars/geometry.rs Outdated Show resolved Hide resolved
src/query/functions/src/scalars/geometry.rs Outdated Show resolved Hide resolved
@kkk25641463
Copy link
Contributor Author

And more code clean:

  1. With geozero srid issues resolved in version 0.13.0. The srid was get from geos struct.
  2. Geozero project maintainer believes that displaying SRID=0 is meaningless in the realm of spatial information. So test case with srid=0 is not display with geozero lib.

@kkk25641463
Copy link
Contributor Author

kkk25641463 commented May 22, 2024

The implementation of the st_transform function has been affected by the static linking of georust/proj. Although it seems like fixed in the main branch, the release cycle of the proj project spans years. Additionally, support for z/m coordinates in georust/geo has also long lacked organized discussions and reviews by maintainers. Given this situation, can we consider incorporating these two projects into the mainline code of Databend for maintenance?Once the georust community's feature mature, we will switch back to using the community libraries.

@b41sh
Copy link
Member

b41sh commented May 22, 2024

The implementation of the st_transform function has been affected by the static linking of georust/proj. Although it seems like fixed in the main branch, the release cycle of the proj project spans years. Additionally, support for z/m coordinates in georust/geo has also long lacked organized discussions and reviews by maintainers. Given this situation, can we consider incorporating these two projects into the mainline code of Databend for maintenance?Once the georust community's feature mature, we will switch back to using the community libraries.

Hi @kkk25641463 You can merge with the commit id first, and then change back when the upstream repository code is merged.

Signed-off-by: Fan Yang <[email protected]>
@b41sh b41sh added this pull request to the merge queue May 27, 2024
@BohuTANG BohuTANG removed this pull request from the merge queue due to a manual request May 27, 2024
@BohuTANG BohuTANG merged commit ce031db into databendlabs:main May 27, 2024
72 checks passed
@kkk25641463 kkk25641463 deleted the ST_YMAX branch May 27, 2024 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants