Fiona <-> OGR field mappings take 2 (refactor) #1366

sgillies · 2024-03-29T21:11:22Z

Follow up on #1358.

The project's original mappings predated OGR's sub-types (https://gdal.org/development/rfc/rfc50_ogr_field_subtype.html) and those were kinda bolted on. In this version we embrace them. We map (fieldtype, subtype) tuples to Fiona field types.

The extremely gross sections of if/else conditionals in the OGR and Fiona feature builders have been reduced to looking up a class in one of the maps and calling one of its methods. The increase in lines of code is largely explained by the new small classes and lack of normalization.

Original tests pass, save one, which had the wrong expected value.

Benchmarking: this script runs in 0.49 seconds with Fiona 1.9.6 and 0.44 seconds with the code in this PR. Measured by cProfile. So, a ~10% speedup for a 288 record dataset with features that have 60+ fields.

import fiona

with fiona.open(
    "ne_10m_admin_0_countries_lakes/ne_10m_admin_0_countries_lakes.shp"
) as src:
    with fiona.open("/tmp/test_perf.shp", "w", **src.profile) as dst:
        dst.writerecords(src)

This PR is about making the code more readable and easier to maintain. A small speedup is a nice bonus.

When reviewing, use the "split" view, it's much easier to read for these particular changes.

Resolves #1321

Instead we dispatch to cdef classes kept in several new mappings.

…321bis

fiona/ogrext.pyx

sgillies · 2024-03-31T18:31:45Z

fiona/schema.pyx

    elif ftype.startswith('int'):
        width = int((ftype.split(':')[1:] or ['0'])[0])
-        if GDAL_VERSION_NUM >= 2000000 and (width == 0 or width >= 10):


We require GDAL 3.4+. I'm removing these checks as I encounter them.

tests/test_collection.py

sgillies · 2024-03-31T19:31:47Z

tests/test_props.py

@@ -24,9 +28,10 @@ def test_types():
    assert prop_type("str") == str
    assert isinstance(0, prop_type("int"))
    assert isinstance(0.0, prop_type("float"))
-    assert prop_type("date") == FionaDateType
+    assert prop_type("date") == str


FionaDateType no longer derives from str.

sgillies · 2024-03-31T19:33:54Z

tests/test_schema.py

-    types = set(FIELD_TYPES)
-    types.remove(None)
-    return list(sorted(types)) + [None]
+    types = set(NAMED_FIELD_TYPES.keys())


FIELD_TYPES remains, but internal usage now uses the more complete NAMED_FIELD_TYPES to get, for example FionaBooleanType by its name "bool".

sgillies · 2024-03-31T19:35:35Z

fiona/ogrext.pyx

+
+    cdef set(self, OGRFeatureH feature, int i, object value, object kwds):
+        """Set the value of a feature's field."""
+        raise NotImplementedError


The classes that derive from this one replace the ugly if/def statements in the feature builders.

sgillies · 2024-03-31T19:36:26Z

fiona/ogrext.pyx

@@ -225,7 +559,37 @@ cdef class FeatureBuilder:
    argument is not destroyed.
    """

-    cdef build(self, void *feature, encoding='utf-8', bbox=False, driver=None, ignore_fields=None, ignore_geometry=False):
+    cdef object driver
+    cdef object property_getter_cache


I've added some state to the class so that we can reuse field getter and setters.

sgillies · 2024-03-31T19:40:08Z

fiona/ogrext.pyx

-                    props[key] = bool(OGR_F_GetFieldAsInteger64(feature, i))
+            else:
+                if fieldkey in self.property_getter_cache:
+                    getter = self.property_getter_cache[fieldkey]


Here we try to reuse an existing field getter.

sgillies · 2024-03-31T19:40:57Z

fiona/ogrext.pyx

-                        string_list_index += 1
-            else:
-                props[key] = None
+                props[key] = getter.get(feature, i, {"encoding": encoding})


All that old crap replaced by this 😄

sgillies · 2024-03-31T19:42:36Z

fiona/ogrext.pyx

+                    value = json.dumps(value)
+
+                setter.set(cogr_feature, i, value, {"encoding": encoding})
+


I'm immoderately pleased about this.

sgillies · 2024-03-31T19:44:48Z

fiona/ogrext.pyx


        if cogr_layer == NULL:
            raise ValueError("Null layer")

+        cdef OGRFeatureBuilder feat_builder = OGRFeatureBuilder(driver=collection.driver)


Here's an optimization that I've overlooked for a while: reusing the feature builder.

To reproduce behavior of Fiona 1.9

sgillies · 2024-04-01T14:12:08Z

fiona/ogrext.pyx

+        (OFTBinary, OFSTNone, "bytearray"): BinaryField,
+        (OFTBinary, OFSTNone, "memoryview"): BinaryField,
+        (OFTStringList, OFSTNone, "list"): StringListField,
+        (OFTString, OFSTJSON, "dict"): JSONField,


Dispatching to a field setter based on field type and value type restores the implicit behavior in fiona version 1.9.x.

sgillies · 2024-04-01T15:31:25Z

@mwtoews @snorfalorpagus @perrygeo I'm tagging you for review of this refactor PR. Meanwhile I'm going to work on benchmarking.

sgillies · 2024-04-03T15:47:42Z

Last call for review! It's a lot to digest, I know, and if you're primarily using fiona via geopandas, maybe not interesting. Still, I appreciate a skim if you can.

sgillies and others added 4 commits March 12, 2024 14:07

Add an "int16" property type

1ce26cd

Resolves #1321

Update reference in change log

13b65c0

Code cleanup to improve readability

32277d0

Refactor OGR and Fiona feature builders to replace ugly conditionals

7a074ac

Instead we dispatch to cdef classes kept in several new mappings.

sgillies added the refactor label Mar 29, 2024

sgillies added this to the 1.10 milestone Mar 29, 2024

sgillies self-assigned this Mar 29, 2024

sgillies added 2 commits March 29, 2024 15:16

Refactor OGR and Fiona feature builders to replace ugly conditionals

314a1c9

Instead we dispatch to cdef classes kept in several new mappings.

Merge branch 'issue1321bis' of github.com:Toblerity/Fiona into issue1…

af68bd7

…321bis

sgillies changed the title ~~Issue1321bis~~ Fiona <-> OGR field mappings take 2 Mar 29, 2024

sgillies marked this pull request as draft March 29, 2024 21:23

sgillies commented Mar 29, 2024

View reviewed changes

fiona/ogrext.pyx Outdated Show resolved Hide resolved

sgillies commented Mar 29, 2024

View reviewed changes

fiona/ogrext.pyx Outdated Show resolved Hide resolved

sgillies added 3 commits March 29, 2024 16:15

xfail a JSON property test for GDAL < 3.5

261f81e

xfail another

85f457f

New feature builder implementation with cleaner OO logic

aac2dfc

sgillies commented Mar 31, 2024

View reviewed changes

tests/test_collection.py Outdated Show resolved Hide resolved

sgillies commented Mar 31, 2024

View reviewed changes

sgillies added 3 commits March 31, 2024 13:54

Fix imports and update change log

be35e37

Fix detection of tz support in DateTimeField

d6ca67e

Register property setters to OGR field type and value type

a7bc597

To reproduce behavior of Fiona 1.9

sgillies commented Apr 1, 2024

View reviewed changes

sgillies changed the title ~~Fiona <-> OGR field mappings take 2~~ Fiona <-> OGR field mappings take 2 (refactor) Apr 1, 2024

sgillies requested review from mwtoews, perrygeo and snorfalorpagus April 1, 2024 14:51

sgillies marked this pull request as ready for review April 1, 2024 15:29

sgillies added 2 commits April 3, 2024 07:30

Make lists JSON serializable

69fa575

Merge branch 'main' into issue1321bis

2622687

sgillies merged commit 6581da6 into main Apr 3, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fiona <-> OGR field mappings take 2 (refactor) #1366

Fiona <-> OGR field mappings take 2 (refactor) #1366

sgillies commented Mar 29, 2024 •

edited

Loading

sgillies Mar 31, 2024 •

edited

Loading

sgillies Mar 31, 2024

sgillies Mar 31, 2024

sgillies Mar 31, 2024

sgillies Mar 31, 2024

sgillies Mar 31, 2024

sgillies Mar 31, 2024

sgillies Mar 31, 2024

sgillies Mar 31, 2024

sgillies Apr 1, 2024

sgillies commented Apr 1, 2024

sgillies commented Apr 3, 2024 •

edited

Loading

		value = json.dumps(value)

		setter.set(cogr_feature, i, value, {"encoding": encoding})

Fiona <-> OGR field mappings take 2 (refactor) #1366

Fiona <-> OGR field mappings take 2 (refactor) #1366

Conversation

sgillies commented Mar 29, 2024 • edited Loading

sgillies Mar 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgillies commented Apr 1, 2024

sgillies commented Apr 3, 2024 • edited Loading

sgillies commented Mar 29, 2024 •

edited

Loading

sgillies Mar 31, 2024 •

edited

Loading

sgillies commented Apr 3, 2024 •

edited

Loading