Skip to content

fix: Improve jts polygon serde logic #26672

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Improve jts polygon serde logic #26672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 51 additions & 14 deletions ...ospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,24 +179,36 @@ private static Geometry readPolygon(SliceInput input, boolean multitype)
try {
for (int i = 0; i < partCount; i++) {
Coordinate[] coordinates = readCoordinates(input, partLengths[i]);
if (isClockwise(coordinates)) {
// next polygon has started
if (shell != null) {
ClockwiseResult isClockwiseFlag = isClockwise(coordinates);
if (multitype) {
if (isClockwiseFlag == ClockwiseResult.ZERO_AREA) {
// When serializing a MultiPolygon, we should throw a user error if
// there is a zero-area ring. This should only get hit due to a bug in
// our serde logic.
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Unexpected zero-area ring in MultiPolygon deserialization.");
}

if ((shell != null) && (isClockwiseFlag == ClockwiseResult.CW)) {
// Next polygon has started
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
holes.clear();
shell = null;
}
}

if (shell == null) {
shell = GEOMETRY_FACTORY.createLinearRing(coordinates);
}
else {
holes.add(GEOMETRY_FACTORY.createLinearRing(coordinates));
}
}
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
}
catch (IllegalArgumentException e) {
throw new TopologyException("Error constructing Polygon: " + e.getMessage());
}

polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
if (multitype) {
return GEOMETRY_FACTORY.createMultiPolygon(polygons.toArray(new Polygon[0]));
}
Expand Down Expand Up @@ -411,7 +423,10 @@ private static void writePolygon(Geometry geometry, SliceOutput output, boolean
}

Coordinate[] coordinates = geometry.getCoordinates();
canonicalizePolygonCoordinates(coordinates, partIndexes, shellPart);
boolean zeroAreaRingEncountered = canonicalizePolygonCoordinates(coordinates, partIndexes, shellPart);
if (zeroAreaRingEncountered && multitype) {
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Input MultiPolygon contains one or more zero-area rings.");
}
writeCoordinates(coordinates, output);
}

Expand Down Expand Up @@ -462,32 +477,44 @@ private static void writeEnvelope(Geometry geometry, SliceOutput output)
output.writeDouble(envelope.getMaxY());
}

private static void canonicalizePolygonCoordinates(Coordinate[] coordinates, int[] partIndexes, boolean[] shellPart)
private static boolean canonicalizePolygonCoordinates(Coordinate[] coordinates, int[] partIndexes, boolean[] shellPart)
{
boolean zeroAreaRingEncountered = false;
for (int part = 0; part < partIndexes.length - 1; part++) {
canonicalizePolygonCoordinates(coordinates, partIndexes[part], partIndexes[part + 1], shellPart[part]);
zeroAreaRingEncountered |= canonicalizePolygonCoordinates(coordinates, partIndexes[part], partIndexes[part + 1], shellPart[part]);
}
if (partIndexes.length > 0) {
canonicalizePolygonCoordinates(coordinates, partIndexes[partIndexes.length - 1], coordinates.length, shellPart[partIndexes.length - 1]);
zeroAreaRingEncountered |= canonicalizePolygonCoordinates(coordinates, partIndexes[partIndexes.length - 1], coordinates.length, shellPart[partIndexes.length - 1]);
}
return zeroAreaRingEncountered;
}

