fix: Improve jts polygon serde logic #26672
Open
Conversation
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
Contributor
Reviewer's GuideEnhanced JTS polygon serialization/deserialization to properly handle zero-area rings and enforce canonical orientation, with MultiPolygon serde now rejecting zero-area rings and extensive tests added for degenerate polygons. Sequence diagram for MultiPolygon deserialization with zero-area ring rejectionsequenceDiagram
participant "readPolygon()"
participant "isClockwise()"
participant "PrestoException"
"readPolygon()"->>"isClockwise()": Check ring orientation
"isClockwise()"-->>"readPolygon()": Return ZERO_AREA
"readPolygon()"->>"PrestoException": Throw error for zero-area ring
"PrestoException"-->>"readPolygon()": Exception propagates
Sequence diagram for MultiPolygon serialization with zero-area ring rejectionsequenceDiagram
participant "writePolygon()"
participant "canonicalizePolygonCoordinates()"
participant "PrestoException"
"writePolygon()"->>"canonicalizePolygonCoordinates()": Canonicalize rings
"canonicalizePolygonCoordinates()"-->>"writePolygon()": Return true if zero-area ring found
"writePolygon()"->>"PrestoException": Throw error for zero-area ring
"PrestoException"-->>"writePolygon()": Exception propagates
Class diagram for updated JtsGeometrySerde polygon serde logicclassDiagram
class JtsGeometrySerde {
+readPolygon(SliceInput input, boolean multitype) Geometry
+writePolygon(Geometry geometry, SliceOutput output, boolean multitype)
+canonicalizePolygonCoordinates(Coordinate[] coordinates, int[] partIndexes, boolean[] shellPart) boolean
+reverse(Coordinate[] coordinates, int start, int end)
+isClockwise(Coordinate[] coordinates) ClockwiseResult
+isClockwise(Coordinate[] coordinates, int start, int end) ClockwiseResult
}
class ClockwiseResult {
CW
CCW
ZERO_AREA
}
JtsGeometrySerde --> ClockwiseResult
Class diagram for ClockwiseResult enumclassDiagram
class ClockwiseResult {
<<enum>>
CW
CCW
ZERO_AREA
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Extract the zero-area ring detection and error handling in readPolygon and writePolygon into a shared helper to reduce duplication and consolidate behavior.
- The testDegeneratePolygons method has grown very large—consider breaking it into smaller parameterized tests or grouping similar cases to improve readability and maintenance.
- Resolve or clarify the TODO in readPolygon for distinguishing internal versus user errors to ensure consistent error classification and avoid leaking implementation details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the zero-area ring detection and error handling in readPolygon and writePolygon into a shared helper to reduce duplication and consolidate behavior.
- The testDegeneratePolygons method has grown very large—consider breaking it into smaller parameterized tests or grouping similar cases to improve readability and maintenance.
- Resolve or clarify the TODO in readPolygon for distinguishing internal versus user errors to ensure consistent error classification and avoid leaking implementation details.
## Individual Comments
### Comment 1
<location> `presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java:506` </location>
<code_context>
+ }
- if ((isShell && !isClockwise) || (!isShell && isClockwise)) {
+ if ((isShell && clockwiseResult == ClockwiseResult.CCW) || (!isShell && clockwiseResult == ClockwiseResult.CW)) {
// shell has to be counter clockwise
reverse(coordinates, start, end);
</code_context>
<issue_to_address>
**nitpick:** The comment above this line incorrectly states the required orientation.
Update the comment to state that shells must be clockwise and holes must be counter-clockwise, matching the code's logic.
</issue_to_address>
### Comment 2
<location> `presto-main-base/src/test/java/com/facebook/presto/geospatial/TestGeoFunctions.java:1433-1444` </location>
<code_context>
+ // Second polygon is zero area
+ testDegeneratePolygonsFuncInvalid("MULTIPOLYGON (((1 1, 2 1, 2 2, 1 1)), ((1 1, 2 2, 3 3, 2 2, 1 1)))");
+
+ // Single polygon with zero area
+ testDegeneratePolygonsFuncInvalid(
+ "MULTIPOLYGON (((5 10, 25 30, 15 20, 5 10)))");
+ testDegeneratePolygonsFuncInvalid(
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for single Polygon with zero-area ring (should succeed).
Please add a test for a single Polygon with a zero-area ring to verify that it passes, as required by the PR.
```suggestion
// MultiPolygons with zero-area rings. These need to fail because our
// serialization format holds MultiPolygons as single vectors that rely on
// orientation for determining shell start points.
// Second polygon is zero area
testDegeneratePolygonsFuncInvalid("MULTIPOLYGON (((1 1, 2 1, 2 2, 1 1)), ((1 1, 2 2, 3 3, 2 2, 1 1)))");
// Single polygon with zero area
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((5 10, 25 30, 15 20, 5 10)))");
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((1 1, 1 2, 1 3, 1 1)))");
// Single Polygon with a zero-area ring (should succeed)
testDegeneratePolygonsFuncValid("POLYGON ((1 1, 2 2, 3 3, 1 1))");
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| if ((isShell && !isClockwise) || (!isShell && isClockwise)) { | ||
| if ((isShell && clockwiseResult == ClockwiseResult.CCW) || (!isShell && clockwiseResult == ClockwiseResult.CW)) { |
Contributor
There was a problem hiding this comment.
nitpick: The comment above this line incorrectly states the required orientation.
Update the comment to state that shells must be clockwise and holes must be counter-clockwise, matching the code's logic.
Comment on lines
+1433
to
+1444
| // MultiPolygons with zero-area rings. These need to fail because our | ||
| // serialization format holds MultiPolygons as single vectors that rely on | ||
| // orientation for determining shell start points. | ||
|
|
||
| // Second polygon is zero area | ||
| testDegeneratePolygonsFuncInvalid("MULTIPOLYGON (((1 1, 2 1, 2 2, 1 1)), ((1 1, 2 2, 3 3, 2 2, 1 1)))"); | ||
|
|
||
| // Single polygon with zero area | ||
| testDegeneratePolygonsFuncInvalid( | ||
| "MULTIPOLYGON (((5 10, 25 30, 15 20, 5 10)))"); | ||
| testDegeneratePolygonsFuncInvalid( | ||
| "MULTIPOLYGON (((1 1, 1 2, 1 3, 1 1)))"); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Missing test for single Polygon with zero-area ring (should succeed).
Please add a test for a single Polygon with a zero-area ring to verify that it passes, as required by the PR.
Suggested change
| // MultiPolygons with zero-area rings. These need to fail because our | |
| // serialization format holds MultiPolygons as single vectors that rely on | |
| // orientation for determining shell start points. | |
| // Second polygon is zero area | |
| testDegeneratePolygonsFuncInvalid("MULTIPOLYGON (((1 1, 2 1, 2 2, 1 1)), ((1 1, 2 2, 3 3, 2 2, 1 1)))"); | |
| // Single polygon with zero area | |
| testDegeneratePolygonsFuncInvalid( | |
| "MULTIPOLYGON (((5 10, 25 30, 15 20, 5 10)))"); | |
| testDegeneratePolygonsFuncInvalid( | |
| "MULTIPOLYGON (((1 1, 1 2, 1 3, 1 1)))"); | |
| // MultiPolygons with zero-area rings. These need to fail because our | |
| // serialization format holds MultiPolygons as single vectors that rely on | |
| // orientation for determining shell start points. | |
| // Second polygon is zero area | |
| testDegeneratePolygonsFuncInvalid("MULTIPOLYGON (((1 1, 2 1, 2 2, 1 1)), ((1 1, 2 2, 3 3, 2 2, 1 1)))"); | |
| // Single polygon with zero area | |
| testDegeneratePolygonsFuncInvalid( | |
| "MULTIPOLYGON (((5 10, 25 30, 15 20, 5 10)))"); | |
| testDegeneratePolygonsFuncInvalid( | |
| "MULTIPOLYGON (((1 1, 1 2, 1 3, 1 1)))"); | |
| // Single Polygon with a zero-area ring (should succeed) | |
| testDegeneratePolygonsFuncValid("POLYGON ((1 1, 2 2, 3 3, 1 1))"); |
Sullivan-Patrick
force-pushed
the
improve-polygon-serde
branch
from
34b772b to
a7697b4
Compare
Sullivan-Patrick
force-pushed
the
improve-polygon-serde
branch
from
a7697b4 to
2711c38
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Changes jts polygon serde to:
-Always allow serialization/deserialization of single
Polygons, accepting zero-area rings.-Always fail on encountering zero-area rings in MultiPolygon serde.
See facebookincubator/velox#15558 for parallel change in C++ for context and motive.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: