Opened 12 years ago

Closed 9 years ago

#1960 closed defect (fixed)

OCI: ogr2ogr with append option is not considering layer creation option(lco) values

Reported by: Spatial01 Owned by: ilucena
Priority: normal Milestone:
Component: OGR_SF Version: 1.4.2
Severity: normal Keywords: append lco dim oracle oci
Cc: warmerdam

Description

When using the 'append' option of ogr2ogr with -lco DIM=2 with OCI driver for Oracle it's creating geometries in 3 dimensionality even though I have specified 2 dimension using DIM=2 option.

Here is the syntax I have used:

ogr2ogr -append -update -f OCI "OCI:username/password@tnsalias" -lco DIM=2 -lco SRID=8307 -nln table1 Shapefile.shp

My geometry looks like this after executing the above command:

sdo_geometry(3003,8307,Null,SDO_ELEM_INFO_ARRAY(1,1003,3),SDO_ORDINATE_ARRAY(1,1, 5,1, 5,7, 1,7, 1,1))

But the correct entry should be 2003 instead of 3003

sdo_geometry(2003,8307,Null,SDO_ELEM_INFO_ARRAY(1,1003,3),SDO_ORDINATE_ARRAY(1,1, 5,1, 5,7, 1,7, 1,1))

Attachments (2)

ogrocitablelayer.cpp (62.6 KB) - added by Nicolas Simon 11 years ago.
ogrocitablelayer.2.cpp (66.9 KB) - added by bala 11 years ago.
With geometry dimension functionality

Download all attachments as: .zip

Change History (30)

comment:1 Changed 12 years ago by warmerdam

Cc: warmerdam added
Milestone: 1.5.0
Owner: changed from warmerdam to Mateusz Łoskot

Spatial01,

Layer creation options are not used in append mode since the layer is not being created.

Assuming the layer was originally created with DIM=2, I think the driver needs to detect that when the layer is accessed, and thereafter work in 2D mode.

Is it practical for you to confirm that this problem remains the same with GDAL/OGR 1.4.2 or newer? In fact, I suspect there is already an outstanding bug report on this particular issue.

Turning over to Mateusz to address along with a batch of other oracle issues for the 1.5 release. I will also modify ogr2ogr to issue a warning if layer creation options are used along with -append to reduce confusion.

comment:2 Changed 12 years ago by Spatial01

Version: 1.3.21.4.2

I actually tested with 1.4.2 version. Sorry about that typing wrong version number. In Oracle there is no way of knowing dimensionality if the table is empty and there is no spatial index on it. After testing more I found couple of things that are happening in append mode.

-It commits after each record since MULTI_LOAD option is not considered in append mode.

  • It is not using bind variables with INSERT statement and is causing high CPU usage by forcing Oracle to parse every INSERT statement in append mode. But if ogr2ogr is creating a table and inserts the data it binds all columns with variables. Can we make MULTI_LOAD as default option in append mode?

I think we have to incorporate the following checks into OCI layer to check for dimensionality.

If data exists in table, check for existing geometry dimensionality.

select a.geometry_column.get_Dims() from table_name a where rownum=1

If no data but spatial index exists then check for index dimensionality.

select m.sdo_index_dims from all_sdo_index_info i, all_sdo_index_metadata m where i.sdo_index_owner=m.sdo_index_owner and i.index_name=m.sdo_index_name and i.table_name='TABLE_NAME'

comment:3 Changed 12 years ago by warmerdam

Priority: highnormal
Severity: majornormal

comment:4 Changed 12 years ago by warmerdam

Milestone: 1.5.01.5.1
Summary: ogr2ogr with append option is not considering layer creation option(lco) valuesOCI: ogr2ogr with append option is not considering layer creation option(lco) values

comment:5 Changed 11 years ago by mloskot

Keywords: oracle added
Status: newassigned

Changed 11 years ago by Nicolas Simon

Attachment: ogrocitablelayer.cpp added

