[feature](function) Add ST_NumGeometries, ST_NumPoints, ST_Geometries functions#63049
Open
acarofpigs wants to merge 4 commits into
Open
[feature](function) Add ST_NumGeometries, ST_NumPoints, ST_Geometries functions#63049acarofpigs wants to merge 4 commits into
acarofpigs wants to merge 4 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
acarofpigs
force-pushed
the
add-geo-functions
branch
from
0a9a3ff to
e870bd2
Compare
Author
|
run buildall |
Author
|
/review |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
acarofpigs
force-pushed
the
add-geo-functions
branch
from
facf26f to
0d5d800
Compare
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
acarofpigs
force-pushed
the
add-geo-functions
branch
from
0d5d800 to
0fdb594
Compare
acarofpigs
force-pushed
the
add-geo-functions
branch
from
0fdb594 to
8feb287
Compare
Author
|
run buildall |
1 similar comment
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 30924 ms |
Contributor
TPC-DS: Total hot run time: 170352 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
acarofpigs
force-pushed
the
add-geo-functions
branch
from
303b19f to
fb632cd
Compare
Author
|
run buildall |
acarofpigs
force-pushed
the
add-geo-functions
branch
from
fb632cd to
4eef041
Compare
Author
|
run buildall |
acarofpigs
force-pushed
the
add-geo-functions
branch
from
4eef041 to
fb632cd
Compare
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31061 ms |
Contributor
TPC-DS: Total hot run time: 169633 ms |
Author
|
run cloud_p0 |
Contributor
FE UT Coverage ReportIncrement line coverage |
Author
|
run external |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 31101 ms |
Contributor
TPC-DS: Total hot run time: 169952 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
acarofpigs
force-pushed
the
add-geo-functions
branch
from
b07854b to
fb632cd
Compare
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31041 ms |
Contributor
TPC-DS: Total hot run time: 170122 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Author
|
run beut |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Author
|
/review |
1 similar comment
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one blocking correctness issue in the new spatial function behavior.
Critical checkpoint conclusions:
- Goal and tests: The PR adds ST_NumGeometries, ST_NumPoints, and ST_Geometries with FE/BE registration plus BE, FE, and regression tests. The tests cover normal geometry/null/invalid paths, but one expected result codifies an invalid sentinel value.
- Scope/focus: The actual GitHub PR diff is focused on the spatial functions and their tests.
- Concurrency/lifecycle/config/transaction/persistence: Not applicable; this is scalar function execution and type registration only.
- FE-BE consistency: Mostly aligned for function names and return families, but ST_NumPoints currently exposes an internal BE sentinel for unsupported shapes.
- Data correctness: Blocking issue below; valid input can return -1 as a point count.
- Tests/results: Regression output currently expects -1 for ST_Circle, which should be corrected together with the implementation.
- Observability/performance: No additional observability required; performance is acceptable for scalar spatial decoding at this scope.
User focus: No additional user-provided review focus was supplied.
| } | ||
|
|
||
| res->insert_value(shape->num_points()); | ||
| } |
Contributor
There was a problem hiding this comment.
GeoShape::num_points() uses -1 as the base-class sentinel for shapes that do not implement a point count, and GeoCircle does not override it. This line therefore returns -1 for ST_NumPoints(ST_Circle(...)) (the new regression output also locks that in), which exposes an internal sentinel as a user-visible count. A point-count function should not return a negative count for a valid geometry; please convert negative num_points() results to NULL (or define a real non-negative count for circles) before inserting the value.
Contributor
Possible file(s) that should be tracked in LFS detected: 🚨The following file(s) exceeds the file size limit:
Consider using |
github-actions
Bot
added
the
lfs-detected!
acarofpigs
force-pushed
the
add-geo-functions
branch
from
b37bae6 to
29bda20
Compare
Author
|
run buildall |
1 similar comment
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31517 ms |
Contributor
TPC-DS: Total hot run time: 171687 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE UT Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… functions
What problem does this PR solve?
Issue Number: ref #48203
Related PR: apache/doris-website#3623
Problem Summary:
Add three new spatial functions for geometry collection operations:
ST_NumGeometries: Returns the number of sub-geometries in a geometry object.ST_NumPoints: Returns the total number of vertices (points) in a geometry object.ST_Geometries: Decomposes a geometry object into an array of its sub-geometries.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)