Skip to content

Commit 2711c38

Browse files
Improve polygon serde logic
1 parent 02d78e8 commit 2711c38

File tree

2 files changed

+170
-15
lines changed

2 files changed

+170
-15
lines changed

presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -179,24 +179,36 @@ private static Geometry readPolygon(SliceInput input, boolean multitype)
179179
try {
180180
for (int i = 0; i < partCount; i++) {
181181
Coordinate[] coordinates = readCoordinates(input, partLengths[i]);
182-
if (isClockwise(coordinates)) {
183-
// next polygon has started
184-
if (shell != null) {
182+
ClockwiseResult isClockwiseFlag = isClockwise(coordinates);
183+
if (multitype) {
184+
if (isClockwiseFlag == ClockwiseResult.ZERO_AREA) {
185+
// When serializing a MultiPolygon, we should throw a user error if
186+
// there is a zero-area ring. This should only get hit due to a bug in
187+
// our serde logic.
188+
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Unexpected zero-area ring in MultiPolygon deserialization.");
189+
}
190+
191+
if ((shell != null) && (isClockwiseFlag == ClockwiseResult.CW)) {
192+
// Next polygon has started
185193
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
186194
holes.clear();
195+
shell = null;
187196
}
197+
}
198+
199+
if (shell == null) {
188200
shell = GEOMETRY_FACTORY.createLinearRing(coordinates);
189201
}
190202
else {
191203
holes.add(GEOMETRY_FACTORY.createLinearRing(coordinates));
192204
}
193205
}
194-
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
195206
}
196207
catch (IllegalArgumentException e) {
197208
throw new TopologyException("Error constructing Polygon: " + e.getMessage());
198209
}
199210

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

413425
Coordinate[] coordinates = geometry.getCoordinates();
414-
canonicalizePolygonCoordinates(coordinates, partIndexes, shellPart);
426+
boolean zeroAreaRingEncountered = canonicalizePolygonCoordinates(coordinates, partIndexes, shellPart);
427+
if (zeroAreaRingEncountered && multitype) {
428+
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Input MultiPolygon contains one or more zero-area rings.");
429+
}
415430
writeCoordinates(coordinates, output);
416431
}
417432

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

465-
private static void canonicalizePolygonCoordinates(Coordinate[] coordinates, int[] partIndexes, boolean[] shellPart)
480+
private static boolean canonicalizePolygonCoordinates(Coordinate[] coordinates, int[] partIndexes, boolean[] shellPart)
466481
{
482+
boolean zeroAreaRingEncountered = false;
467483
for (int part = 0; part < partIndexes.length - 1; part++) {
468-
canonicalizePolygonCoordinates(coordinates, partIndexes[part], partIndexes[part + 1], shellPart[part]);
484+
zeroAreaRingEncountered |= canonicalizePolygonCoordinates(coordinates, partIndexes[part], partIndexes[part + 1], shellPart[part]);
469485
}
470486
if (partIndexes.length > 0) {
471-
canonicalizePolygonCoordinates(coordinates, partIndexes[partIndexes.length - 1], coordinates.length, shellPart[partIndexes.length - 1]);
487+
zeroAreaRingEncountered |= canonicalizePolygonCoordinates(coordinates, partIndexes[partIndexes.length - 1], coordinates.length, shellPart[partIndexes.length - 1]);
472488
}
489+
return zeroAreaRingEncountered;
473490
}
474491

475-
private static void canonicalizePolygonCoordinates(Coordinate[] coordinates, int start, int end, boolean isShell)
492+
/**
493+
* Ensures that a polygon ring has the canonical orientation:
494+
* Exterior rings (shells) must be clockwise.
495+
* Interior rings (holes) must be counter-clockwise.
496+
* A return value of true indicates a zero-area ring was encountered
497+
*/
498+
private static boolean canonicalizePolygonCoordinates(Coordinate[] coordinates, int start, int end, boolean isShell)
476499
{
477-
boolean isClockwise = isClockwise(coordinates, start, end);
500+
ClockwiseResult clockwiseResult = isClockwise(coordinates, start, end);
501+
if (clockwiseResult == ClockwiseResult.ZERO_AREA) {
502+
return true;
503+
}
478504

479-
if ((isShell && !isClockwise) || (!isShell && isClockwise)) {
505+
if ((isShell && clockwiseResult == ClockwiseResult.CCW) || (!isShell && clockwiseResult == ClockwiseResult.CW)) {
480506
// shell has to be counter clockwise
481507
reverse(coordinates, start, end);
482508
}
509+
return false;
483510
}
484511

485-
private static boolean isClockwise(Coordinate[] coordinates)
512+
private static ClockwiseResult isClockwise(Coordinate[] coordinates)
486513
{
487514
return isClockwise(coordinates, 0, coordinates.length);
488515
}
489516

490-
private static boolean isClockwise(Coordinate[] coordinates, int start, int end)
517+
private static ClockwiseResult isClockwise(Coordinate[] coordinates, int start, int end)
491518
{
492519
// Sum over the edges: (x2 − x1) * (y2 + y1).
493520
// If the result is positive the curve is clockwise,
@@ -497,7 +524,10 @@ private static boolean isClockwise(Coordinate[] coordinates, int start, int end)
497524
area += (coordinates[i].x - coordinates[i - 1].x) * (coordinates[i].y + coordinates[i - 1].y);
498525
}
499526
area += (coordinates[start].x - coordinates[end - 1].x) * (coordinates[start].y + coordinates[end - 1].y);
500-
return area > 0;
527+
if (area == 0.0) {
528+
return ClockwiseResult.ZERO_AREA;
529+
}
530+
return area > 0.0 ? ClockwiseResult.CW : ClockwiseResult.CCW;
501531
}
502532

503533
private static void reverse(Coordinate[] coordinates, int start, int end)
@@ -510,6 +540,13 @@ private static void reverse(Coordinate[] coordinates, int start, int end)
510540
}
511541
}
512542

