Skip to content

Arrow: fix extension name key in field metadata#5829

Merged
rouault merged 2 commits intoOSGeo:masterfrom
jorisvandenbossche:arrow-extension-name
Jun 1, 2022
Merged

Arrow: fix extension name key in field metadata#5829
rouault merged 2 commits intoOSGeo:masterfrom
jorisvandenbossche:arrow-extension-name

Conversation

@jorisvandenbossche
Copy link
Copy Markdown
Contributor

What does this PR do?

The current field metadata key for the extension name is using a double colon, but that should be a single colon, see https://arrow.apache.org/docs/format/Columnar.html#extension-types

Noticed this while testing to read GDAL produced files with pyarrow with extension types registered for those "geoarrow.<geometry_type>" types (https://github.com/jorisvandenbossche/python-geoarrow/)

Tasklist

  • Add test case(s)
  • Add documentation
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

Does this require a test case?
I see that this key is actually checked for when reading, but since it also checks in the metadata for the known geometry columns (and its encoding), this ARROW:extension:name key is not required I suppose.

@rouault
Copy link
Copy Markdown
Member

rouault commented May 31, 2022

Does this require a test case?
I see that this key is actually checked for when reading, but since it also checks in the metadata for the known geometry columns (and its encoding), this ARROW:extension:name key is not required I suppose.

One way of checking the fix has effect is to add a new test case in autotest/ogr/ogr_arrow.py and generate a GeoArrow file with the following configuration options set:

  • OGR_ARROW_WRITE_GDAL_FOOTER=NO
  • OGR_ARROW_WRITE_GEO=NO
    which will prevent both the "geo" and "gdal:geo" metadata to be written.
    And open it back and check that the geometry column is correctly detected.
    I can help writing it if needed

@jorisvandenbossche
Copy link
Copy Markdown
Contributor Author

Thanks for those pointers. Did a first attempt to add a test based on that (and verified that the test failed before the patch)

@rouault rouault merged commit e615c45 into OSGeo:master Jun 1, 2022
@jorisvandenbossche jorisvandenbossche deleted the arrow-extension-name branch June 1, 2022 10:49
a0x8o pushed a commit to a0x8o/gdal that referenced this pull request Jun 6, 2022
a0x8o pushed a commit to a0x8o/gdal that referenced this pull request Jun 10, 2022
a0x8o pushed a commit to a0x8o/gdal that referenced this pull request Jun 15, 2022
a0x8o pushed a commit to a0x8o/gdal that referenced this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants