Opened 8 years ago

Closed 8 years ago

#6235 closed defect (fixed)

MSSpatial (when connecting to SQL Server) set varchar primary keys as FID integer

Reported by: robe Owned by: tamas
Priority: normal Milestone: 2.0.2
Component: default Version: 2.0.1
Severity: major Keywords: mssql
Cc:

Description

I haven't tested this on lower and maybe this is the first time I've tried to query a table that didn't have an integer for primary key.

If my table has a primary key and that column is say a varchar(50), then ogr_info and friends map it to FID but set it as an integer, so all the values end up coming back as 0. So to work around the issue I always have to take off the primary key.

I thought it may be a general ODBC issue, but I tried it on an MS Access db with a table that has a varchar field and it doesn't try to map it to FID.

For perspective here is an example OGR info output from a SQL Server 2008 R2. I get the same behavior with both ODBC and MSSpatial driver

That pgName field is the primary key and if I remove the primary key, then it comes thru as a regular old field.

Layer name: dbo.PersGroups
Geometry: Unknown (any)
Feature Count: 11
Layer SRS WKT:
(unknown)
FID Column = pgName
pgRights: String (0.0)
pgTemplates: String (1000.0)
pgRecordSchedule: String (2000.0)
pgParentName: String (50.0)
OGRFeature(dbo.PersGroups):0
  pgRights (String) = *AllRights
  pgTemplates (String) = (null)
  pgRecordSchedule (String) = (null)
  pgParentName (String) = (null)

Attachments (4)

6235.patch (2.3 KB ) - added by Even Rouault 8 years ago.
Proposed patch
robe6235.patch (1011 bytes ) - added by robe 8 years ago.
patch to ignore non integer FIDs
6235_brach20.patch (2.2 KB ) - added by robe 8 years ago.
patch for branch 2.0
6235_brach20.results (2.4 KB ) - added by robe 8 years ago.
test results of branch on msspatial driver

Download all attachments as: .zip

Change History (15)

comment:1 by robe, 8 years ago

Just confirmed see the same behavior in GDAL 2.1.0 http://www.gisinternals.com/development.php(temas binaries). So guess not specific to my compile.

comment:2 by Even Rouault, 8 years ago

Keywords: mssql added
Owner: changed from warmerdam to tamas

comment:3 by robe, 8 years ago

In talks with EvenR on IRC he gave me some pointers on how to fix. I'll take a stab at fixing this and submit a patch once I've tested on my SQL Server and MS Access databases.

comment:4 by robe, 8 years ago

Summary: MSSpatial and ODBC (when connecting to SQL Server) set varchar primark keys as FID integerMSSpatial (when connecting to SQL Server) set varchar primary keys as FID integer

Changing ticket title. I was mistaken, it's only the MSSSpatial driver that converts the primary key to FID Column even if it's not an integer. Actually the ODBC doesn't even do that with integer primary keys.

