Skip to content

Commit a7697b4

Browse files
Improve polygon serde logic
1 parent 02d78e8 commit a7697b4

File tree

2 files changed

+171
-15
lines changed

2 files changed

+171
-15
lines changed

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

Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -179,24 +179,37 @@ 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+
//todo: how to distinguish this as internal error?
189+
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Unexpected zero-area ring in MultiPolygon deserialization.");
190+
}
191+
192+
if ((shell != null) && (isClockwiseFlag == ClockwiseResult.CW)) {
193+
// Next polygon has started
185194
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
186195
holes.clear();
196+
shell = null;
187197
}
198+
}
199+
200+
if (shell == null) {
188201
shell = GEOMETRY_FACTORY.createLinearRing(coordinates);
189202
}
190203
else {
191204
holes.add(GEOMETRY_FACTORY.createLinearRing(coordinates));
192205
}
193206
}
194-
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
195207
}
196208
catch (IllegalArgumentException e) {
197209
throw new TopologyException("Error constructing Polygon: " + e.getMessage());
198210
}
199211

212+
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
200213
if (multitype) {
201214
return GEOMETRY_FACTORY.createMultiPolygon(polygons.toArray(new Polygon[0]));
202215
}
@@ -411,7 +424,10 @@ private static void writePolygon(Geometry geometry, SliceOutput output, boolean
411424
}
412425

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

@@ -462,32 +478,44 @@ private static void writeEnvelope(Geometry geometry, SliceOutput output)
462478
output.writeDouble(envelope.getMaxY());
463479
}
464480

465-
private static void canonicalizePolygonCoordinates(Coordinate[] coordinates, int[] partIndexes, boolean[] shellPart)
481+
private static boolean canonicalizePolygonCoordinates(Coordinate[] coordinates, int[] partIndexes, boolean[] shellPart)
466482
{
483+
boolean zeroAreaRingEncountered = false;
467484
for (int part = 0; part < partIndexes.length - 1; part++) {
468-
canonicalizePolygonCoordinates(coordinates, partIndexes[part], partIndexes[part + 1], shellPart[part]);
485+
zeroAreaRingEncountered |= canonicalizePolygonCoordinates(coordinates, partIndexes[part], partIndexes[part + 1], shellPart[part]);
469486
}
470487
if (partIndexes.length > 0) {
471-
canonicalizePolygonCoordinates(coordinates, partIndexes[partIndexes.length - 1], coordinates.length, shellPart[partIndexes.length - 1]);
488+
zeroAreaRingEncountered |= canonicalizePolygonCoordinates(coordinates, partIndexes[partIndexes.length - 1], coordinates.length, shellPart[partIndexes.length - 1]);
472489
}
490+
return zeroAreaRingEncountered;
473491
}
474492

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

479-
if ((isShell && !isClockwise) || (!isShell && isClockwise)) {
506+
if ((isShell && clockwiseResult == ClockwiseResult.CCW) || (!isShell && clockwiseResult == ClockwiseResult.CW)) {
480507
// shell has to be counter clockwise
481508
reverse(coordinates, start, end);
482509
}
510+
return false;
483511
}
484512

485-
private static boolean isClockwise(Coordinate[] coordinates)
513+
private static ClockwiseResult isClockwise(Coordinate[] coordinates)
486514
{
487515
return isClockwise(coordinates, 0, coordinates.length);
488516
}
489517

490-
private static boolean isClockwise(Coordinate[] coordinates, int start, int end)
518+
private static ClockwiseResult isClockwise(Coordinate[] coordinates, int start, int end)
491519
{
492520
// Sum over the edges: (x2 − x1) * (y2 + y1).
493521
// If the result is positive the curve is clockwise,
@@ -497,7 +525,10 @@ private static boolean isClockwise(Coordinate[] coordinates, int start, int end)
497525
area += (coordinates[i].x - coordinates[i - 1].x) * (coordinates[i].y + coordinates[i - 1].y);
498526
}
499527
area += (coordinates[start].x - coordinates[end - 1].x) * (coordinates[start].y + coordinates[end - 1].y);
500-
return area > 0;
528+
if (area == 0.0) {
529+
return ClockwiseResult.ZERO_AREA;
530+
}
531+
return area > 0.0 ? ClockwiseResult.CW : ClockwiseResult.CCW;
501532
}
502533

503534
private static void reverse(Coordinate[] coordinates, int start, int end)
@@ -510,6 +541,13 @@ private static void reverse(Coordinate[] coordinates, int start, int end)
510541
}
511542
}
512543

544+
enum ClockwiseResult
545+
{
546+
CW,
547+
CCW,
548+
ZERO_AREA
549+
}
550+
513551
/**
514552
* Shape type codes from ERSI's specification
515553
* 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)