Merge pull request #8855 from rouault/fix_8849

ogr2ogr: fix GPKG -> Shapefile when field names are truncated (fix #8849, 3.8.0 regression)

autotest/utilities/test_ogr2ogr_lib.py

@@ -2406,3 +2406,46 @@ def my_handler(errorClass, errno, msg):
     f = out_lyr.GetNextFeature()
     assert f.GetFID() == 1
     assert f["foo"] == "baz"
+
+
+###############################################################################
+
+
+@pytest.mark.require_driver("GPKG")
+def test_ogr2ogr_lib_gpkg_to_shp_truncated_field_names(tmp_vsimem):
+
+    src_filename = str(
+        tmp_vsimem / "test_ogr2ogr_lib_gpkg_to_shp_truncated_field_names.gpkg"
+    )
+    src_ds = gdal.GetDriverByName("GPKG").Create(
+        src_filename, 0, 0, 0, gdal.GDT_Unknown
+    )
+    src_lyr = src_ds.CreateLayer("test")
+    src_lyr.CreateField(ogr.FieldDefn("shortname"))
+    src_lyr.CreateField(ogr.FieldDefn("too_long_for_shapefile"))
+    f = ogr.Feature(src_lyr.GetLayerDefn())
+    f.SetField("shortname", "foo")
+    f.SetField("too_long_for_shapefile", "bar")
+    src_lyr.CreateFeature(f)
+    src_ds.Close()
+
+    out_filename = str(
+        tmp_vsimem / "test_ogr2ogr_lib_gpkg_to_shp_truncated_field_names.shp"
+    )
+
+    got_msg = []
+
+    def my_handler(errorClass, errno, msg):
+        if errorClass != gdal.CE_Debug:
+            got_msg.append(msg)
+        return
+
+    with gdaltest.error_handler(my_handler):
+        out_ds = gdal.VectorTranslate(out_filename, src_filename)
+    assert got_msg == [
+        "Normalized/laundered field name: 'too_long_for_shapefile' to 'too_long_f'"
+    ]
+    out_lyr = out_ds.GetLayer(0)
+    f = out_lyr.GetNextFeature()
+    assert f["shortname"] == "foo"
+    assert f["too_long_f"] == "bar"

ogr/ogrsf_frmts/generic/ogrlayer.cpp

@@ -34,17 +34,13 @@
 #include "ogr_swq.h"
 #include "ograpispy.h"
 #include "ogr_wkb.h"
+#include "ogrlayer_private.h"
 
 #include "cpl_time.h"
 #include <cassert>
 #include <limits>
 #include <set>
 
-struct OGRLayer::Private
-{
-    bool m_bInFeatureIterator = false;
-};
-
 /************************************************************************/
 /*                              OGRLayer()                              */
 /************************************************************************/

ogr/ogrsf_frmts/generic/ogrlayer_private.h

@@ -0,0 +1,50 @@
+/******************************************************************************
+ *
+ * Project:  OpenGIS Simple Features Reference Implementation
+ * Purpose:  OGRLayer::Private struct
+ * Author:   Even Rouault <even dot rouault at spatialys.com>
+ *
+ ******************************************************************************
+ * Copyright (c) 2024, Even Rouault <even dot rouault at spatialys.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ ****************************************************************************/
+
+#ifndef OGRLAYER_PRIVATE_H_INCLUDED
+#define OGRLAYER_PRIVATE_H_INCLUDED
+
+#include "ogrsf_frmts.h"
+
+//! @cond Doxygen_Suppress
+struct OGRLayer::Private
+{
+    bool m_bInFeatureIterator = false;
+
+    // Used by CreateFieldFromArrowSchema() and WriteArrowBatch()
+    // to store the mapping between the input Arrow field name and the
+    // output OGR field name, that can be different sometimes (for example
+    // Shapefile truncating at 10 characters)
+    // This is admitedly not super clean to store that mapping at that level.
+    // We should probably have CreateFieldFromArrowSchema() and
+    // WriteArrowBatch() explictly returning and accepting that mapping.
+    std::map<std::string, std::string> m_oMapArrowFieldNameToOGRFieldName{};
+};
+//! @endcond
+
+#endif /* OGRLAYER_PRIVATE_H_INCLUDED */

ogr/ogrsf_frmts/generic/ogrlayerarrow.cpp

@@ -35,6 +35,7 @@
 #include "ogr_swq.h"
 #include "ogr_wkb.h"
 #include "ogr_p.h"
+#include "ogrlayer_private.h"
 
 #include "cpl_float.h"
 #include "cpl_json.h"
@@ -5874,7 +5875,8 @@ bool OGRLayer::CreateFieldFromArrowSchemaInternal(
                     : OFSTNone;
         }
 
-        OGRFieldDefn oFieldDefn((osFieldPrefix + fieldName).c_str(), eTypeOut);
+        const std::string osWantedOGRFieldName = osFieldPrefix + fieldName;
+        OGRFieldDefn oFieldDefn(osWantedOGRFieldName.c_str(), eTypeOut);
         oFieldDefn.SetSubType(eSubTypeOut);
         if (eTypeOut == eTypeIn && eSubTypeOut == eSubTypeIn)
         {
@@ -5928,7 +5930,22 @@ bool OGRLayer::CreateFieldFromArrowSchemaInternal(
                 }
             }
         }