Here is same output using the ODBC driver:

      using driver `ODBC' successful.

Layer name: dbo.PersGroups
Geometry: Unknown (any)
Feature Count: 11
Layer SRS WKT:
(unknown)
pgName: String (50.0)
pgRights: String (0.0)
pgTemplates: String (1000.0)
pgRecordSchedule: String (2000.0)
pgParentName: String (50.0)
OGRFeature(dbo.PersGroups):0
  pgName (String) = Administrators
  pgRights (String) = *AllRights
  pgTemplates (String) = (null)
  pgRecordSchedule (String) = (null)
  pgParentName (String) = (null)

I guess that's why I never noticed this issue before because it's only recently I started using MSSPatial driver to save myself the hassle of registering an ODBC connection.

comment:5 by Jukka Rahkonen, 8 years ago

There seems to be an ancient open ticket about the ODBC case https://trac.osgeo.org/gdal/ticket/1006

comment:6 by robe, 8 years ago

I'm actually complaining about MSSpatial here. ODBC works the way I would want, but I think ODBC works because the logic it has to check for primary key for FID I believe is broken as noted in the ticket you referred to.

ODBC does have provisions for specifying an FID, though I didn't test that out to see if it works:

https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogrsf_frmts/odbc/ogrodbclayer.cpp#L177

In the case of MSSSpatial, it tries to use the primary key of the table here:

https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatialtablelayer.cpp#L146

(If I wipe out all this primary key checking code just set FID column to null, things work the same as ODBC against SQL Server (except in the case of MSSPatial still using an identity field for FID if one is available)

Because of this code https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatiallayer.cpp#L156

I thought the easiest place to fix the issue that it tries to use a non-integer primary key as FID (and fails cause evidentally FID must be an integer) was around here:

https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogrsf_frmts/mssqlspatial/ogrmssqlspatiallayer.cpp#L142

If it's not an identity or bigint, just reset pszFIDColumn to NULL. I'm going to give that another try, my last try didn't seem to work, but I think maybe the continue got in my way.

Why the ODBC logic here https://trac.osgeo.org/gdal/browser/trunk/gdal/ogr/ogrsf_frmts/odbc/ogrodbctablelayer.cpp#L106 doesn't seem to work is a big mystery to me or maybe it is working and just gets lost somewhere else.

This is my first time really studying GDAL/OGR code, so I'm having a hard-time like figuring out where oGetKey is coming from. The oGetKey.GetColData(3) I presume comes from a query somewhere that pulls namespace, schema, table, column name (thus picking the 4th cell corresponding to col 3)

Being C++ ignorant doesn't help much.

by Even Rouault, 8 years ago

Attachment: 6235.patch added

Proposed patch

comment:7 by Even Rouault, 8 years ago

@robe Could you try the attached patch ? I could just check it compiles

comment:8 by robe, 8 years ago

roualt,

Okay will give it a try. FWIW I came up with a patch too changes same area of code, but didn't take out those other parts like yours. Yours looks much cleaner :).

I also came up with a test but haven't figured out how to add it in. I've attached the patch for posterity and will list results of both patches to compare against current behavior.

Last edited 8 years ago by robe (previous) (diff)

by robe, 8 years ago

Attachment: robe6235.patch added

patch to ignore non integer FIDs

comment:9 by robe, 8 years ago

Here is test data to test the different scenarios, stiill have to figure out how to make this a GDAL test.

-- primary key identity, should be picked up as FID
CREATE TABLE [dbo].[testogr1](
	[testpk] [varchar](50) NOT NULL,
	[geom] [geometry] NULL,
	[testf] [varchar](50) NULL,
	[id] [int] IDENTITY(1,1) NOT NULL PRIMARY KEY)
GO

INSERT INTO testogr1(testpk,geom, testf)
VALUES ('testpk 1', Geometry::STGeomFromText('POINT(1 2)', 4326), 'test field 1'),
('testpk 2', Geometry::STGeomFromText('POINT(1 3)', 4326), 'test field 2');

GO

-- primary key is text field, but has an identity field
-- identity field should be picked up as FID
CREATE TABLE [dbo].[testogr2](
	[testpk] [varchar](50) NOT NULL PRIMARY KEY,
	[geom] [geometry] NULL,
	[testf] [varchar](50) NULL,
	[id] [int] IDENTITY(1,1) NOT NULL)
GO

INSERT INTO testogr2(testpk,geom, testf)
VALUES ('testpk 1', Geometry::STGeomFromText('POINT(1 2)', 4326), 'test field 1'),
('testpk 2', Geometry::STGeomFromText('POINT(1 3)', 4326), 'test field 2');


-- primary key is text field, but and has no identity
-- generic FID should be created
CREATE TABLE [dbo].[testogr3](
	[testpk] [varchar](50) NOT NULL PRIMARY KEY,
	[geom] [geometry] NULL,
	[testf] [varchar](50) NULL)
GO

INSERT INTO testogr3(testpk,geom, testf)
VALUES ('testpk 1', Geometry::STGeomFromText('POINT(1 2)', 4326), 'test field 1'),
('testpk 2', Geometry::STGeomFromText('POINT(1 3)', 4326), 'test field 2');

comment:10 by robe, 8 years ago

roualt,

I tested your 6235.patch on 2.0, didn't get a chance to test on 2.1. For 2.0 I had to modify replacing the STARTS_WITH_CI with equivalent EQUALSN.

Haven't had a chance to test 2.1, but don't see why it wouldn't work if more or less same code works on 2.0.

Attached is your patch modified to work for 2.0 and results which match what I would expect them to be.

by robe, 8 years ago

Attachment: 6235_brach20.patch added

patch for branch 2.0

by robe, 8 years ago

Attachment: 6235_brach20.results added

test results of branch on msspatial driver

comment:11 by Even Rouault, 8 years ago

Resolution: fixed
Status: newclosed

trunk r31801, branches/2.0 r31802 "MSSQLSpatial: do not treat a primary key that is not of integer type as the FID (#6235)"

Note: See TracTickets for help on using tickets.