[OSGeo-Standards] Comments about OGC 12-128r1 Version: 0.2.0 - OpenGIS(R) GeoPackage Implementation Specification

Sat Jan 12 12:31:09 PST 2013

PART A

1. Evaluator:
    Even Rouault, even.rouault at mines-paris dot org
    GDAL/OGR contributor

2. Submission:
    OGC 12-128r1 Version: 0.2.0 - OpenGIS® GeoPackage Implementation
    Specification

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    General

3. Criticality:
    Minor

4. Comments/justifications for changes:

    It could perhaps be made more clear in the preface/introduction/scope that 
the  specification contain 2 different things :
        - storage requirements that make possible the exchange of data
        - and various SQL operations that can be used on the data stored in a 
geopackage, but are optionnal to just read or write the data.

    The Spatialite reference implementation, or any other implementation with 
equivalent SQL functions, is in no way necesserary to be able to use the 
vector information in a GeoPackage. What you need is to support the Spatialite 
geometry blob format. This is for example done in the GDAL/OGR library.

    Linked to that, I'm uneasy with the term "container" as it is used in the 
document. It makes sense  in REQ 1 to define it as a file, but later 
requirements mention properties that cannot be supported by a container in 
itself, but rather by a software implementation that provides capabilities. 
For example REQ 4 "The GeoPackage container shall provide a “C” language CLI": 
a .geopackage file doesn't provide a C language CLI, it is libsqlite3 that 
provides that. Perhaps different terms could be used to separate the format 
specification, and the services that a software implementation handling 
.geopackage should offer.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "7 - Table diagram"

3. Criticality:
    Editorial

4. Comments/justifications for changes:

    The max_y field of table geopackage_contents should be of type REAL, not 
BLOB.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    REQ 9

2. Implementation Specification Section number:
    Paragraph "6.2 - Reference Implementation"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    Is SQLite 3.7 really necessary to define the storage file format ?
    According to http://www.sqlite.org/formatchng.html , the last file format 
change was in SQLite 3.6.0. Many enterprise Linux distributions ship with 
SQLite 3.6.X (e.g. SQLite 3.6.20 on RHEL 6).

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    REQ 10

2. Implementation Specification Section number:
    Paragraph "6.2 - Reference Implementation"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    What's the usefulness of redefining the header of a SQLite3 database ? This 
is implied by REQ 9, and there are so many other requirements that should be 
written to specify the interoperability with a SQLite3 database, if you don't 
use the "SQLite3 reference implementation". This might be just an 
informational note however, to help writing a preliminary check that the file 
is a good candidate for being a geopackage container.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "8.2 - Geopackage Contents Table"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    There are inconsistencies between the default values in "Table  – 
Geopackage Contents Table or View Definition" and "Table 2 – 
geopackage_contents Table Definition SQL" for columns "identifier" and 
"description". I haven't verified, but I doubt that a default value makes sense 
for identifier if it is supposed to be UNIQUE. And for description, table 1 
mentions "", whereas table 2 mentions "none".

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "9.4 - Feature Tables"

3. Criticality:
    Major

4. Comments/justifications for changes:

    This paragraph can lead the implementator of another implementation in 
error, since you get the false impression that the "gb" storage type is WKB, 
whereas it is in fact later explained (page 46) to be the spatialite blob 
format. I think this should be more clearly stated right here.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    REQ 25

2. Implementation Specification Section number:
    Paragraph "9.4 - Feature Tables"

3. Criticality:
    Major

4. Comments/justifications for changes:

    It is unfortunate that spatial index is mentionned, but not specified ! 
Different implementations of GeoPackage could have incompatible spatial index. 
Why not just specifying the implementation done in Spatialite ?, i.e. based on 
the SQLite3 R-Tree extension.

    If that was done, it should be mentionned in REQ 9 that the SQLite3 
implementation to use must have R-Tree support, which is an optional 
configuration to enable at build time.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    REQ 26 and REQ 27

