Parquet: make counterclockwise external rings, clockwise internal rings

Implements opengeospatial/geoparquet#59
rouault · Apr 19, 2022 · ed975f1 · ed975f1
1 parent 0a4389d
commit ed975f1
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 6 deletions.
diff --git a/autotest/ogr/ogr_parquet.py b/autotest/ogr/ogr_parquet.py
@@ -630,18 +630,21 @@ def test_ogr_parquet_coordinate_epoch():
 @pytest.mark.parametrize("written_geom_type,written_wkt,expected_geom_type,expected_wkts", [
     (ogr.wkbPoint, ["POINT (1 2)"], ogr.wkbPoint, None),
     (ogr.wkbLineString, ["LINESTRING (1 2,3 4)"], ogr.wkbLineString, None),
-    (ogr.wkbPolygon, ["POLYGON ((0 0,0 1,1 1,1 0,0 0))"], ogr.wkbPolygon, None),
+    (ogr.wkbPolygon, ["POLYGON ((0 0,1 0,1 1,0 1,0 0))",
+                      "POLYGON ((0 0,0 1,1 1,1 0,0 0),(0.2 0.2,0.8 0.8,0.2 0.8,0.2 0.2))"],  # winding order must be fixed
+     ogr.wkbPolygon, ["POLYGON ((0 0,1 0,1 1,0 1,0 0))",
+                      "POLYGON ((0 0,1 0,1 1,0 1,0 0),(0.2 0.2,0.2 0.8,0.8 0.8,0.2 0.2))"]),
     (ogr.wkbMultiPoint, ["MULTIPOINT ((1 2))"], ogr.wkbMultiPoint, None),
     (ogr.wkbMultiLineString, ["MULTILINESTRING ((1 2,3 4))"], ogr.wkbMultiLineString, None),
-    (ogr.wkbMultiPolygon, ["MULTIPOLYGON (((0 0,0 1,1 1,1 0,0 0)))"], ogr.wkbMultiPolygon, None),
+    (ogr.wkbMultiPolygon, ["MULTIPOLYGON (((0 0,1 0,1 1,0 1,0 0)))"], ogr.wkbMultiPolygon, None),
     (ogr.wkbGeometryCollection, ["GEOMETRYCOLLECTION (POINT (1 2))"], ogr.wkbGeometryCollection, None),
     (ogr.wkbPoint25D, ["POINT Z (1 2 3)"], ogr.wkbPoint25D, None),
     (ogr.wkbUnknown, ["POINT (1 2)"], ogr.wkbPoint, None),
     (ogr.wkbUnknown, ["POINT Z (1 2 3)"], ogr.wkbPoint25D, None),
     (ogr.wkbUnknown, ["POLYGON ((0 0,0 1,1 1,1 0,0 0))",
-                      "MULTIPOLYGON (((0 0,0 1,1 1,1 0,0 0)))"],
-     ogr.wkbMultiPolygon, ["MULTIPOLYGON (((0 0,0 1,1 1,1 0,0 0)))",
-                           "MULTIPOLYGON (((0 0,0 1,1 1,1 0,0 0)))"]),
+                      "MULTIPOLYGON (((0 0,1 0,1 1,0 1,0 0)))"],
+     ogr.wkbMultiPolygon, ["MULTIPOLYGON (((0 0,1 0,1 1,0 1,0 0)))",
+                           "MULTIPOLYGON (((0 0,1 0,1 1,0 1,0 0)))"]),
     (ogr.wkbUnknown, ["LINESTRING (1 2,3 4)",
                       "MULTILINESTRING ((10 2,3 4))"],
      ogr.wkbMultiLineString, ["MULTILINESTRING ((1 2,3 4))",

diff --git a/ogr/ogrsf_frmts/arrow_common/ogr_arrow.h b/ogr/ogrsf_frmts/arrow_common/ogr_arrow.h
@@ -228,6 +228,8 @@ class OGRArrowWriterLayer CPL_NON_FINAL: public OGRLayer
         bool                    WriteArrays(std::function<bool(const std::shared_ptr<arrow::Field>&,
                                                                const std::shared_ptr<arrow::Array>&)> postProcessArray);
 
+        virtual void            FixupGeometryBeforeWriting(OGRGeometry* /* poGeom */ ) {}
+
 public:
         OGRArrowWriterLayer( arrow::MemoryPool* poMemoryPool,
                                const std::shared_ptr<arrow::io::OutputStream>& poOutputStream,

diff --git a/ogr/ogrsf_frmts/arrow_common/ograrrowwriterlayer.hpp b/ogr/ogrsf_frmts/arrow_common/ograrrowwriterlayer.hpp
@@ -1121,7 +1121,7 @@ OGRErr OGRArrowWriterLayer::ICreateFeature( OGRFeature* poFeature )
     for( int i = 0; i < nGeomFieldCount; ++i, ++nArrowIdx )
     {
         auto poBuilder = m_apoBuilders[nArrowIdx].get();
-        const OGRGeometry* poGeom = poFeature->GetGeomFieldRef(i);
+        OGRGeometry* poGeom = poFeature->GetGeomFieldRef(i);
         const auto eGType = poGeom ? poGeom->getGeometryType() : wkbNone;
         const auto eColumnGType = m_poFeatureDefn->GetGeomFieldDefn(i)->GetType();
         const bool bIsEmpty = poGeom != nullptr && poGeom->IsEmpty();
@@ -1170,6 +1170,7 @@ OGRErr OGRArrowWriterLayer::ICreateFeature( OGRFeature* poFeature )
                 poGeomModified->setMeasured(false);
                 poGeom = poGeomModified.get();
             }
+            FixupGeometryBeforeWriting(poGeom);
             const auto nSize = poGeom->WkbSize();
             if( nSize < INT_MAX )
             {

diff --git a/ogr/ogrsf_frmts/parquet/ogr_parquet.h b/ogr/ogrsf_frmts/parquet/ogr_parquet.h
@@ -134,6 +134,8 @@ class OGRParquetWriterLayer final: public OGRArrowWriterLayer
 
         virtual bool            IsSupportedGeometryType(OGRwkbGeometryType eGType) const override;
 
+        virtual void            FixupGeometryBeforeWriting(OGRGeometry* poGeom) override;
+
 public:
         OGRParquetWriterLayer( arrow::MemoryPool* poMemoryPool,
                                const std::shared_ptr<arrow::io::OutputStream>& poOutputStream,

diff --git a/ogr/ogrsf_frmts/parquet/ogrparquetwriterlayer.cpp b/ogr/ogrsf_frmts/parquet/ogrparquetwriterlayer.cpp
@@ -456,3 +456,35 @@ bool OGRParquetWriterLayer::FlushGroup()
     m_apoBuilders.clear();
     return ret;
 }
+
+/************************************************************************/
+/*                     FixupGeometryBeforeWriting()                     */
+/************************************************************************/
+
+void OGRParquetWriterLayer::FixupGeometryBeforeWriting(OGRGeometry* poGeom)
+{
+    const auto eFlattenType = wkbFlatten(poGeom->getGeometryType());
+    // Polygon rings MUST follow the right-hand rule for orientation
+    // (counterclockwise external rings, clockwise internal rings)
+    if( eFlattenType == wkbPolygon )
+    {
+        bool bFirstRing = true;
+        for( auto poRing: poGeom->toPolygon() )
+        {
+            if( (bFirstRing && poRing->isClockwise()) ||
+                (!bFirstRing && !poRing->isClockwise()) )
+            {
+                poRing->reverseWindingOrder();
+            }
+            bFirstRing = false;
+        }
+    }
+    else if( eFlattenType == wkbMultiPolygon ||
+             eFlattenType == wkbGeometryCollection )
+    {
+        for( auto poSubGeom: poGeom->toGeometryCollection() )
+        {
+            FixupGeometryBeforeWriting(poSubGeom);
+        }
+    }
+}