private static void canonicalizePolygonCoordinates(Coordinate[] coordinates, int start, int end, boolean isShell)
/**
* Ensures that a polygon ring has the canonical orientation:
* Exterior rings (shells) must be clockwise.
* Interior rings (holes) must be counter-clockwise.
* A return value of true indicates a zero-area ring was encountered
*/
private static boolean canonicalizePolygonCoordinates(Coordinate[] coordinates, int start, int end, boolean isShell)
{
boolean isClockwise = isClockwise(coordinates, start, end);
ClockwiseResult clockwiseResult = isClockwise(coordinates, start, end);
if (clockwiseResult == ClockwiseResult.ZERO_AREA) {
return true;
}

if ((isShell && !isClockwise) || (!isShell && isClockwise)) {
if ((isShell && clockwiseResult == ClockwiseResult.CCW) || (!isShell && clockwiseResult == ClockwiseResult.CW)) {
Copy link
Contributor

@sourcery-ai sourcery-ai bot Nov 20, 2025

Choose a reason for hiding this comment

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

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.

// shell has to be counter clockwise
reverse(coordinates, start, end);
}
return false;
}

private static boolean isClockwise(Coordinate[] coordinates)
private static ClockwiseResult isClockwise(Coordinate[] coordinates)
{
return isClockwise(coordinates, 0, coordinates.length);
}

private static boolean isClockwise(Coordinate[] coordinates, int start, int end)
private static ClockwiseResult isClockwise(Coordinate[] coordinates, int start, int end)
{
// Sum over the edges: (x2 − x1) * (y2 + y1).
// If the result is positive the curve is clockwise,
Expand All @@ -497,7 +524,10 @@ private static boolean isClockwise(Coordinate[] coordinates, int start, int end)
area += (coordinates[i].x - coordinates[i - 1].x) * (coordinates[i].y + coordinates[i - 1].y);
}
area += (coordinates[start].x - coordinates[end - 1].x) * (coordinates[start].y + coordinates[end - 1].y);
return area > 0;
if (area == 0.0) {
return ClockwiseResult.ZERO_AREA;
}
return area > 0.0 ? ClockwiseResult.CW : ClockwiseResult.CCW;
}

private static void reverse(Coordinate[] coordinates, int start, int end)
Expand All @@ -510,6 +540,13 @@ private static void reverse(Coordinate[] coordinates, int start, int end)
}
}

enum ClockwiseResult
{
CW,
CCW,
ZERO_AREA
}

