[gdal-dev] Re: OGR/Spatialite patches

[Even Rouault]

Hi Alessandro,

(CC'ing gdal-dev)

Impressive work !

My remarks and questions:					
											
1) Could you confirm that all this work might be included under the usual GDAL 
X/MIT licence ? Due to the significant code addition for the 3D geometry 
support, you might want to add your copyright in the header of 
ogrsqlitelayer.cpp.

2) It would be really convenient to have different patches for the different 
issues that could be applied sequentially. Applying all this stuff in a single 
gulp will make bisection and analysis of the changes quite painfull. I've 
skimmed quickly through the whole diff and your README and I can see several 
patches, preferably in the following order (if possible) :
	1) 3D geometry support
	2) use the spatialite SQL functions and drop the manual workarounds to 
avoid using spatialite functions
	3) general cleanup to remove the use of ancient sqlite API (that seem not 
to be directly related to spatialite support)

I'd be in favor of dropping the stuff related to #ifdef SPATIALITE_AMALGAMATION 
for now. Or it might be traced under a different ticket

3) From your patch I can see that my efforts to produce a spatialite database 
without linking to spatialite were vain. I'm not that surprised however.... So 
the question is : is there really an interest in allowing the user to create a 
spatialite database without linking to spatialite if that database cannot be 
directly used by other spatialite-enabled software ant that the user must 
"repair" it afterwards with the procedure at the end of your README.txt. This 
really sounds to be compatible with only super power users... We might also 
want to be sure that no write/update/delete operations can happen to a 
spatialite database if it is opened with a driver not linked against 
libspatialite to avoid corrupting it.
However I think it is still desirable to be able to read a spatialite database 
without requiring linking to spatialite.

4) We must be aware that using the ALTER TABLE ... ADD COLUMN ... stuff will 
produce databases (even regular SQLite ones) that will not be compatible with 
the latest FWTools for Windows that uses an ancient sqlite versions. Might not 
be a big deal however because I somehow remember that dropping a new version 
of sqlite3.dll in the bin directory make it work again.

5) Would you be willing to update/extend the autotest/ogr/ogr_sqlite.py test 
to ensure that it still passes and add some tests for all the new stuff, 
particularly the 3D geometry support.

6) From your README.TXT, can I assume that your changes have been tested 
against regular libsqlite3 (without linking to spatialite) with a spatialite 
database (and also a regular sqlite database not using spatialite geometries), 
and against libspatialite 2.3.1 and libspatialite 2.4.0 for spatialite 
databases ?

7) A minor code note : it could be interesting to replace the magic values for 
the geometry types by appropriate #define with symbolic names.

8) I've seen you have removed the tests to detect int overflows, for example 
"if( nPointCount < 0 || nPointCount > INT_MAX / (2 * 8))" has become "if( 
nPointCount < 0)". Could you justify it ? I still think that they are 
necessary to avoid issues in lines below if the nPointCount value is huge 
enough.

9) About your [3c] patch. Yes indeed this is a painfull issue with shapefiles 
(the format I mean, not the driver) where we cannot know in advance if it only 
contains LINESTRING or MULTILINESTRING. Currently the shapefile driver chooses 
to report the layer as having LINESTRING geometry type. Maybe Frank can 
comment about this. I imagine this was decided because some application 
software wouldn't know to deal with multi-geometry. But I can confirm that 
inserting such shapefiles with mixed linestring/multilinestring geometries into 
Postgis requires a -nlt flag to the ogr2ogr command line. To get back to your 
patch, I'm wondering what will happen if we try to insert a feature with a 
LINESTRING geometry into a layer created with the LINESTRING geometry type, 
that has been "promoted" to MULTILINESTRING by the driver ?

Best regards,

Even

Le mercredi 30 mars 2011 12:37:05, Alessandro Furieri a écrit :
> Hi All,
> 
> I know for sure that all you are strongly
> interested in both SpatiaLite and GDAL/OGR:
> so I'm glad to submit to your attention a
> patch-set intended to enhance the OGR/spatialite
> driver.
> 
> - correcting several issues found into the current
>    implementation
> - supporting the most recent features introduced
>     by v.2.4.0 [3D Geometries and so on]
> 
> You'll find everything within the attached zipfile:
> - source code
> - svn diff
> - detailed explanations on README.txt
> 
> Please note: the current patch-set (although quite
> thoroughly tested and debugged by myself) simply
> represents a preliminary interim version.
> I suppose some further (small) optimization would
> be required before actually committing into the
> SVN trunk.
> Any suggestion, useful hint, criticism and testing
> coming from you surely will be warmly welcome and
> highly appreciated.
> 
> bye
> Sando