2. Implementation Specification Section number:
    Paragraph "9.4 - Feature Tables"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    I fail to see why the various quantities (1 degree bounding box, 1000 
features, etc...) are mentionned. REQ 27 could be just written "Spatial 
queries in a GeoPackage shall provide the same results with and without a 
spatial index." And REQ 28 should be just removed.
    You can certainly find artificial cases of data where the spatial index will 
lead to slower request times.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    REQ 44

2. Implementation Specification Section number:
    Paragraph "9.6 - Reference Implementation"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    Why not just specifying Spatialite V4 as the minimum version ? The various 
hacks given at page 46 to turn a Spatialite V3 into a pseudo V4 are just ugly 
and lead to confusion. They could be mentionned on non-official documents, such 
as the Spatialite wiki, but it is awkward to see them mentionned in a formal 
document, like that one.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    REQ 47

2. Implementation Specification Section number:
    Paragraph "9.6 - Reference Implementation"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    It would be worth mentionning that the "Compressed geometries" described 
in http://www.gaia-gis.it/gaia-sins/BLOB-Geometry.html are not valid for a 
baseline GeoPackage.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "10.2 Raster Columns"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    I'd suggest making the compr_qual_factor and georectification columns 
nullable, or have another default value meaning "unknown". If you convert an 
existing set of tiled JPEGs into a GeoPackage, you generally don't know the 
quality associated (it is certainly different of 100 %, but is it 25% or 75% 
??). Same for georectification. Those columns provide non essential 
information.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "10.7 Rasters or Tiles Table Metadata"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    Actually, in RasterLite (current state of public sources at least), the 
raster metadata table is suffixed by _metadata, not _rt_metadata.

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "10.8.10 gpkgBboxToTiles SQL Function"

3. Criticality:
    Major

4. Comments/justifications for changes:

    I'm not sure how gpkgBboxToTiles() can be implemented as a SQLite 
function. To the best of my knowledge, and according to 
http://www.sqlite.org/c3ref/create_function.html, custom SQLite functions can 
return a single value (string, integer, real, text, blob), potentially 
operating as aggregates on several source rows, but not generating several 
rows. What is the "set of integers" return value mentionned here : a string of 
ids, separated by commas or spaces ?

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    -

3. Criticality:
    Major

4. Comments/justifications for changes:

    In the context of a raster pyramid whose more resoluted level is e.g. a 
world region, but that has pyramids up to the "whole world" zoom level, my 
understanding is that the bounding box recorded into geopackage_contents would 
be -180,-90,180,90. Not particularly useful. It would be good to have a direct 
way of knowing the extent of the more resoluted data, without having to run a 
query on the _rt_metadata table, like "SELECT MIN(min_x), MIN(min_y), 
MAX(max_x), MAX(max_y) FROM xxx_rt_metadata", which can be slow. Furthermore, 
if the edge tiles have padding, you would get that padding in the returned 
extent, which might be undesirable.
    In the tile_matrix_metadata table, this could perhaps be implemented by 
adding min_x, min_y, max_x, max_y columns, which would represent the extent of 
the significant area (that is to say without any potential padding).

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "10.2 Raster Columns"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    In the raster_columns table, it might be appropriate to have a 
"band_count" column, that would indicate the maximal number of raster bands 
(GDAL terminology, or also called image channels) among all the tiles / 
rasters associated with the table with the raster column.
    For example, 1 for grey-level raster, 3 for RGB images, 4 for  RGBA 
images. This would be usefull for example when converting a GeoPackage into a 
old-fashioned GIS format, like GeoTIFF (PNG8 without transparency would be 
considered as 3 bands after color table expansion, and with transparency as 4 
bands).

-----------------------------------------------------------------------------------

PART B

1. Requirement:
    -

2. Implementation Specification Section number:
    Paragraph "13.2 Manifest Content Model and Table Definition"

3. Criticality:
    Minor

4. Comments/justifications for changes:

    Is the default value proposed for the manifest column (empty string) 
considered as a valid value that will comply with REQ 78 (I think not) ?