Implement a Tables interface for features

[Sov-trotter]

Ran into problems since I updated my fork with the old tables branch of origin.

[Sov-trotter]

If this is right then should I push the updated dataset?
https://github.com/Sov-trotter/ArchGDALDatasets/blob/master/data/multi_geom.csv

[yeesian]

Yeah sure! I think that will help -- maybe we can have it as a new test case, rather than modifying an existing one?

[Sov-trotter]

One thing to note is that for csv files (like the one here) GDAL would read all fields except geometry ones as String. This generally shouldn't be a problem?
However in the case of missing fields, the data is actually a String i.e. "missing". This may cause issues if someone wishes to use something like dropmissing()?

[yeesian]

Thanks! I have a few minor nits, but this PR should be good to go from my end.

@visr, anything else from your end?

[evetion]

Shall we test this on an (existing) shapefile or geopackage as well?

[Sov-trotter]

Yeah. That does sound cool. Though there isn't any existing geopackage dataset in the repo.
Since adding tests for shp and gpkg will be easy, I wish to leave it out for this year's upcoming Hacktoberfest as good first issues? I know a few people willing to make contributions?

[yeesian]

Hey everyone, we're close to getting this PR merged -- let's see it through to completion.

@visr @evetion are there any specific requests that must go into this PR, that we cannot open as additional feature requests?

I'll merge this PR if I don't hear any blockers in the next 24hours.

[visr]

I see the Base.getindex(t::Table, idx::Int) is 0-based. Shouldn't that be 1-based? And I think it would be clearer if the docs use the table[i] syntax rather than getindex(table, i).
Otherwise I think it's good to merge this.

The iteration state issue I described in #118 (comment) is still present I believe. But we could also turn that into an issue, and tackle it later.

[Sov-trotter]

@visr can you take a look?

[visr]

Thanks, looks good.

[Sov-trotter]

Btw the issue that you described above, I think it is now handled well in the interface unless one goes around inappropriate indices?

[evetion]

I'm fine with merging if @visr items are merged. Things like shapefile, geopackage testing are nice Hacktoberfest issues.

Happy to have this functionality in. ☺️

[visr]

Btw the issue that you described above, I think it is now handled well in the interface unless one goes around inappropriate indices?

No it is still there, just try running iterate(gt, 2) several times on such a table. The state (2) is not connected to the GDAL tracked current row. Connecting this in some way would perhaps be a nice improvement, and less prone to errors, but doesn't need to be a part of this PR.

[yeesian]

@visr I agree that Base.iterate() should not be used as a public method. I think a robust way of handling it might be use setnextbyindex!(layer, i) inside Base.iterate(), but it'll make it unusable for drivers that have O(n) performance in setting the index.

FWIW, the same issue is there for the current code, and we're lucky we haven't run into any issues so far. We can open a feature request for us to wrap the layer/table (similar to how we do it for rasterbands) and have ways of qualifying the state (rather than using arbitrary integers).

[yeesian]

This is huge, congrats @Sov-trotter! :)