Table to IFeatureLayer

[mathieu17g]

Here is a 1st draft to fix issue #152

I have implemented the conversion from table to an IFeatureLayer in a memory dataset.
It is done via an IFeatureLayer constructor but it can be moved to a createlayer method.

Here are some modifications that has / may need to be implemented:

• add tests
  • DONE with commits up to 843155d
• handle the case where Tables.schema returns nothing or Table.Schema{names, nothing}
  • DONE with commit d8849e4
• add documentation
  • DONE with commits up to 541ff9b
• once PR [Breaking] Return missing if the field is set but null. #238 is merged, set field/geomfield values === nothing to null
  • DONE with commits up to 6454e09
  • Note: there is no distinction between NULL and UNSET states for geometry fields, only null ptr or a pointer to a valid geometry
• (MAYBE) define a function dataset(::AbstractFeatureLayer) instead of calling ownedby property. Could be generalized to all ArchGDAL objects once Issue Model and handle relationships between GDAL objects systematically #215 has been addressed
  • DROPPED. It will be implemented with correction of Issue Model and handle relationships between GDAL objects systematically #215
• (MAYBE) in column type conversion, move from try-catch with convert to if-else with collections (to be defined)
  • SOLVED by refactoring following @yeesian suggestion
• accept column with element type <: Union{Missing, Nothing, GeoInterface.AbstractGeometry} or WKT/WKB
  • _DONE with commits up to 77d561d
• add documentation on workflow as the one mentioned in issue Documentation [question] writing a table to a layer #153
• add test on workflows
• (MAYBE) define write(::AbstractFeatureLayer, args...) to simplify table to geofile workflow syntax

Other additions:

• added convert methods between OGRwkbGeometryType and IGeometry{OGRwkbGeometryType}. Necessary in column types conversion since it is based on try-catch on convert methods
• used GDAL.ogr_gfld_setname to set first geomfield name, since setname! is not implemented for FeatureDefn. set name!(::FeatureDefn, ::String) may require to fix issue Model and handle relationships between GDAL objects systematically #215 first
• fixed @ogrerr: message has to be escaped. This fix may also be necessary in other similar macros (not tested)
• added convert methods between OGRFieldType and DataType for vectors, Int8 and Float16

Here is a test on multi_geom.csv test file:

julia> ds = AG.read("multi_geom.csv",
           options = [
               "GEOM_POSSIBLE_NAMES=point,linestring",
               "KEEP_GEOM_COLUMNS=NO",
           ],
       )
       layer = AG.getlayer(ds, 0)
       @show layer
       df = DataFrame(layer)
       @show df; println()
       @show describe(df); println()
       new_layer = AG.IFeatureLayer(df)
       @show new_layer
       new_df = DataFrame(new_layer)
       @show new_df; println()
       @show describe(new_df); println()
layer = Layer: multi_geom
  Geometry 0 (point): [wkbUnknown]
  Geometry 1 (linestring): [wkbUnknown]
     Field 0 (id): [OFTString], 5.1, 5.2
     Field 1 (zoom): [OFTString], 1.0, 2.0
     Field 2 (location): [OFTString], Mumbai, New Delhi

df = 2×5 DataFrame
 Row │ point               linestring               id      zoom    location
     │ IGeometr…           IGeometry…               String  String  String
─────┼────────────────────────────────────────────────────────────────────────
   1 │ Geometry: wkbPoint  Geometry: wkbLineString  5.1     1.0     Mumbai
   2 │ Geometry: wkbPoint  Geometry: wkbLineString  5.2     2.0     New Delhi

describe(df) = 5×7 DataFrame
 Row │ variable    mean     min     median   max        nmissing  eltype
     │ Symbol      Nothing  Union…  Nothing  Union…     Int64     DataType
─────┼─────────────────────────────────────────────────────────────────────────────────────
   1 │ point                                                   0  IGeometry{wkbPoint}
   2 │ linestring                                              0  IGeometry{wkbLineString}
   3 │ id                   5.1              5.2               0  String
   4 │ zoom                 1.0              2.0               0  String
   5 │ location             Mumbai           New Delhi         0  String