-        return CreateField(&oFieldDefn) == OGRERR_NONE;
+        auto poLayerDefn = GetLayerDefn();
+        const int nFieldCountBefore = poLayerDefn->GetFieldCount();
+        if (CreateField(&oFieldDefn) != OGRERR_NONE ||
+            nFieldCountBefore + 1 != poLayerDefn->GetFieldCount())
+        {
+            return false;
+        }
+        const char *pszActualFieldName =
+            poLayerDefn->GetFieldDefn(nFieldCountBefore)->GetNameRef();
+        if (pszActualFieldName != osWantedOGRFieldName)
+        {
+            m_poPrivate
+                ->m_oMapArrowFieldNameToOGRFieldName[osWantedOGRFieldName] =
+                pszActualFieldName;
+        }
+        return true;
     };
 
     for (const auto &sType : gasArrowTypesToOGR)
@@ -6159,15 +6176,15 @@ struct FieldInfo
     int nScale = 0;         // only used for decimal fields
 };
 
-static bool
-BuildOGRFieldInfo(const struct ArrowSchema *schema, struct ArrowArray *array,
-                  const OGRFeatureDefn *poFeatureDefn,
-                  const std::string &osFieldPrefix,
-                  const CPLStringList &aosNativeTypes, bool &bFallbackTypesUsed,
-                  std::vector<FieldInfo> &asFieldInfo, const char *pszFIDName,
-                  const char *pszGeomFieldName, OGRLayer *poLayer,
-                  const struct ArrowSchema *&schemaFIDColumn,
-                  struct ArrowArray *&arrayFIDColumn)
+static bool BuildOGRFieldInfo(
+    const struct ArrowSchema *schema, struct ArrowArray *array,
+    const OGRFeatureDefn *poFeatureDefn, const std::string &osFieldPrefix,
+    const CPLStringList &aosNativeTypes, bool &bFallbackTypesUsed,
+    std::vector<FieldInfo> &asFieldInfo, const char *pszFIDName,
+    const char *pszGeomFieldName, OGRLayer *poLayer,
+    const std::map<std::string, std::string> &oMapArrowFieldNameToOGRFieldName,
+    const struct ArrowSchema *&schemaFIDColumn,
+    struct ArrowArray *&arrayFIDColumn)
 {
     const char *fieldName = schema->name;
     const char *format = schema->format;
@@ -6179,8 +6196,9 @@ BuildOGRFieldInfo(const struct ArrowSchema *schema, struct ArrowArray *array,
             if (!BuildOGRFieldInfo(schema->children[i], array->children[i],
                                    poFeatureDefn, osNewPrefix, aosNativeTypes,
                                    bFallbackTypesUsed, asFieldInfo, pszFIDName,
-                                   pszGeomFieldName, poLayer, schemaFIDColumn,
-                                   arrayFIDColumn))
+                                   pszGeomFieldName, poLayer,
+                                   oMapArrowFieldNameToOGRFieldName,
+                                   schemaFIDColumn, arrayFIDColumn))
             {
                 return false;
             }
@@ -6228,7 +6246,17 @@ BuildOGRFieldInfo(const struct ArrowSchema *schema, struct ArrowArray *array,
     }
     else
     {
-        sInfo.iOGRFieldIdx = poFeatureDefn->GetFieldIndex(sInfo.osName.c_str());
+        const auto osExpectedOGRFieldName =
+            [&oMapArrowFieldNameToOGRFieldName, &sInfo]()
+        {
+            const auto oIter =
+                oMapArrowFieldNameToOGRFieldName.find(sInfo.osName);
+            if (oIter != oMapArrowFieldNameToOGRFieldName.end())
+                return oIter->second;
+            return sInfo.osName;
+        }();
+        sInfo.iOGRFieldIdx =
+            poFeatureDefn->GetFieldIndex(osExpectedOGRFieldName.c_str());
         if (sInfo.iOGRFieldIdx >= 0)
         {
             bool bTypeOK = false;
@@ -6495,8 +6523,8 @@ BuildOGRFieldInfo(const struct ArrowSchema *schema, struct ArrowArray *array,
         }
         else
         {
-            sInfo.iOGRFieldIdx =
-                poFeatureDefn->GetGeomFieldIndex(sInfo.osName.c_str());
+            sInfo.iOGRFieldIdx = poFeatureDefn->GetGeomFieldIndex(
+                osExpectedOGRFieldName.c_str());
             if (sInfo.iOGRFieldIdx < 0)
             {
                 if (pszGeomFieldName && pszGeomFieldName == sInfo.osName)
@@ -7267,8 +7295,9 @@ bool OGRLayer::WriteArrowBatch(const struct ArrowSchema *schema,
         if (!BuildOGRFieldInfo(schema->children[i], array->children[i],
                                poLayerDefn, std::string(), aosNativeTypes,
                                bFallbackTypesUsed, asFieldInfo, pszFIDName,
-                               pszGeomFieldName, this, schemaFIDColumn,
-                               arrayFIDColumn))
+                               pszGeomFieldName, this,
+                               m_poPrivate->m_oMapArrowFieldNameToOGRFieldName,
+                               schemaFIDColumn, arrayFIDColumn))
         {
             return false;
         }