Opened 15 years ago

Closed 14 years ago

#2694 closed defect (fixed)

OGRLayer::GetFIDColumn() and GetGeometryColumn() just return empty strings from Python bindings

Reported by: jjr8 Owned by: warmerdam
Priority: normal Milestone: 1.6.3
Component: OGR_SF Version: 1.5.0
Severity: normal Keywords: pgeo
Cc:

Description

I tried this with both the shape and pgeo drivers and they both returned empty strings. I did not try this with any other language bindings, so I don't know if it is a problem with the Python bindings or with the drivers themselves. I looked through previous bugs and came across #2013, which suggests these functions are supposed to work for Python bindings in 1.5.0. I'm assuming the problem is with the drivers. Please reassign as appropriate.

To repro:

  1. On Windows, follow steps 1 and 2 on #2688 to install Python 2.5.2, GDAL 1.5.0, and the Python bindings.
  1. Download the attached ZIP and uncompress to C:\. It will create a directory C:\TestPGeo with a shapefile and personal geodatabase in it.
  1. From a CMD shell, run C:\Python25\python.exe and run this:
>>> ds = ogr.Open('C:\\TestPGeo\\TestPoints.shp')
>>> layer = ds.GetLayer(0)
>>> layer.GetFIDColumn()
''
>>> layer.GetGeometryColumn()
''
>>> del layer,ds
>>> ds = ogr.Open('C:\\TestPGeo\\MyGeoDB.mdb')
>>> layer = ds.GetLayerByName('TestPoints')
>>> layer.GetFIDColumn()
''
>>> layer.GetGeometryColumn()
''

This is not a critical bug for my application. For my next release, I only need to read shapefiles and PGDBs, and the names of the FID and geometry columns are well-known for these formats. I can just hard code them. But the overall usability of OGR would be improved if all drivers returned these consistently. (I do understand there are some issues for formats that support multiple geometry columns, but it seems straightforward to implement it for all drivers that support just one column.)

Thanks for looking at this.

Attachments (1)

TestPGeo.zip (15.7 KB ) - added by jjr8 15 years ago.
Test data for this ticket

Download all attachments as: .zip

Change History (7)

by jjr8, 15 years ago

Attachment: TestPGeo.zip added

Test data for this ticket

comment:1 by warmerdam, 15 years ago

Keywords: pgeo added

The GetFIDColumn() and GetGeometryColumn() methods have only been implemented for a few of the RDBMS based providers. They do not make sense for shapefiles. They would make sense for the PGEO driver but just haven't been implemented. If you see a particular near for them, let me know and I could add them to the PGEO driver.

comment:2 by warmerdam, 15 years ago

Milestone: 1.6.0

dropping milestone ...

in reply to:  1 comment:3 by jjr8, 15 years ago

Replying to warmerdam:

Our application is currently built largely on ArcGIS because that's what most of our users use. But we're trying to expand the utility of our application for non-ESRI users by leveraging OGR where possible, and also trying to improve performance for ESRI users (OGR is waaaay faster than ArcGIS).

In our application, we have a wrapper around OGR that emulates the ArcGIS geoprocessor's cursor APIs. (See here for descriptions of those APIs.) The main part of our application sits on top of the wrapper. We are enhancing the wrapper to determine what kind of data source the caller wants to access and internally call OGR when it supports that source and call ArcGIS when it doesn't.

The ArcGIS cursor APIs require the caller ask for all fields by name, including the geometry field. (See here for an example.) OGR requires the caller to invoke the GetGeometryRef() function. Since our wrapper emulates the ArcGIS cursor APIs, it must accept a field name and then decide whether it is the geometry field or not, and either call GetGeometryRef() or one of the GetFieldAsXXXXX() functions.

It would be convenient if our wrapper could rely on OGR to tell us the name of the geometry field (and FID field, which has this same issue). If OGR does not intend to provide this as a reliable service, it is ok, but too bad for us. It means that when we switch our wrapper to call OGR for more and more data sources besides shape and PGeo, we will have to hard code in the names of the geometry and FID fields. We wouldn't have to do that if OGR could provide the names to us.

