Opened 10 years ago

Closed 5 years ago

#5582 closed enhancement (wontfix)

SpatialView - writable

Reported by: mj10777 Owned by: Even Rouault
Priority: normal Milestone: closed_because_of_github_migration
Component: default Version: svn-trunk
Severity: normal Keywords: ogr sqlite view writable
Cc:

Description

Adapted ogrsqliteviewlayer.cpp

  • to read view triggers: UPDATE,INSERT and DELETE
  • when true is writable for those functions
  • the following functions were taken from ogrsqlitetablelayer.cpp with slite changes: SetFeature,DeleteFeature,CreateFeature changes: will return FALSE if bTrigger* is FALSE BindValues (in both view/tablelayer.cpp): extra 'T' in case OFTDate was removed ClearInsertStmt: no changes

Adapted ogrsqlitedatasource.cpp:

view sql-query will retrieve srid of the view OpenView: adapted to receive srid

the svn diff is based of the version of this morning (Revision 27538)

this version seems to be faulty I always receive a:

glibc detected * ogrinfo: malloc(): memory corruption:

this does not happen when ogrinfo is called with valgrind it happens with every gdal command, QGIS crashes when valgrind is not used

This does not happen with (Revision 27304) Compiling QGIS with changes made in Revision 27304

update of SpatialView works correctly

delete and insert not tested

test_ogrsd shows no useful results with an UPDATE command

test_ogrsd shows 1 error with a SELECT command

Attachments (6)

20140721.27538.diff_spatial_view.txt (35.6 KB ) - added by mj10777 10 years ago.
svn diff on revision 27538
20140721.test_ogrsd.txt (1.6 KB ) - added by mj10777 10 years ago.
Output result with given command from test_ogrsd
spatialite_views_writable.py (16.7 KB ) - added by mj10777 9 years ago.
python function to add to ogr_sqlite.py testing SpatialView - writable
spatialite_views_writable.sql (7.5 KB ) - added by mj10777 9 years ago.
corresponding sql scrip for the same task as spatialite_views_writable.py
20150208.ogrsf_frmts_sqlite_datasource.diff (355.2 KB ) - added by mj10777 9 years ago.
rename of OSGF_ prefix to OSVLT_. Removed commented out debug messages.
20151031.trunk_sqlite_vector.diff (398.8 KB ) - added by mj10777 8 years ago.
diff based on trunk of 2015-10-31

Download all attachments as: .zip

Change History (48)

by mj10777, 10 years ago

svn diff on revision 27538

by mj10777, 10 years ago

Attachment: 20140721.test_ogrsd.txt added

Output result with given command from test_ogrsd

comment:1 by Even Rouault, 10 years ago