new_layer = Layer: 
  Geometry 0 (point): [wkbPoint]
  Geometry 1 (linestring): [wkbLineString]
     Field 0 (id): [OFTString], 5.1, 5.2
     Field 1 (zoom): [OFTString], 1.0, 2.0
     Field 2 (location): [OFTString], Mumbai, New Delhi

new_df = 2×5 DataFrame
 Row │ point               linestring               id      zoom    location
     │ IGeometr…           IGeometry…               String  String  String
─────┼────────────────────────────────────────────────────────────────────────
   1 │ Geometry: wkbPoint  Geometry: wkbLineString  5.1     1.0     Mumbai
   2 │ Geometry: wkbPoint  Geometry: wkbLineString  5.2     2.0     New Delhi

describe(new_df) = 5×7 DataFrame
 Row │ variable    mean     min     median   max        nmissing  eltype
     │ Symbol      Nothing  Union…  Nothing  Union…     Int64     DataType
─────┼─────────────────────────────────────────────────────────────────────────────────────
   1 │ point                                                   0  IGeometry{wkbPoint}
   2 │ linestring                                              0  IGeometry{wkbLineString}
   3 │ id                   5.1              5.2               0  String
   4 │ zoom                 1.0              2.0               0  String
   5 │ location             Mumbai           New Delhi         0  String

[yeesian]

(MAYBE) define a function dataset(::AbstractFeatureLayer) instead of calling ownedby property.

I do think it's a good idea to move away from the ownedby property -- I have ideas/plans of moving away from it in a systematic approach for addressing #171 and #215.

I'll write up about it in a RFC and get thoughts and feedback first.

[yeesian]

Unfortunately, it'll be difficult to review this PR with 38a23ef. If I still remember it right, I'd suggest

1. keeping a copy of this branch (in case something goes wrong),
2. deleting the merge commit 38a23ef (e.g. possibly in a git rebase interactive session)
3. rebasing to master using git at the command line, resolve any conflicts, and
4. force pushing to this branch

[mathieu17g]

OK as PR #245, I will close this PR and open a new one.
Currently, I have rebase to abort locally that blocks me. I'm digging into it

1. keeping a copy of this branch (in case something goes wrong),
2. deleting the merge commit 38a23ef (e.g. possibly in a git rebase interactive session)
3. rebasing to master using git at the command line, resolve any conflicts, and
4. force pushing to this branch

Thanks, I did as advised below

[yeesian]

Thanks for taking this on -- it's ambitious in all the right ways and I really appreciate it!

My comments are mostly around src/tables.jl since it has become pretty complex to follow along, and I think it needs to get to a state where it "feels obvious" for maintainability.

• The number of if-else suggests that the current system of types might not be sufficiently tight for doing this is a clean way, but
• I don't yet have a good understanding where the gap might be.
• Therefore, my suggestions are mostly around https://docs.julialang.org/en/v1/manual/performance-tips/#Break-functions-into-multiple-definitions.

[yeesian]

Thank you so much! This PR is shaping up really nicely, and I have a much better grasp of it now :)

The generated functions are really quite magical haha, so my remaining comments are mostly stylistic in nature.

[yeesian]

Thanks! I think the overall flow is there, but the amount of coherence checks and inference and so on makes me very hesitant about the API. I've suggested some changes to see if they might help keep things simple.

[mathieu17g]

@yeesian I dropped fieldtypes kwarg in src/ and test/ to check if it still works properly before stashing all the WKT/WKB parsing and geomcols kwarg

[mathieu17g]

@yeesian, in commit e8560e4, I dropped geomcols kwarg in src/ and test/

Note: Coverage is still impaired by codecov coverage not handling generated code properly. When I test coverage locally with Pkg.jl and Coverage.jl everything new is covered

[yeesian]

Should this be named as _show or something, or does it need to have the Base. prefix?

[yeesian]

Wanted to say I like the new API a lot, thank you so much for working through all the review comments!

[evetion]

This (and also Vector{Float16}) is not correct for OFSTInt16 AFAIK

[ErickChacon]

It would very nice to have this feature. It would make integration with other geometry packages easier. Is there any reason why this effort stopped? Is there still interest on this feature?