543+
enum ClockwiseResult
544+
{
545+
CW,
546+
CCW,
547+
ZERO_AREA
548+
}
549+
513550
/**
514551
* Shape type codes from ERSI's specification
515552
* https://www.esri.com/library/whitepapers/pdfs/shapefile.pdf

presto-main-base/src/test/java/com/facebook/presto/geospatial/TestGeoFunctions.java

Lines changed: 119 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ public void testGeometryInvalidReason()
454454
// invalid geometries
455455
assertInvalidReason("MULTIPOINT ((0 0), (0 1), (1 1), (0 1))", "[MultiPoint] Repeated point: (0.0 1.0)");
456456
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)");
457-
assertInvalidReason("POLYGON ((0 0, 1 1, 0 1, 1 0, 0 0))", "Error constructing Polygon: shell is empty but holes are not");
457+
assertInvalidReason("POLYGON ((0 0, 1 1, 0 1, 1 0, 0 0))", "Self-intersection");
458458
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");
459459
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");
460460
assertInvalidReason("POLYGON ((0 0, 0 1, 2 1, 1 1, 1 0, 0 0))", "Ring Self-intersection");
@@ -1372,6 +1372,124 @@ private void assertInvalidGeometryJson(String json)
13721372
assertInvalidFunction("geometry_from_geojson('" + json + "')", "Invalid GeoJSON:.*");
13731373
}
13741374

1375+
@Test
1376+
public void testDegeneratePolygons()
1377+
{
1378+
// Single polygon with CCW orientation - should orient to CW
1379+
testDegeneratePolygonsFunc(
1380+
"POLYGON ((1 2, 3 4, 5 7, 1 2))",
1381+
"POLYGON ((1 2, 5 7, 3 4, 1 2))");
1382+
1383+
// Single polygons with no area- should not reverse these
1384+
testDegeneratePolygonsFunc(
1385+
"POLYGON ((1 2, 5 6, 3 4, 1 2))", "POLYGON ((1 2, 5 6, 3 4, 1 2))");
1386+
testDegeneratePolygonsFunc(
1387+
"POLYGON ((1 2, 3 4, 5 6, 1 2))", "POLYGON ((1 2, 3 4, 5 6, 1 2))");
1388+
1389+
// Single polygons with interior rings- should canonicalize so any shells have
1390+
// CW orientation and holes have CCW orientation.
1391+
testDegeneratePolygonsFunc(
1392+
"POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 3 7, 7 7, 7 3, 3 3))",
1393+
"POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3))");
1394+
testDegeneratePolygonsFunc(
1395+
"POLYGON ((0 0, 10 0, 10 10, 0 10, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3))",
1396+
"POLYGON ((0 0, 0 10, 10 10, 10 0, 0 0), (3 3, 7 3, 7 7, 3 7, 3 3))");
1397+
1398+
// Multipolygons where polygons after the first are CCW for shell or CW for
1399+
// hole. These should be correctly oriented after serde.
1400+
1401+
// First polygon has CW shell and CCW hole, second polygon has CCW
1402+
// shell and CCW hole -> second polygon shell should be reoriented
1403+
testDegeneratePolygonsFunc(
1404+
"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)))",
1405+
"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)))");
1406+
1407+
// First polygon has CW shell and CW hole, second polygon has CCW
1408+
// shell and CCW hole -> first polygon hole and second polygon shell should be
1409+
// reoriented
1410+
testDegeneratePolygonsFunc(
1411+
"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)))",
1412+
"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)))");
1413+
1414+
// First polygon has CCW shell and CW hole, second polygon has CCW
1415+
// shell and CW hole -> both polygons should have shell and hole reoriented
1416+
testDegeneratePolygonsFunc(
1417+
"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)))",
1418+
"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)))");
1419+
1420+
// First polygon has CCW shell and CCW hole, second polygon has CCW
1421+
// shell and CCW hole -> both polygons should have shells reoriented
1422+
testDegeneratePolygonsFunc(
1423+
"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)))",
1424+
"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)))");
1425+
1426+
// First polygon has CW shell and CW hole, second polygon has CW
1427+
// shell and CW hole -> both polygons should have holes reoriented
1428+
testDegeneratePolygonsFunc(
1429+
"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)))",
1430+
"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)))");
1431+
1432+
1433+
// MultiPolygons with zero-area rings. These need to fail because our
1434+
// serialization format holds MultiPolygons as single vectors that rely on
1435+
// orientation for determining shell start points.
1436+
1437+
// Second polygon is zero area
1438+
testDegeneratePolygonsFuncInvalid("MULTIPOLYGON (((1 1, 2 1, 2 2, 1 1)), ((1 1, 2 2, 3 3, 2 2, 1 1)))");
1439+
1440+
// Single polygon with zero area
1441+
testDegeneratePolygonsFuncInvalid(
1442+
"MULTIPOLYGON (((5 10, 25 30, 15 20, 5 10)))");
1443+
testDegeneratePolygonsFuncInvalid(
1444+
"MULTIPOLYGON (((1 1, 1 2, 1 3, 1 1)))");
1445+
1446+
// First polygon has CW shell and CCW hole, second polygon has CW shell and
1447+
// zero-area hole
1448+
testDegeneratePolygonsFuncInvalid(
1449+
"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)))");
1450+
1451+
// First polygon has CW shell and CCW hole, second polygon has zero-area shell
1452+
// and CCW hole
1453+
testDegeneratePolygonsFuncInvalid(
1454+
"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)))");
1455+
1456+
// First polygon has CW shell and CCW hole, second polygon has CW shell
1457+
// and zero-area hole
1458+
testDegeneratePolygonsFuncInvalid(
1459+
"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)))");
1460+
1461+
1462+
// First polygon has CW shell and CCW hole, second polygon has zero-area shell
1463+
// and zero-area hole
1464+
testDegeneratePolygonsFuncInvalid(
1465+
"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)))");
1466+
1467+
// First polygon has zero-area shell and CCW hole, second polygon has CW shell
1468+
// and CCW hole
1469+
testDegeneratePolygonsFuncInvalid(
1470+
"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))))");
1471+
1472+
// First polygon has CW shell and zero-area hole, second polygon has CW shell
1473+
// and CCW hole
1474+
testDegeneratePolygonsFuncInvalid(
1475+
"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)))");
1476+
1477+
// First polygon has zero-area shell and zero-area hole, second polygon has CW
1478+
// shell and CCW hole
1479+
testDegeneratePolygonsFuncInvalid(
1480+
"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)))");
1481+
}
1482+
1483+
private void testDegeneratePolygonsFunc(String wkt, String expected)
1484+
{
1485+
assertFunction(format("ST_ASText(ST_GeometryFromText('%s'))", wkt), VARCHAR, expected);
1486+
}
1487+
1488+
private void testDegeneratePolygonsFuncInvalid(String wkt)
1489+
{
1490+
assertInvalidFunction(format("ST_ASText(ST_GeometryFromText('%s'))", wkt), "Input MultiPolygon contains one or more zero-area rings.");
1491+
}
1492+
13751493
@Test
13761494
public void testGooglePolylineDecode()
13771495
{

0 commit comments

Comments
 (0)