comment:6 Changed 11 years ago by Nicolas Simon

Hello,

I've adapted the OCI driver to handle the dimension according to the dimension of the data that already exists. This overwrite the default dimension setting.

To do that, I've included this code in the function OGROCITableLayer::ReadTableDefinition? (file ogrocitablelayer.cpp)

I've tested it succesfully and it's easy to understand.

/* ------------------------------------------------------------ */ /* Identify Geometry dimension */ /* ------------------------------------------------------------ */

OGROCIStringBuf oDimCmd; OGROCIStatement oDimStatement( poSession ); char papszResult; int iDim = -1;

oDimCmd.Append( "SELECT a." ); oDimCmd.Append( pszGeomName ); oDimCmd.Append( ".GET_DIMS() DIM FROM " ); oDimCmd.Append( pszTable ); oDimCmd.Append( " a WHERE ROWNUM = 1" );

oDimStatement.Execute( oDimCmd.GetString?() );

papszResult = oDimStatement.SimpleFetchRow?();

if( CSLCount(papszResult) < 1 )

CPLDebug( "OCI", "get dim based on data failed." );

else {

iDim = atoi(papszResult[0]); SetDimension?( iDim );

}

I join the full source

May this code help

Nicolas

comment:7 Changed 11 years ago by bala

Hi Nicolas, Thanks for providing the code. When I tested the it error out since I use layer name <owner>.<tablename>. So I copied the split function from another function.

OGROCITableLayer::ReadTableDefinition?? (file ogrocitablelayer.cpp)

158 /* -------------------------------------------------------------------- */ 159 /* Split out the owner if available. */ 160 /* -------------------------------------------------------------------- */ 161 const char *pszTableName = pszTable; 162 char *pszOwner = NULL; 163 164 if( strstr(pszTableName,".") != NULL ) 165 { 166 pszOwner = CPLStrdup(pszTableName); 167 pszTableName = strstr(pszTableName,".") + 1; 168 169 *(strstr(pszOwner,".")) = '\0'; 170 }

Also I have modified the dimension query so that it does not depend on the table having data. Here is the modified code.

OGROCITableLayer::ReadTableDefinition?? (file ogrocitablelayer.cpp)

256 /* -------------------------------------------------------------------- */ 257 /* Identify Geometry dimension */ 258 /* -------------------------------------------------------------------- */ 259 260 OGROCIStringBuf oDimCmd; 261 OGROCIStatement oDimStatement( poSession ); 262 char papszResult; 263 int iDim = -1; 264 265 oDimCmd.Append( "SELECT COUNT(*) FROM ALL_SDO_GEOM_METADATA u,TABLE(u.diminfo) t" ); 266 oDimCmd.Append( " WHERE u.TABLE_NAME='" ); 267 oDimCmd.Append( pszTableName ); 268 oDimCmd.Append( "' AND u.COLUMN_NAME='" ); 269 oDimCmd.Append( pszGeomName ); 270 oDimCmd.Append( "'" ); 271 272 oDimStatement.Execute( oDimCmd.GetString?() ); 273 274 papszResult = oDimStatement.SimpleFetchRow?(); 275 276 if( CSLCount(papszResult) < 1 ) 277 CPLDebug( "OCI", "get dim based on all_sdo_geom_metadata failed." ); 278 else 279 { 280 iDim = atoi(papszResult[0]); 281 SetDimension?( iDim ); 282 } 283 284 /* -------------------------------------------------------------------- */ 285 /* End modif */ 286 /* -------------------------------------------------------------------- */

Thanks again. It will be good if someone can modify UnboundCreateFeature? to use bind variables and commit after every 100 rows instead of after each row.

-Bala

Changed 11 years ago by bala

Attachment: ogrocitablelayer.2.cpp added

With geometry dimension functionality

comment:8 Changed 10 years ago by warmerdam