It is true that OGR could be said to provide a better interface than the ArcGIS one, because OGR allows the caller to get the field without knowing its name. Unfortunately we need to emulate the ArcGIS cursor interface. We can't really redo our wrapper to be more like OGR.

comment:4 by warmerdam, 15 years ago

jjr8,

I don't know all the implications of your wrapper layer, but there are a few things you might want to keep in mind:

1) Even in the rare circumstances where OGR does return information on the names of the FID and GEOMETRY columns, they are not necessarily visible as part of the schema (field list) reported as part of the OGRFeatureDefn for that Layer.

2) There is no generic mechanism to instruct OGR to use a different field from the default as the FID or geometry.

3) There is a standard pseudo-name for the FID field according to RFC 6 (http://trac.osgeo.org/gdal/wiki/rfc6_sqlgeom) which is "FID". There are also some pseudo-field names for different views of the geometry (OGR_GEOMETRY and OGR_GEOM_WKT).

4) I'm happy to extend the PGEO driver to return the names of the underlying FID and geometry columns but ultimately you will still need to deal with lots of others drivers that don't have this information with some sort of hard coded names.

Let me know if you feel it would be valuable to have the FID and geometry field names from the PGEO driver.

in reply to:  4 comment:5 by jjr8, 15 years ago

Replying to warmerdam:

1) Even in the rare circumstances where OGR does return information on the names of the FID and GEOMETRY columns, they are not necessarily visible as part of the schema (field list) reported as part of the OGRFeatureDefn for that Layer.

Yes, I did notice that. For example, if I recall correctly, the shape driver omits FID from the field list but the PGeo driver does include OBJECTID. I can deal with this.

Question: does this impact the ability to reference the FID field (whatever it happens to be called) in a call to ExecuteSQL? There are cases where it would be very convenient to order on FID. If the ability to do this depends on whether the driver happens to include the FID field in the field list, then this might be a problem for us.

2) There is no generic mechanism to instruct OGR to use a different field from the default as the FID or geometry.

That is ok with us as far as I know.

3) There is a standard pseudo-name for the FID field according to RFC 6 (http://trac.osgeo.org/gdal/wiki/rfc6_sqlgeom) which is "FID". There are also some pseudo-field names for different views of the geometry (OGR_GEOMETRY and OGR_GEOM_WKT).

I think this answers my question posed above. I think the best way to order by FID is to use the "FID" pseudonym rather than the name of the FID field. Is this correct? Will it work reliably regardless of driver?

It is also very nice that OGR_GEOMETRY is available. The ArcGIS cursor API that I'm trying to emulate allows you to request specific fields, but I think you automatically always get the geometry field (not 100% sure though). If this is true, then OGR_GEOMETRY provides a convenient way for me to implement that.

4) I'm happy to extend the PGEO driver to return the names of the underlying FID and geometry columns but ultimately you will still need to deal with lots of others drivers that don't have this information with some sort of hard coded names.

Given this, I would say don't bother to do it for now. For my situation, it would be convenient if there was consistent support across the drivers. From my POV, it would be better to take that on as a low priority enhancement for some future release, rather than take this one off case as yet another 1.6.0 checkin when you're trying to get 1.6.0 done. The hard-coding is a minor issue for us. But thank you for offering.

comment:6 by Even Rouault, 14 years ago

Milestone: 1.6.3
Resolution: fixed
Status: newclosed

r17844 /trunk/gdal/ogr/ogrsf_frmts/pgeo/ (ogr_pgeo.h ogrpgeolayer.cpp): PGEO : implement GetFIDColumn() and GetGeometryColumn() methods r17845 /branches/1.6/gdal/ogr/ogrsf_frmts/pgeo/ (ogr_pgeo.h ogrpgeolayer.cpp): PGEO : implement GetFIDColumn() and GetGeometryColumn() methods

Note: See TracTickets for help on using tickets.