Perhaps a make clean should fix the segfault (GDAL makefiles don't detect header changes and necessary .cpp recompilation)

My main remark with this patch is that it copies functions from OGRSQLiteTableLayer instead of trying to share code with it. Perhaps there should be a OGRSQLiteUpdatableLayer deriving from OGRSQLiteLayer that would host the common methods, and OGRSQLiteTableLayer and OGRSQLiteViewLayer would inherit from it.

autotest/ogr/ogr_sqlite.py should be updated to test the new functionnality. And the documentation of the driver should also reflect it.

comment:2 by mj10777, 10 years ago

The Revision 27538 was a fresh svn checkout

so there was nothing to 'clean'

comment:3 by Even Rouault, 10 years ago

Hum, strange then. Doesn't happen on my environment or on the continuous integration builds (https://travis-ci.org/rouault/gdal_coverage/builds). You mention that Valgrind helps command running, but doesn't it emit errors ? And if you attach a debugger, maybe something interesting will show up. If you don't manage to identify what's going wrong with that, you could still "bisect" down to r27304 to identify the problematic revision.

comment:4 by mj10777, 10 years ago

Yes, Valgrind runs correctly without error. No quite sure how to run the debugger, always found that a bit complicated

comment:5 by Even Rouault, 10 years ago

You should invest some time learning to use a debugger. Will make you save a lot of time ! Install ddd, and then "ddd --args ogrinfo the_arguments"

comment:6 by mj10777, 10 years ago

ddd --args ogrinfo the_arguments

After recompiling with '--enable-debug'

  • no errors occur

comment:7 by mj10777, 10 years ago

After updating to Ubuntu 14.04 and recompiling everything

  • ogrinfo for 'SELECT' and 'UPDATE' run without error

comment:8 by mj10777, 10 years ago

I have again had these memory corruptions.

use of ddd or gdb does not work for me

Despite '--enable-debug' I receive 'no debugging symbols found'.

However running valgrind I noticed that it was showing errors for

GDALRegister_GRASS (in /usr/local/lib/gdalplugins/gdal_GRASS.so)

Since I have not used grass with gdal for many years, I removed this and everything worked correctly.

in reply to:  8 comment:9 by Even Rouault, 10 years ago

Replying to mj10777:

I have again had these memory corruptions.

use of ddd or gdb does not work for me

Despite '--enable-debug' I receive 'no debugging symbols found'.

However running valgrind I noticed that it was showing errors for

GDALRegister_GRASS (in /usr/local/lib/gdalplugins/gdal_GRASS.so)

Since I have not used grass with gdal for many years, I removed this and everything worked correctly.

That might be it indeed. Plugins depend on the ABI of the GDAL headers they are compiled against. So a plugin compiled with GDAL version X is likely to not work with version Y. However there's a macro in the plugin code to detect that situation normally, but if your plugin is very old, that mechanism might not exist at that time.

comment:10 by mj10777, 10 years ago

OGRSQLiteUpdatableLayer

This may be a good idea, for the above reason, but also it may be wise to add a 'Raster2Layer' which could gather the Raster2-specfic Information, transforming the different information to gdal-types.

table 'raster_coverages' [which has itself has no geometries, but has the extent values] contains all major information that would be needed for a Rasterlite2- Layer.

the RasterLite2 driver could then retrieve this layer and retrieve the information it needs:

extent bands compression tile_width/height

and much more, so that functions like GetBlockParams() would not be needed.

This may simplify

comment:11 by mj10777, 9 years ago

Will the needed changes to make SpatialViews incoperated into the next version?

in reply to:  11 comment:12 by Even Rouault, 9 years ago

Replying to mj10777:

Will the needed changes to make SpatialViews incoperated into the next version?

Likely not, unless you invest time to take into account my remarks

comment:13 by mj10777, 9 years ago

You mean this?: Perhaps there should be a OGRSQLiteUpdatableLayer deriving from OGRSQLiteLayer that would host the common methods, and OGRSQLiteTableLayer and OGRSQLiteViewLayer would inherit from it.

I had assumed that that was a general thought for someone who more knowledgeable OGR classes. If that is it, I will look into this.

in reply to:  13 comment:14 by Even Rouault, 9 years ago

Replying to mj10777:

You mean this?: Perhaps there should be a OGRSQLiteUpdatableLayer deriving from OGRSQLiteLayer that would host the common methods, and OGRSQLiteTableLayer and OGRSQLiteViewLayer would inherit from it.

Yes, and the part with adding tests and updating doc too.

comment:15 by Even Rouault, 9 years ago

Ah, just wanted to mention that while working on a new RFC, I've also started a work on the SQlite driver that involves quite a lot of refactoring (#5494), so you'd better wait that this has landed into trunk before trying to update your writable view support work.

comment:16 by mj10777, 9 years ago

It may be wise to split the queries in OGRSQLiteDataSource::Open based on pre Spatialite 4.0 or not This is now being indirectly done through the setting of bSpatialite4Layout

Starting with Spatialite 4.0 and 'vector_layer' exists So a simple check if 'vector_layer' exists would set bSpatialite4Layout

if (!bSpatialite4Layout)
{ // all the pre 4.0 datatypes field-names and different queries
 // for SpatialViews the present query must be replaced with this to support 2.4-3.x Databases properly[[BR]]
 // SELECT view_name, view_geometry, view_rowid, views_geometry_columns.f_table_name, views_geometry_columns.f_geometry_column,srid,spatial_index_enabled FROM views_geometry_columns  INNER JOIN geometry_columns ON (views_geometry_columns.f_table_name=geometry_columns.f_table_name AND views_geometry_columns.f_geometry_column=geometry_columns.f_geometry_column)
}
else
{ // starting with 4.0 use of 'vector_layers' distinguishes between 'SpatialTable', 'SpatialView' and 'VirtualShape' and in the future maybe others[[BR]]
 // SELECT table_name,geometry_column,geometry_type,coord_dimension,srid,spatial_index_enabled FROM vector_layers WHERE layer_type='SpatialTable'[[BR]]
 // SELECT table_name,geometry_column,geometry_type,coord_dimension,srid,spatial_index_enabled FROM vector_layers WHERE layer_type='VirtualShape'
 // SELECT view_name, view_geometry, view_rowid, f_table_name, f_geometry_column,srid,spatial_index_enabled FROM views_geometry_columns INNER JOIN vector_layers ON (view_name=table_name AND f_geometry_column=geometry_column)[[BR]]

}

The OpenView function should be adapted to recieve the same parameters as OpenTable.
I would suggest this be done now during the present refactoring.

After which then:

class OGRSQLiteVectorLayer : public OGRSQLiteLayer

  • the new base class

class OGRSQLiteTableLayer : public OGRSQLiteVectorLayer

class OGRSQLiteViewLayer : public OGRSQLiteVectorLayer

And later class OGRSQLiteRasterlite2Layer : public OGRSQLiteVectorLayer

  • for Rasterlite2 specific tasks

Mark

comment:17 by mj10777, 9 years ago

I have downloaded the lastest source and the autotest areas and have run the ogr_sqlite.py

  • it brings 3 skipped which I assume is correct

I will now work on a sql-script that will create a simple table with 2 views

  • one with insert, update and delete and one only with a insert trigger

Then combinations of insert, update and delete will be attempted and the results tested.

That logic will then be used to create a python function based on 'ogr_spatialite_8'

comment:18 by Even Rouault, 9 years ago

I've committed in r28386 an updated version of the SQLite driver that adds support for RFC41 multiple geometry columns. This should be a more stable basis if you want to update your work.

comment:19 by mj10777, 9 years ago

After the latest update, I am getting undefined reference to:

`VFKFeature::VFKFeature(IVFKDataBlock*, long)' `OGRMemLayer::DeleteFeature(long)' `OGRCouchDBTableLayer::GetFeature(long)' `OGRMemLayer::GetFeature(long)' `OGRMemLayer::SetNextByIndex(long)' `OGRXPlaneLayer::GetFeature(long)' `OGRGFTLayer::SetNextByIndex(long)' `OGRCouchDBTableLayer::DeleteFeature(long)' `OGRCouchDBLayer::SetNextByIndex(long)' `GMLFeatureClass::SetFeatureCount(int)' `OGRSQLiteLayer::GetFeature(long)' `OGRLayer::DeleteFeature(long)' `OGRLayer::SetNextByIndex(long)' `OGRLayer::GetFeature(long)' `OGRXPlaneLayer::SetNextByIndex(long)'

comment:20 by mj10777, 9 years ago

sorry, my fault - forgot 'make clean'

comment:21 by mj10777, 9 years ago

I am nearing a point where I can offer a 'OGRSQLiteVectorLayer' class that OGRSQLiteTableLayer and OGRSQLiteViewLayer inherit.

This was based on the r28386 version and OGRSQLiteViewLayer adapted to work as OGRSQLiteTableLayer did.

Both orginfo on some of my test databases and ogr_sqlite.py bring the same results.

No additions to make OGRSQLiteViewLayer writable have been done yet, since I thought that getting the present version adapted into an acceptable form should be done first.

There are 2 things yet to be done, which I will start tomorrow.

Also I assume you will be busy getting the next version completed, which if I remember correctly was planned for tomorrow.

So my question would be: when would you have time to look at this. Monday?

in reply to:  21 comment:22 by Even Rouault, 9 years ago

Replying to mj10777:

I am nearing a point where I can offer a 'OGRSQLiteVectorLayer' class that OGRSQLiteTableLayer and OGRSQLiteViewLayer inherit.

Perhaps OGRSQLiteEditableLayer would be a better name than OGRSQLiteVectorLayer ?

So my question would be: when would you have time to look at this. Monday?

Presumably, but don't depend on my schedule ;-)

comment:23 by mj10777, 9 years ago

The reason for the name, is because is misread your first note: the common logic of both are in OGRSQLiteVectorLayer and not only the editing portion.

I have build in VIEW writable logic and recompiled QGIS and it works properly.

QGIS, it seems, must be recompile after any change - which is a pain.

While doing so, I ran into another error, which I believe has nothing to do with my changes:

When opening a QGIS project that had a misplaced mbtiles file, the following error crashed QGIS

ERROR 4: `../wrong directory/1861_Mercator_Countries.mbtiles' does not exist in the file system,
and is not recognised as a supported dataset name.

ERROR 10: Pointer 'hDS' is NULL in 'OGR_DS_Destroy'.

The ERROR 4 comes from gcore/gdaldataset.cpp, but I could not find where ERROR 10 came from.

Tomorrow I will start working on the python test.

I am creating documentation in drv_sqlite.html on what is changing, but I am not sure that is what you want since it seems to be more of general information about the sqlite driver.

in reply to:  23 comment:24 by Even Rouault, 9 years ago

Replying to mj10777:

The reason for the name, is because is misread your first note: the common logic of both are in OGRSQLiteVectorLayer and not only the editing portion.

Yes the intent is to maximize code sharing. I'm not particularly good at finding names, so pick up one...

I have build in VIEW writable logic and recompiled QGIS and it works properly.

QGIS, it seems, must be recompile after any change - which is a pain.

Weird. You're only changing internal classes of GDAL. I've never had to recompile QGIS for that.

While doing so, I ran into another error, which I believe has nothing to do with my changes:

When opening a QGIS project that had a misplaced mbtiles file, the following error crashed QGIS

ERROR 4: `../wrong directory/1861_Mercator_Countries.mbtiles' does not exist in the file system,
and is not recognised as a supported dataset name.

ERROR 10: Pointer 'hDS' is NULL in 'OGR_DS_Destroy'.

The ERROR 4 comes from gcore/gdaldataset.cpp, but I could not find where ERROR 10 came from.

Hum. As far as I can see, this issue existed in older GDAL releases as well. Anyway should be fixed by r28426 "OGR_DS_Destroy(): silently return if passed a NULL pointer"

Tomorrow I will start working on the python test.

I am creating documentation in drv_sqlite.html on what is changing, but I am not sure that is what you want since it seems to be more of general information about the sqlite driver.

The doc in drv_sqlite.html is only for user visible behaviour, i.e. mentionning that updatables views can be edited with OGR API in GDAL 2.0

by mj10777, 9 years ago

python function to add to ogr_sqlite.py testing SpatialView - writable

by mj10777, 9 years ago

corresponding sql scrip for the same task as spatialite_views_writable.py

comment:25 by mj10777, 9 years ago

QGIS: I was using a version of gdal from last August. Then QGIS brought up a list of missing files and did not crash.

I have prepared a html table for drv_sqlite.html titled:

Spatialite specific Version Information

Which can serve as a reference table.

There is also basic information how the SpatialView work.

So I will place it with a Chapter title:

Writable SpatialViews with SpatiaLite library (Spatial extension for SQLite)

(Starting with in GDAL OGR API 2.0)

I will probably upload the diff tomorrow around 12 (CET).

I have added the python function as an attachment which should perform all needed tests in ogr_sqlite.py.

ogr_sqlite.py now runs (plus that function) without error as before with my version - will, however fail, with the present version.

comment:26 by mj10777, 9 years ago

A new class called 'OGRSQLiteVectorLayer', derived from 'OGRSQLiteLayer', has been created.

It replaces the functionality previous done in 'OGRSQLiteTableLayer' and 'OGRSQLiteViewLayer'.

These classes have, however, been retain in case of new needs in the future.

Both classes derive from 'OGRSQLiteVectorLayer'.

In 'OGRSQLiteVectorLayer' there is now a static function called 'GetSQLiteLayerType' that can determine the how the Sqlite-Container is being used.

Mainly 'OGRSpatialTable' and 'SpatialTable' (for Spatialite), but also with which Spatialite version the database has been created with.

This is read at the start of 'OGRSQLiteDataSource::Open' and stored in 'OGRSQLiteDataSource' as a OGRSQLiteVectorLayerType.

After this point the needed sql is created based on the found value of the set OGRSQLiteVectorLayerType.

For the collection of TABLES, there is only one loop for 'OGRSpatialTable' and 'SpatialTable'.

Note: the result of this loop, runs 2 times for reasons I do not understand. I have added commented out code which I believe could replace the second loop.

When a 'OGRSQLiteTableLayer' is created, it will receive the set OGRSQLiteVectorLayerType.

For 'SpatialView' the OpenView logic as been adapted to work in the same way as 'OpenTable', with the exception of receiving an OGRSQLiteVectorLayerType value of OSGF_SpatialTable_4 a OSGF_SpatialView_4.

The OGRSQLiteVectorLayer::Initialize with then know what type of layer it is.

All sql will be created based on these values.

Once the 'bIsVirtualShape' and the new 'bIsVirtualXL' have been set, the OGRSQLiteVectorLayerType values it set to the corresponding OSGF_SpatialView_2/3/4.

3 major functions have been added in 'OGRSQLiteVectorLayer': IsTableLayer(),IsViewLayer(), GetGeometryTable()

For areas where only Table or View specific queries are needed (like TestCapability()) this is used.

For queries such as in 'EstablishFeatureDefn()' where for views the UnterliningTable must be queried, GetGeometryTable() will return for views pszEscapedUnderlyingTableName and for Tables pszEscapedTableName.

Although it was not my intention that everything in both classes be replaced by 'OGRSQLiteVectorLayer', everything seem to been have been resolved.

When I restart RasterLite2, I will use the same logic (derive from 'OGRSQLiteVectorLayer'), but most of the tasks needed (dealing with the admin-tables for the Raster portions) in the OGRRasterLite2 - but the retrieval of the information still be done by 'OGRSQLiteVectorLayer', avoiding the need of recoding.

At least I hope it will work this way. We shall see.

comment:27 by Even Rouault, 9 years ago

The patch does not include ogrsqlitevectorlayer.cpp. I'm a bit frightened to see that there's hardly any code left in OGRSQLiteTableLayer and OGRSQLiteViewLayer, which must imply a lot if if() else() logic in the new class...

Regarding the values in the OGRSQLiteVectorLayerType enumeration, the OSGF_ prefix is inappropriate. It means Ogr Sqlite Geometry Format. So in that case it should rather be OSVLT_

comment:28 by mj10777, 9 years ago

Yes, did not think of that - was not added to svn.

should have noticed that.

if() else() logic

no very little, more like switch's when creating the needed sql strings.

I have to leave now so I cannot make a quick change of OSGF_ to OSVLT_

by mj10777, 9 years ago

rename of OSGF_ prefix to OSVLT_. Removed commented out debug messages.

comment:29 by mj10777, 9 years ago

Thoughts about the next step:

In August I was doing work on a RasterLite2 Driver, mainly based on the way 'RasterLite' (which I will call RasterLite1). (https://trac.osgeo.org/gdal/ticket/5603)

In the mean time, much has changed in RasterLite2 which must be taken into consideration.

Also, I assume, that with 2.0 certin ways of doing things have changed.

RasterLite2 used the ogr class (i.e. the file was opened) and one of the Spatial-Admin tables were created as a Vector-Layer.

This was used the by the Raster logic to access the Raster-Data.

So my idea at the moment is to create a OGRRasterLite2Layer derived from OGRSQLiteVectorLayer.

As apposed to the SpatialTable and SpatialView, which at the moment are contain inside OGRSQLiteVectorLayer, the goal of OGRRasterLite2Layer would be to combine the 4 Admin Tables (2 with Geometries, 2 without) as a READ-ONLY source for the Raster-logic to access.

Any changes of these table is a task of Rasterlite2, done through sql.

The 'raster_coverages.coverage_name' would then be the ogr-layername Sample: 'berlin_stadtteilgrenzen.1880'

  • the 4 Admin table would then be as above PLUS the suffix: _layers,_sections, _tiles,_tile_data

Due to the different logic needed, the OGRSQLiteVectorLayer::Initialize would NOT be called (as done now with OGRSQLiteTableLayer and OGRSQLiteViewLayer)

Also EstablishFeatureDefn would be different and being READ-ONLY everything that has to do with writeing would also not be called in OGRSQLiteVectorLayer

But things like GetLayerDefn(),GetNextFeature(),GetMetadata(),GetName() etc. would.

Two different tasks, two classes - the common logic being served by OGRSQLiteVectorLayer, otherwise by OGRRasterLite2Layer.

If this works correctly, then in the end the present classes OGRSQLiteTableLayer and OGRSQLiteViewLayer could be removed, with OGRSQLiteVectorLayer beinging used as the default for Tables/Views and derived classes for exceptions to the default.

As OGRRasterLite1Layer could the be create to do the same (it has 2 Admin-Tables).

MBTiles, which does not use the sqlite-driver at present (there was a report that there was an open-file problem existed), but also as an admin table (metadata) with an extent that could be emulated and would also not be read-only.

GeoPackage, which also does not use the sqlite-driver, is so complicated that it should remain extra.

It was this in mind that I added the checks for these 2.

For MbTiles for possible support in the future.

For GeoPackage and a sign to backout and leave it to the OGRGeoPackageDriver class.

comment:30 by Even Rouault, 9 years ago

I've spent the whole morning reviewing your patch and believe the following points should be addressed :

  • The OGRSQLiteVectorLayerType enumeration seems to serve 2 different purposes : identifying database type (regular OGR, Spatialite 2, Spatialite 3, Spatialite 4) and layer types (table, view, virtualshape, virtual xl and others that are provision for the future rasterlite, geopackage, mbtiles). So I think there should be 2 enumerations rather than just one. The database type should be a member of the OGRSQLiteDataSource class. The layer type a member of OGRSQLiteVectorLayer class. And there's redundancy between the enumerated values of VirtualShape and VirtualXL with the existing boolean value bIsVirtualShape and the new one bIsVirtualXL that you've added and that isn't actually used. I'm not even sure we need a vector layer type distinction. Just having a Table and View classes, and the bIsVirtualShape should be enough perhaps.
  • OSVLT_SQLiteLayer should be renamed to OSVLT_Unknown for less confusion (well you would probably need 2 unknown enum values if the enumeration is split in 2)
  • With the help of virtual methods, I guess that all the view specific members (osUnderlyingTableName, osUnderlyingGeometryColumn, bTriggerInsert, bTriggerUpdate, bTriggerDelete) could be moved back to OGRSQLiteViewLayer
  • bHasCheckedSpatialIndexTable that was in OGRSQLiteViewLayer previously and you've moved into OGRSQLiteVectorLayer is probably no longer needed now.
  • OGRSQLiteVectorLayer::GetGeometryTable(): should be virtual have separate implementation in the Table and View classes
  • OGRSQLiteDataSource::OpenVirtualTable(), OpenTable() and OpenView() probably don't need the "OGRSQLiteVectorLayerType eVectorLayerTypeXXXX" argument if there's a datasource member that identifies the database type
  • In ExecuteSQL(), the """OpenVirtualTable(papszTokens[3],OSVLT_SpatialTable_4, pszSQLCommand);""" snippet is weird. Why OSVLT_SpatialTable_4 ? This could be any version
  • Simarly in ICreateLayer(), """poLayer->Initialize( pszLayerName,OSVLT_SpatialTable_4, TRUE ) """ is suspicious. What about if you use spatialite 3 ?
  • OGRSQLiteVectorLayer::CreateSpatialIndexIfNecessary() should have stayed in OGRSQLiteTableLayer only (see the if (IsTableLayer()) test )
  • OGRSQLiteVectorLayer::Initialize(): linked to what I told this is confusing since the passed parameter OGRSQLiteVectorLayerType eVectorLayerType is actually a database type, and the OGRSQLiteVectorLayer member variable eVectorLayerType has the same name, but is affected other values.
  • OGRSQLiteVectorLayer::GetSQLiteLayerType(): should really be splitted in 2 different functions for less confusion. The beginning of this function really identifies the database type, and should be a method of OGRSQLiteDataSource (and not take any argument). The end of this function identifies a layer type with a database type as input and the database type. It could remain a method of OGRSQLiteVectorLayer.
  • int OGRSQLiteVectorLayer::IsTableLayer() shouldn't exist. The OGRSQLiteLayer::IsTableLayer() { return FALSE } implementation is enough. And OGRSQLiteTableLayer::IsTableLayer() should return TRUE
  • int OGRSQLiteVectorLayer::IsViewLayer(). Not sure if it is really needed if correct dispatching of view specific work is done with virtual methods. If it is needed, should rather be a virtual method that returns FALSE in OGRSQLiteVectorLayer, and TRUE in OGRSQLiteViewLayer (no need to test the eVectorLayerType)
  • OGRSQLiteVectorLayer::GetGeometryTable() should be pure virtual. And have an implementation in OGRSQLiteTableLayer and OGRSQLiteViewLayer
  • OGRSQLiteVectorLayer::SetCreationParameters() should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::EstablishFeatureDefn(): part of the if (IsViewLayer()) or if (IsViewLayer()) work could be done by moving those part of the code in a OGRSQLiteTableLayer::EstablishFeatureDefn() and OGRSQLiteViewLayer::EstablishFeatureDefn() that would call OGRSQLiteVectorLayer::EstablishFeatureDefn() for the generic part. For example the detection of the view triggers certainly can be moved in a OGRSQLiteViewLayer::EstablishFeatureDefn()
  • OGRSQLiteVectorLayer::RecomputeOrdinals(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::ResetStatement() : the if (IsViewLayer()) test could be removed if a virtual method GetEscapedRowId() was introduced with 2 different implementations
  • in all methods that do if( bDeferredCreation && RunDeferredCreationIfNecessary() ) tests. That could be done in OGRSQLiteTableLayer only, by redefinition of virtual methods. And the OGRSQLiteTableLayer implementation would call OGRSQLiteVectorLayer::XXXXX() to do the job
  • OGRSQLiteVectorLayer::GetFeature(): same as in ResetStatement(). GetEscapedRowId() could be used
  • OGRSQLiteVectorLayer::TestCapability(): it is obvious here that this method should not exist at the OGRSQLiteVectorLayer level, but have 2 distinct implementations in OGRSQLiteTableLayer and OGRSQLiteViewLayer
  • OGRSQLiteFieldDefnToSQliteFieldDefn(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::FieldDefnToSQliteFieldDefn(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::CreateField(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::CreateGeomField(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::RunAddGeometryColumn(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::InitFieldListForRecrerate(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::AddColumnAncientMethod(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::RecreateTable(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::DeleteField(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::AlterFieldDefn(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::ReorderFields(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::ISetFeature(): the "if ((IsViewLayer()) && (!bTriggerInsert))" test should be done in a OGRSQLiteViewLayer::ISetFeature() that would do that test only and then call OGRSQLiteVectorLayer::ISetFeature()
  • OGRSQLiteVectorLayer::ICreateFeature(): same remark than above
  • OGRSQLiteVectorLayer::DeleteFeature(): same remark than above
  • indentation: you seem to use 1 space whereas the GDAL convention is 4 characters
  • OGRSQLiteVectorLayer::CreateSpatialIndex(): should remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::RunDeferredCreationIfNecessary(): should remain in OGRSQLiteTableLayer
  • not sure about all the statistics related methods (from InitFeatureCount() to SaveStatistics()) but should probably remain in OGRSQLiteTableLayer
  • OGRSQLiteVectorLayer::SetCompressedColumns(): should remain in OGRSQLiteTableLayer

For the sake of easier reviewing, it would help if you could (at least for now) let in ogrsqlitetablelayer.cpp the methods that are moved into OGRSQLiteVectorLayer. That way the diff will be much smaller and less frightening to review. Just things like :

  • OGRErr OGRSQLiteTableLayer::ISetFeature( OGRFeature *poFeature )

+ OGRErr OGRSQLiteVectorLayer::ISetFeature( OGRFeature *poFeature )

Afterwards we can clean that up and move them to the appropriate .cpp file.

comment:31 by mj10777, 9 years ago

Sorry for the delay on this, but I hade to do some other work that was pending.

During this, however, I was able to check the functionality of the present solution - which worked without any problems.

I have just done a update, replacing my present version and will rebuild the solution - going through your list, point by point

  • taking your 'For the sake of easier reviewing' into account.

But before going into your list thoroughly, some observations on how some of this was done the way it was.

1) Once the Base-Class was running, it was hard to get back out to the calling class. And in the end, the difference between Table/View were so slim that everything could be resolved.

This was another reason for the pause, so that a fresh look cound be made to avoid this problem.

2) 'OSVLT_SpatialTable_4, snippet is weird': Yes, and was the cause of adding the type to 'OGRSQLiteDataSource' in the end and is probly not needed.

3) ' eVectorLayerTypeXXXX" argument' : this, I think, is needed, since they are being called in OGRSQLiteDataSource::open where the type has been determined.

Without this parameter, checking would have to be done again in the Initialize() functions to determin, again, what is already As stated in 2), adding the type to 'OGRSQLiteDataSource' came in later due to OGRSQLiteDataSource::GetLayerByName, so I will attemt to resolve this

  • but Initialize() will then need - for tables only - an extra parameter due to 'bIsVirtualShape'

4) 'bIsVirtualXL': having no idea why OGRSQLiteDataSource::open checks for 'VirtualXL', my assumtion being that it was forseen in the past for possible future use

'bIsVirtualShape' is being used once and thus is needed

5) One goal was to place the Sqlite-specific version tasks in one place (OGRSQLiteVectorLayer). Any classes would then reacte to the determined type (mainly sql building).

Up to now OGRSQLiteDataSource::open was doing this version (OGR, Spatialite and in the future mayby something else) specific queries for each layer, which make it diffecult to maintain. Thus I would like to retain the OGRSQLiteVectorLayer::Initialize() as it is. Also OpenView?() would then not be needed, the task being done with OpenTable?()

6) 'EstablishFeatureDefn': I will try to get that working properly with the fresh copy, hoping to avoid the problen stated in 1)

Question about the double loop in OGRSQLiteDataSource::open

for ( iRow = 0; iRow < nRowCount; iRow++ ) cannot the 'aoMapTableToSetOfGeomCols' task be done inside one loop after thecking for NULL?

comment:32 by mj10777, 9 years ago

I have completed a great portion of this list, mainly:

  • moving of functions back to OGRSQLiteTableLayer
  • splitting between OGRSQLiteTableLayer/OGRSQLiteViewLayer and calling OGRSQLiteVectorLayer for the common portions

The enums haven adapted, GetEscapedRowId implemented and GetSQLiteLayerType() split

Tomorrow I will start on the 'let in ogrsqlitetablelayer.cpp the methods that are moved into OGRSQLiteVectorLayer' part.

statistics related methods: see next note.

comment:33 by mj10777, 9 years ago

The statistics methods are outdated

So these Methods should remain in OGRSQLiteVectorLayer

Based on the spatialite Version of the Database

  • reading and writing must be adapted
  • Table layer_statistics is outdated (only pre-4.0* Databases) After 4.0:
  • to read : 'vector_layers_statistics' should be used
  • to write:
  • SpatialTables: 'geometry_columns_statistics'
  • SpatialViews: 'views_geometry_columns'
  • VirtualTables: 'virts_geometry_columns'

comment:34 by mj10777, 9 years ago

After completing your list and getting everything compiled with a correct running ogrinfo, it turned out that that the python test script for creating a SpatialView failed at 'CreateGeomField' because of many of these that were moved back to OGRSQLiteTableLayer.

So I am now trying move back those functions to make a proper SpatialView support possible.

This is partially due to the bit of neglect that SpatialView's have received in the past, but I assum should now be resolved properly.

comment:35 by mj10777, 9 years ago

Getting OGRSQLiteVectorLayer to work based on the suggestions of 2 months ago have not worked.

A view, when writable, is to similar to a table and thus the same logic must be used.

I have keep my first version uptodate with the changes made in the last month and use it consistently in a present project without problems. I can understand the reasons of avoiding bringing the two together as done in the first version of OGRSQLiteVectorLayer.

Maybe an attempt should be made in the direction of deriving the view from a table and skip the OGRSQLiteVectorLayer. In this way maybe it might be easier to take care of only the differences between the two.

Getting a workable writable view for a gdal 2.0 pre-release, would be important in my option, since this capability has existed for sqlite database for quite a long time.

by mj10777, 8 years ago

diff based on trunk of 2015-10-31

comment:36 by mj10777, 8 years ago

I would like to bring up this topic again, since at some point work will be started for support of RasterLite2 after that development will be continued (after spatialite Topology has been completed).

I have been working with the version based of the February diff sent without any problems

  • this is the version you did not like because most of the SpatialTable/View is in OGRSQLiteVectorLayer
    • I still believe that a full writable SpatialView is basically the same as a writable SpatialTable
      • so that it is best to deal with this in the common OGRSQLiteVectorLayer

This morning I have made a fresh git copy of trunk. Created a branch called 'sqlite_vector' from trunk

  • patched my changes, that was created from a diff of the svn version of March and my changes
  • added a ogr_spatialite_views_writable() function to the present ogr_sqlite.py

This function runs without error, but ogr_sqlite.py fails with a segment error

  • but it also fails in the original trunk version at the same point in ogr_spatialite_8
    • just tried this again, but it runs now correctly ...

Since I use SpatialView during the creation of historical (date-based) geometries heavily

  • I would like to get this working in the trunk version

This version also prepares for any future sqlite container that may turn up one day

  • since there is now in a central function that determines what container is being read
    • and if that container is being supported by this class
      • which in the case of mbtiles and GeoPackage it does not
      • and for RasterLite2 it will (and for RasterLite1 does)

comment:37 by Even Rouault, 8 years ago

A number of my comments in https://trac.osgeo.org/gdal/ticket/5582#comment:30 have not been addressed. In particular methods that have been moved in OGRSQliteVectorLayer (by the way I don't find this name very expressive since the base OGRSQLiteLayer is also a vector layer. My initial suggestion was OGRSQLiteUpdatableLayer, or OGRSQLiteWritableLayer), whereas they should have stayed in OGRSQLiteTableLayer : all the ones that deal with field creation, deletion, etc..., table creation. And by using virtual method overriding, you could probably remove all the if( IsViewLayer() ) / if (IsTableLayer() ) logic.

And due to the move of code into a new file, it is impossible for me to review properly changes when looking at the diff since I cannot know if methods have just been moved or if things have changed in them. The physical move of existing methods that have been ported to the new class should be done in a second step.

Your merge of drv_sqlite.html has lost content.

You should consider running Valgrind or something like that to investigate why you get random crashes (valgrind python ogr_sqlite.py). Currently, this doesn't help being confident ;-)

I'm not convinced that the enum OGRSQLiteDatabaseType properly captures all the complexity of the situations. For example, OSDBT_Rasterlite2_Tables shouldn't be exclusive of OSDBT_SpatialTable_4. Rasterlite2 is an extension of Spatialite4. So maybe something an additional flag indicating the presence of raster tables might be more appropriate.

Also I don't get why enum OGRSQLiteVectorLayerType get items that are about rasterlite tables or mbtiles tables/views, which are *not* vector layer. This is really confusing.

For clarity, OSDBT_SpatialTable_x should be renamed OSDBT_Spatialite_x.

Same for OSVLT_SpatialTable_x --> OSVLT_SpatialiteTable_x

There are indentation inconsistencies in your code with the rest of the code base: 4 space characters per indentation level are the usual practice in GDAL. Some lines are also clearly much longer than 80 characters. You can split long strings on several lines.

comment:38 by mj10777, 8 years ago

https://trac.osgeo.org/gdal/ticket/5582#comment:14

OGRSQLiteLayer that would host the common methods, and OGRSQLiteTableLayer and OGRSQLiteViewLayer would inherit from it.

  • and you agreed to this

https://trac.osgeo.org/gdal/ticket/5582#comment:24

The reason for the name, is because is misread your first note: the common logic of both are in OGRSQLiteVectorLayer and not only the editing portion.

Yes the intent is to maximize code sharing. I'm not particularly good at finding names, so pick up one...

  • and have being using it since

For clarity, OSDBT_SpatialTable_x should be renamed OSDBT_Spatialite_x.

  • not clear at all - using the original name as used in Spatialite is clearer


OSDBT_Rasterlite2_Tables

  • is intended for use with the RL2-administration Tables
    • which in most cases will also be OSVLT_SpatialTable_4 tables

Also I don't get why enum OGRSQLiteVectorLayerType get items that are about rasterlite tables or mbtiles tables/views, which are *not* vector layer. This is really confusing.

https://trac.osgeo.org/gdal/ticket/5582#comment:30 The database type should be a member of the OGRSQLiteDataSource class.

  • even for mbtiles OGRSQLiteDataSource::Open is being called (as for GeoPackage)
    • this->eDatabaseType=OGRSQLiteVectorLayer::GetSQLiteDatabaseType(this);
      • whether is a table or view originated mbtiles file is important
  • for OSDBT_Rasterlite2_Tables
    • is not exclusive, but inclusive
      • the administration tables will/should not be listed as normal Geometry tables

The layer type a member of OGRSQLiteVectorLayer class.

  • OSVLT_Rasterlite2_Raster
    • are administration tables that, in most - but not all, contain geometries
      • and therefore should not be treated as OSVLT_SpatialView_4

Maybe the following will help to clarify where these ideas came from: During the Alpha stage of RasterLite2, I made a first attempt in creating a Raster-Driver

  • I used the implementation of frmts/rasterlite as a starting point
    • there was an interaction between the two
      • the information needed from the administration tables came from the ogr class

So the intention is creating a OGRRasterLite2Layer derived from OGRSQLiteVectorLayer

  • that will collect the admin-tables needed for the frmts/rasterlite2 driver being rendered

BTW: this is a much better system for writing these notes

comment:39 by Even Rouault, 8 years ago

Regarding the review process, you could make a pull request against the GitHub mirror at https://github.com/OSGeo/gdal . The UI would make it easier to comment, and by the way, we could check that your patch compiles cleanly and the tests pass in Travis-CI.

Some of the explanations you give about the meaning of the constants would be good to be included as comments in the source.

I still think that an effort should made to clean OGRSQLiteDatabaseType and OGRSQLiteVectorLayerType enumerations. There are unused stuff in them or stuff not strictly needed for now, even if it might be helpful in the future. The less code, the better. It will be always time to extend if needed layer. But for now I'd prefer not seeing anything rasterlite, mbtiles or geopackage related.

I appreciate your efforts for this contribution, and I know this can be frustrating, but if you don't make the changes I suggest, at least for the major points I pointed, it is unlikely that I spend more time doing them myself until I need myself that feature.

comment:40 by mj10777, 8 years ago

I am restarting this from scratch

  • creating an local 'sqlite_spatialite' branch
    • based on 'trunk' of this morning (2015-11-19)

I believe at the beginning of this, I was not sure if other (non-spatialite) may be using the present 'OGRSQLiteTableLayer' and 'OGRSQLiteViewLayer'

  • but have come to the conclusion that this is not so
    • the use of the non-spatialite specific enums was intended to insure that these others would not use the classes

Thus I would suggest the following:

  • 'OGRSQLiteVectorLayer' should be called 'OGRSpatialiteLayer'
    • the sole task would be to deal with everything that has to deal with a

the understanding being:

  • that a 'SpatialView', when writable
    • has only slight differences from a 'SpatialTable'
      • which 'OGRSpatialiteLayer' will deal with
        • having a 95% common code base

the (presently in 'OGRSQLiteVectorLayer') function

  • GetSQLiteDatabaseType
    • is to determine the sqlite container being read
      • supported by GDAL
        • which type
      • not supported by GDAL
        • but is a known sqlite container (such as fossil)
          • other drivers would in such cases not need to check further
            • OSDBT_NotSupported (as a suggestion, otherwise OSDBT_Unknown)
      • unknown [OSDBT_Unknown]
    • would be moved, with corresponding enums, to
      • 'OGRSQLiteLayer'

in the suggested 'OGRSpatialiteLayer'

I have been giving thoughts on future support for a Spatialite specific support of

  • RasterLite2 and Topology
    • any enums needed for those
      • would be defined in any future classes
        • which will probably be more of a administration classes
          • to service any requests needed

Originally the code base of 'OGRSQLiteVectorLayer'

  • was based on 'OGRSQLiteTableLayer'
    • so I have created a diff of the then version to today's
      • and will adapt in other changes such as
        • 'CE_Failure' to 'OGRERR_FAILURE'
        • 'EQUALN' to 'STARTS_WITH_CI'
          • and anything else I can find

If a 'OGRSpatialiteLayer' accepted

  • I would suggest that the new, stripped down version, of
    • 'OGRSQLiteTableLayer' and 'OGRSQLiteViewLayer' be replaced with
    • 'OGRSpatialTableLayer' and 'OGRSpatialViewLayer'

Inside 'OGRSpatialiteLayer'

  • and its derived classes
    • naming conventions should be a near as possible to the original Spatialite names

But if you think that

  • 'OGRSpatialiteTableLayer' and 'OGRSpatialiteViewLayer'
    • is needed, lets do it now and finalize these points

There are indentation inconsistencies in your code with the rest of the code base

  • do you have a 'astyle' setting that corresponds to the usual practice in GDAL?
    • I could then apply that as the easiest way to get everything into an acceptable form

Your merge of drv_sqlite.html has lost content.

  • will adapt today's trunk version for the new sqlite_spatialite branch

pull request against the GitHub? mirror at


If we can agree on these basics

  • I will prepare a pull request
    • so that it can be reviewed to determine any remaining irregularities

I see this ticket being only for a

  • proper support of writable SpatialView
    • as a step 1 that should be completed and fully integrated
      • before a step 2 and 3 can be started
        • RasterLite2 and Topology (not necessarily in that order)

Sandro is at the moment finalizing the Topology support

  • which I have been working with in the last weeks
    • so supporting a 'OGRSpatialiteTopologyLayer' in applications such has QGIS
      • may be the next logical step

comment:41 by mj10777, 8 years ago

A working version is now available and can be viewed here:

https://github.com/mj10777/gdal

https://github.com/OSGeo/gdal/compare/trunk...mj10777:trunk

If you know of a 'astyle' (or another linux formatting tool) command to reformat and indentation differences, I would apply them.

The html page has been adapted and comments have been added in the functions header where needed (OGRSpatialiteViewLayer::GetGeometryTable())

I can send a direct pull request directly, but at moment I assume you would prefer to look at the differences first.

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

comment:42 by Even Rouault, 5 years ago

Milestone: closed_because_of_github_migration
Resolution: wontfix
Status: newclosed

This ticket has been automatically closed because Trac is no longer used for GDAL bug tracking, since the project has migrated to GitHub. If you believe this ticket is still valid, you may file it to https://github.com/OSGeo/gdal/issues if it is not already reported there.

Note: See TracTickets for help on using tickets.