Keywords: og2ogr removed
Milestone: 1.5.41.7.0
Owner: changed from Mateusz Łoskot to warmerdam
Status: assignednew

Pushed to 1.7.0 for review, taking it back myself.

comment:9 Changed 10 years ago by warmerdam

Milestone: 1.7.0
Owner: changed from warmerdam to ilucena

Turning over to Ivan.

comment:10 Changed 10 years ago by ilucena

Status: newassigned

Committed revision r19477

I loaded two polygon on each one of those commands:

% ogr2ogr -f OCI oci:scott/tiger@orcl test.shp -lco DIM=2
% ogr2ogr -append -update -f OCI oci:scott/tiger@orcl -nln test -lco DIM=2 test2.shp
WARNING: Layer creation options ignored since an existing layer is
         being appended to.

And this is the resulting geometries:

SQL> select ora_geometry from test;

ORA_GEOMETRY(SDO_GTYPE, SDO_SRID, SDO_POINT(X, Y, Z), SDO_ELEM_INFO, SDO_ORDINAT
--------------------------------------------------------------------------------
SDO_GEOMETRY(2003, NULL, NULL, SDO_ELEM_INFO_ARRAY(1, 1003, 1), SDO_ORDINATE_ARR
AY(-74, 40, -73.999995, 40, -73.999995, 40.000009, -74, 40.000009, -74, 40))

SDO_GEOMETRY(2003, NULL, NULL, SDO_ELEM_INFO_ARRAY(1, 1003, 1), SDO_ORDINATE_ARR
AY(-73.999995, 40, -73.999991, 40, -73.999991, 40.000009, -73.999995, 40.000009,
 -73.999995, 40))

SDO_GEOMETRY(2003, NULL, NULL, SDO_ELEM_INFO_ARRAY(1, 1003, 1), SDO_ORDINATE_ARR
AY(-74.000005, 40, -74, 40, -74, 40.000009, -74.000005, 40.000009, -74.000005, 4
0))


ORA_GEOMETRY(SDO_GTYPE, SDO_SRID, SDO_POINT(X, Y, Z), SDO_ELEM_INFO, SDO_ORDINAT
--------------------------------------------------------------------------------
SDO_GEOMETRY(2003, NULL, NULL, SDO_ELEM_INFO_ARRAY(1, 1003, 1), SDO_ORDINATE_ARR
AY(-74.000009, 40, -74.000005, 40, -74.000005, 40.000009, -74.000009, 40.000009,
 -74.000009, 40))

I haven't touched the warning message problem or optimization issues yet.

Ivan.

comment:11 Changed 9 years ago by msmitherdc

Ivan,

Could you also add support for setting the SRID? See ticket #3622 for example

comment:12 Changed 9 years ago by ilucena

The option -lco SRID works when the layer is "new":

% ogr2ogr -f OCI oci:scott/tiger@orcl test.shp -nln test4 -lco DIM=*2* -lco SRID=*8307*

SQL> select ora_geometry from test4;

ORA_GEOMETRY(SDO_GTYPE, SDO_SRID, SDO_POINT(X, Y, Z), SDO_ELEM_INFO, SDO_ORDINAT
--------------------------------------------------------------------------------
SDO_GEOMETRY(*2003*, *8307*, NULL, SDO_ELEM_INFO_ARRAY(1, 1003, 1), SDO_ORDINATE_ARR
AY(-74, 40, -73.999995, 40, -73.999995, 40.000009, -74, 40.000009, -74, 40))

The options -append and -update works well and it is not necessary to inform -lco DIM or -lco SRID. The OCI driver will use the ones entered previously:

% ogr2ogr -append -update -f OCI oci:scott/tiger@orcl -nln test4 test2.shp

SQL> select ora_geometry from test4;
...
SDO_GEOMETRY(2003, 8307, NULL, SDO_ELEM_INFO_ARRAY(1, 1003, 1), SDO_ORDINATE_ARR
AY(-73.999995, 40, -73.999991, 40, -73.999991, 40.000009, -73.999995, 40.000009,
 -73.999995, 40))

That looks consistent. Now, if I try to inform -lco DIM or -lco SRID that is what I got:

% ogr2ogr -append -update -f OCI oci:scott/tiger@orcl -nln test4 test2.shp -lco SRID=8307

WARNING: Layer creation options ignored since an existing layer is
         being appended to.

What also seems consistent.

Should we call that bug fixed?

comment:13 Changed 9 years ago by msmitherdc

The dimensionality determination does seem to work ok but the SRID code only works for certain types of data. I was trying from KMZ using the libkml driver and the SRID does not get populated properly. It would be better if the append code read the srid from the user_sdo_geom_metadata table to determine the SRID.


u4rt9mds@maps:~/programming/MapServer/gdal_trunk$ ~/programming/MapServer/gdal_trunk/apps/ogr2ogr -append -update -f OCI oci:user/pass@tns:marine_test2 /htdocs/dhs/marine/MarineTraffic.kmz -nln marine_test2 -lco dim=2 -lco srid=8307 WARNING: Layer creation options ignored since an existing layer is

being appended to.

ERROR 1: ORA-29875: failed in the execution of the ODCIINDEXINSERT routine ORA-13365: layer SRID does not match geometry SRID ORA-06512: at "MDSYS.SDO_INDEX_METHOD_10I", line 709 ORA-06512: at "MDSYS.SDO_INDEX_METHOD_10I", line 225

in INSERT INTO marine_test2 (ORA_GEOMETRY, OGR_FID, "Name", "description", "tessellate", "extrude", "visibility") VALUES (MDSYS.SDO_GEOMETRY(2001,NULL,MDSYS.SDO_POINT_TYPE(117.7996,38.97557,0),NULL,NULL), 1, 'JIN YUAN 8', '<b>Speed/Course:</b>&nbsp;9.1 knots / 131&deg;<br/><b>Flag:</b>&nbsp;KH&nbsp;<img src="http://www.marinetraffic.com/ais/flags/KH.gif"/><br/><br/><a href="http://www.marinetraffic.com/ais/shipdetails.aspx?mmsi=514465000"><img src="http://photos.marinetraffic.com/ais/showphoto.aspx?size=thumb&mmsi=514465000"/><br/>Vessels details on MarineTraffic?.com<br/></a><a href="http://www.marinetraffic.com/ais/gettrackkml.aspx?mmsi=514465000">Show Vessels Track</a>', -1, -1, -1)

comment:14 Changed 9 years ago by ilucena

Mike?

It is still working fine for me. I can see the warning and then the features are loaded with the same SRID of the pre loaded ones.

How did your initial ogr2ogr went, what was -lco options, What was the SRID?

Can I have access to your input file?

Thanks a lot.

Ivan

comment:15 Changed 9 years ago by msmitherdc

Original load:

/ogr2ogr -f OCI oci:user/pass@tns:tmp /htdocs/dhs/marine/MarineTraffic.kmz -nln marine_test2 -lco dim=2 -lco srid=8307

Source File:

curl -o MarineTraffic?.kmz http://marinetraffic2.aegean.gr/ais/getkml.aspx

comment:16 Changed 9 years ago by warmerdam

I haven't taken the time to understand the oracle specific issues, but I would like to stress that layer creation options are only for layer creation, and we should not expect them to have any effect with ogr2ogr when just appending to an existing layer.

If the OCI driver cannot determine important information like the dimension for an existing layer, then the alternatives are:

  • Having some similar config options to the layer creation options that could be specified on the ogr2ogr commandline using --config
  • or it might just not be possible to append to an existing table with good control. I can't really think of other good options.

But I do hope all possibilities for detecting reasonable settings would be used. This may not have been well done in the past.

comment:17 Changed 9 years ago by ilucena

The code changes suggested previously on that ticket, that have been committed to the trunk, are working fine on my system so I am trying to add KMZ support to try to replicate the same error. No luck so far.

comment:18 Changed 9 years ago by msmitherdc

I am using trunk and using the libkml driver to read KMZ. Oracle version is 10.2.0.4 with instant client 11.1.0.7 if that helps. The dim checks work fine. Its the srid checks that don't work for me.

comment:19 Changed 9 years ago by ilucena

I tried to run the same test on a Windows system with Oracle 10g but I wasn't able to use libkmz. How did you compile it? Can you send the makefile.vc?

But I went back to Linux and then I was able to replicated the same problem with the same data but in a Oracle 11g server.

% ogr2ogr -append -update -f oci oci:scott/tiger@orcl:marine_test1 MarineTraffic.kmz -nln marine_test1 -lco dim=2 -lco srid=8307
WARNING: Layer creation options ignored since an existing layer is
         being appended to.
ERROR 1: ORA-29875: failed in the execution of the ODCIINDEXINSERT routine
ORA-13365: layer SRID does not match geometry SRID
ORA-06512: at "MDSYS.SDO_INDEX_METHOD_10I", line 720
ORA-06512: at "MDSYS.SDO_INDEX_METHOD_10I", line 225
 in INSERT INTO marine_test1 (ORA_GEOMETRY, OGR_FID, "Name", "description", "tessellate", "extrude", "visibility") VALUES (MDSYS.SDO_GEOMETRY(2001,NULL,MDSYS.SDO_POINT_TYPE(4.292449,51.34238,0),NULL,NULL), 1, 'TUG 21', '<b>Speed/Course:</b>&nbsp;0 knots / 126&deg;<br/><b>Flag:</b>&nbsp;BE&nbsp;<img src="http://www.marinetraffic.com/ais/flags/BE.gif"/><br/><br/><a href="http://www.marinetraffic.com/ais/shipdetails.aspx?mmsi=205360190"><img src="http://photos.marinetraffic.com/ais/showphoto.aspx?size=thumb&mmsi=205360190"/><br/>Vessel''s details on MarineTraffic.com<br/></a><a href="http://www.marinetraffic.com/ais/gettrackkml.aspx?mmsi=205360190">Show Vessel''s Track</a>', -1, -1, -1)
ERROR 1: Terminating translation prematurely after failed
translation of layer 0 (use -skipfailures to skip errors)

But I noticed that if I don't inform the layer name on the identification string the problem doesn't show up and the data is included perfectly.

% ogr2ogr -append -update -f oci oci:scott/tiger@orcl MarineTraffic.kmz -nln marine_test1 -lco dim=2 -lco srid=8307
WARNING: Layer creation options ignored since an existing layer is
         being appended to.

We still have a problem to fix on the code but could that be a work around for you?

comment:20 Changed 9 years ago by msmitherdc

I haven't been able to compile libkml on windows yet. I'm not sure if the options have been added to the makefile.vc yet.

Hmm. I'll give it a try without the layer name. However, in the past when doing that, it has to run through all the spatial layers we have in the database and it really increases the time to go through 1000 layers.

comment:21 Changed 9 years ago by msmitherdc

Due to the number of layers that we have in our database, it fails. I get a series of

OCI: get dim based of existing data failed. OCI: Prepare(SELECT SRID FROM ALL_SDO_GEOM_METADATA WHERE TABLE_NAME = 'AQUIFRP025' AND COLUMN_NAME = 'SHAPE' AND OWNER = 'NAT_ATLAS') OCI: Prepare(SELECT a.SHAPE.GET_DIMS() DIM FROM SIP_USN_PARKING_A a WHERE ROWNUM = 1) ERROR 1: ORA-00942: table or view does not exist

in SELECT a.SHAPE.GET_DIMS() DIM FROM SIP_USN_PARKING_A a WHERE ROWNUM = 1

ERROR 1: ORA-24374: define not done before fetch or execute and fetch

in OCIStmtFetch

OCI: get dim based of existing data failed. OCI: Prepare(SELECT SRID FROM ALL_SDO_GEOM_METADATA WHERE TABLE_NAME = 'SIP_USN_PARKING_A' AND COLUMN_NAME = 'SHAPE' AND OWNER = 'DISDI') OCI: Prepare(SELECT a.SHAPE.GET_DIMS() DIM FROM MC a WHERE ROWNUM = 1) OCI: Prepare(SELECT SRID FROM ALL_SDO_GEOM_METADATA WHERE TABLE_NAME = 'MC' AND COLUMN_NAME = 'SHAPE') OCI: Prepare(SELECT a.SHAPE.GET_DIMS() DIM FROM RH a WHERE ROWNUM = 1) OCI: Prepare(SELECT SRID FROM ALL_SDO_GEOM_METADATA WHERE TABLE_NAME = 'RH' AND COLUMN_NAME = 'SHAPE') Aborted

And then it aborts and no data gets loaded. So looks like this isn't a workaround I can use.

comment:22 Changed 9 years ago by ilucena

Yes,

It takes forever to run if you don't inform table name, so I have another work around, actually it might be the solution. Try to use the table name in uppercase:

% ogr2ogr -append -update -f oci oci:scott/tiger@orcl:MARINE_TEST1 MarineTraffic?.kmz -nln marine_test1 -lco dim=2 -lco srid=8307

If that solves your problem then you can grab and build the code from GDAL trunk with the changes I just made: http://trac.osgeo.org/gdal/changeset/19871/trunk

Then, please test again the same command with table name in lowercase, just to make sure that it is fixed and we are not going to have that problem again.

comment:23 Changed 9 years ago by msmitherdc

Ivan, That worked well for the SRID, it properly picks it up. But the DIM is incorrect. The code examines the first row to check layer dim. Instead, it should the ALL_SDO_INDEX_METADATA and ALL_SDO_INDEX_INFO like this

select m.sdo_index_dims from all_sdo_index_metadata m, all_sdo_index_info i where i.index_name = m.sdo_index_name

and i.sdo_index_owner = m.sdo_index_owner and i.sdo_index_owner = :OWNER_NAME and i.table_name = :TABLE_NAME

Then I think it would all work fine.

comment:24 Changed 9 years ago by ilucena

You are right, the current code get the dimension from the function GET_DIMS applied to the geometry column and as long as the data was recorded consistently, the first row should be equal to all the rest. Right?

SELECT a.<geometry column>.GET_DIMS() FROM <table> a WHERE ROWNUM = 1;

Is ti possible that after all those attempts, your table has inconsistent geometry objects with different dimensions?

comment:25 Changed 9 years ago by ilucena

Resolution: fixed
Status: assignedclosed

I have confirmed that once that changes are applied to OGR/OCI the process of loading the first layer or appending new layers should keep the same dimension as it was initially defined. In that case, the layers type should be consistent with the index and metadata.

comment:26 Changed 9 years ago by msmitherdc

Resolution: fixed
Status: closedreopened

Dimension check only works if there is at least one row. If table is empty, dimension check doesn't work. It would be better to check dimensionality with the SQL I posted.

I want to use this to reload data on a periodic basis so the table will be empty before i reload.

comment:27 Changed 9 years ago by ilucena

msmitherdc, Thanks for your clarification. Give it a try to the current revision r19894 and post your comments so we can close not only based on the initial post but also based on the subsequent additions.

Note that at that point the driver doesn't know who is the owner of the selected table so I needed to simplify your statement. That could be a potential problem if different schemes use same table name, but at least I kept the ROWNUM based query to be safe.

If the MULTI_LOAD questions (3 years ago) is still a concern, I suggest you to file another ticket so that those major changes could be handled properly.

comment:28 Changed 9 years ago by msmitherdc

Resolution: fixed
Status: reopenedclosed

Revision works well. Thanks

Note: See TracTickets for help on using tickets.