/**
* Shape type codes from ERSI's specification
* https://www.esri.com/library/whitepapers/pdfs/shapefile.pdf
Expand Down
120 changes: 119 additions & 1 deletion presto-main-base/src/test/java/com/facebook/presto/geospatial/TestGeoFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ public void testGeometryInvalidReason()
// invalid geometries
assertInvalidReason("MULTIPOINT ((0 0), (0 1), (1 1), (0 1))", "[MultiPoint] Repeated point: (0.0 1.0)");
assertInvalidReason("LINESTRING (0 0, -1 0.5, 0 1, 1 1, 1 0, 0 1, 0 0)", "[LineString] Self-intersection at or near: (0.0 1.0)");
assertInvalidReason("POLYGON ((0 0, 1 1, 0 1, 1 0, 0 0))", "Error constructing Polygon: shell is empty but holes are not");
assertInvalidReason("POLYGON ((0 0, 1 1, 0 1, 1 0, 0 0))", "Self-intersection");
assertInvalidReason("POLYGON ((0 0, 0 1, 0 1, 1 1, 1 0, 0 0), (2 2, 2 3, 3 3, 3 2, 2 2))", "Hole lies outside shell");
assertInvalidReason("POLYGON ((0 0, 0 1, 1 1, 1 0, 0 0), (2 2, 2 3, 3 3, 3 2, 2 2))", "Hole lies outside shell");
assertInvalidReason("POLYGON ((0 0, 0 1, 2 1, 1 1, 1 0, 0 0))", "Ring Self-intersection");
Expand Down Expand Up @@ -1372,6 +1372,124 @@ private void assertInvalidGeometryJson(String json)
assertInvalidFunction("geometry_from_geojson('" + json + "')", "Invalid GeoJSON:.*");
}

@Test
public void testDegeneratePolygons()
{
// Single polygon with CCW orientation - should orient to CW
testDegeneratePolygonsFunc(
"POLYGON ((1 2, 3 4, 5 7, 1 2))",
"POLYGON ((1 2, 5 7, 3 4, 1 2))");

// Single polygons with no area- should not reverse these
testDegeneratePolygonsFunc(
"POLYGON ((1 2, 5 6, 3 4, 1 2))", "POLYGON ((1 2, 5 6, 3 4, 1 2))");
testDegeneratePolygonsFunc(
"POLYGON ((1 2, 3 4, 5 6, 1 2))", "POLYGON ((1 2, 3 4, 5 6, 1 2))");

// Single polygons with interior rings- should canonicalize so any shells have
// CW orientation and holes have CCW orientation.
testDegeneratePolygonsFunc(
"POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 3 7, 7 7, 7 3, 3 3))",
"POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3))");
testDegeneratePolygonsFunc(
"POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3))",
"POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3))");

// Multipolygons where polygons after the first are CCW for shell or CW for
// hole. These should be correctly oriented after serde.

// First polygon has CW shell and CCW hole, second polygon has CCW
// shell and CCW hole -> second polygon shell should be reoriented
testDegeneratePolygonsFunc(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 20 0, 20 20, 0 20, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))",
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");

// First polygon has CW shell and CW hole, second polygon has CCW
// shell and CCW hole -> first polygon hole and second polygon shell should be
// reoriented
testDegeneratePolygonsFunc(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 3 7, 7 7, 7 3, 3 3)), ((0 0, 20 0, 20 20, 0 20, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))",
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");

// First polygon has CCW shell and CW hole, second polygon has CCW
// shell and CW hole -> both polygons should have shell and hole reoriented
testDegeneratePolygonsFunc(
"MULTIPOLYGON (((0 0, 10 0, 10 10, 0 10, 0 0), (3 3, 3 7, 7 7, 7 3, 3 3)), ((0 0, 20 0, 20 20, 0 20, 0 0), (6 6, 6 14, 14 14, 14 6, 6 6)))",
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");

// First polygon has CCW shell and CCW hole, second polygon has CCW
// shell and CCW hole -> both polygons should have shells reoriented
testDegeneratePolygonsFunc(
"MULTIPOLYGON (((0 0, 10 0, 10 10, 0 10, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 20 0, 20 20, 0 20, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))",
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");

// First polygon has CW shell and CW hole, second polygon has CW
// shell and CW hole -> both polygons should have holes reoriented
testDegeneratePolygonsFunc(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 3 7, 7 7, 7 3, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 6 14, 14 14, 14 6, 6 6)))",
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");


// 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)))");
Comment on lines +1433 to +1444
Copy link
Contributor

@sourcery-ai sourcery-ai bot Nov 20, 2025

Choose a reason for hiding this comment

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

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))");


// First polygon has CW shell and CCW hole, second polygon has CW shell and
// zero-area hole
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 9 9, 14 14, 6 6)))");

// First polygon has CW shell and CCW hole, second polygon has zero-area shell
// and CCW hole
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 0 10, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");

// First polygon has CW shell and CCW hole, second polygon has CW shell
// and zero-area hole
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 9 9, 14 14, 9 9, 6 6)))");


// First polygon has CW shell and CCW hole, second polygon has zero-area shell
// and zero-area hole
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 0 10, 0 0), (6 6, 9 9, 14 14, 6 6)))");

// First polygon has zero-area shell and CCW hole, second polygon has CW shell
// and CCW hole
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((0 0, 0 10, 0 15, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6))))");

// First polygon has CW shell and zero-area hole, second polygon has CW shell
// and CCW hole
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 9 3, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");

// First polygon has zero-area shell and zero-area hole, second polygon has CW
// shell and CCW hole
testDegeneratePolygonsFuncInvalid(
"MULTIPOLYGON (((0 0, 0 10, 0 5, 0 10, 0 0), (3 3, 7 3, 9 3, 3 3)), ((0 0, 0 20, 20 20, 20 0, 0 0), (6 6, 14 6, 14 14, 6 14, 6 6)))");
}

private void testDegeneratePolygonsFunc(String wkt, String expected)
{
assertFunction(format("ST_ASText(ST_GeometryFromText('%s'))", wkt), VARCHAR, expected);
}

private void testDegeneratePolygonsFuncInvalid(String wkt)
{
assertInvalidFunction(format("ST_ASText(ST_GeometryFromText('%s'))", wkt), "Input MultiPolygon contains one or more zero-area rings.");
}

@Test
public void testGooglePolylineDecode()
{
Expand Down
Loading