Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Requirement 32/Req 32 Test Clarification Needed #445

Closed
ajanett opened this issue May 31, 2018 · 7 comments · Fixed by #549
Closed

Requirement 32/Req 32 Test Clarification Needed #445

ajanett opened this issue May 31, 2018 · 7 comments · Fixed by #549
Milestone

Comments

@ajanett
Copy link

ajanett commented May 31, 2018

Requirement 32 states: "Feature table geometry columns SHALL contain geometries of the type or assignable for the type specified for the column by the gpkg_geometry_columns table geometry_type_name uppercase column value."

Please define the allowable 'assignable' subtypes for feature table geometries given the supertype specified in gpkg_geometry_columns. The current documentation is rather unclear on this and there appear to be possible conflicts in the documentation.

In section 2.1.1 there is an overview of the Core Geometry Model, with a model diagram, and then there are some descriptions that appear to specifically restrict some of the values for subtypes:
• CurvePolygon: A planar surface defined by an exterior ring and zero or more interior ring. Each ring is defined by a Curve instance.
• Polygon: A restricted form of CurvePolygon where each ring is defined as a simple, closed
LineString.
• GeometryCollection: A collection of zero or more Geometry instances.
• MultiSurface: A restricted form of GeometryCollection where each Geometry in the collection must be of type Surface.
• MultiPolygon: A restricted form of MultiSurface where each Surface in the collection must be of type Polygon.
• MultiCurve: A restricted form of GeometryCollection where each Geometry in the collection must be of type Curve.
• MultiLineString: A restricted form of MultiCurve where each Curve in the collection must be of type LineString.
• MultiPoint: A restricted form of GeometryCollection where each Geometry in the collection must be of type Point.

Within Annex G: Geometry Types, after Table 28 we have:
GEOMETRY subtypes are POINT, CURVE, SURFACE and GEOMCOLLECTION.
CURVE subtypes are LINESTRING, CIRCULARSTRING and COMPOUNDCURVE.
SURFACE subtype is CURVEPOLYGON.
CURVEPOLYGON subtype is POLYGON.
GEOMETRYCOLLECTION subtypes are MULTIPOINT, MULTICURVE and MULTISURFACE. MULTICURVE subtype is MULTILINESTRING.
MULTISURFACE subtype is MULTIPOLYGON.


So there are conflicts; for example:
Annex G says MultiSurface subtype is MultiPolygon but 2.1.1 says MultiSurface instances must be of type Surface.
Annex G lists no shows Polygon only as a subtype to CurvePolygon supertype, but 2.1.1 says Polygon instances are LineStrings
...

Basically, it would be really helpful if there was a clear statement as to which subtypes are allowed per geometry type.

Please also indicate whether the extended geometry types are allowed at this time as the current test code does not check for/allow all of the extended geometry types.

@jyutzler
Copy link
Contributor

jyutzler commented Jun 13, 2018

Since Annex G is normative, it takes precedence. Section 2.1.1 is an overview of Simple Features SQL that explains where this stuff comes from but it does not set GeoPackage requirements regarding assignability or anything else.

Extended geometry types are allowed through the Non-Linear Geometry Types Extension.

@ajanett
Copy link
Author

ajanett commented Jun 14, 2018

The specified set of geometry types is not the problem, the problem is the "is assignable" super type and subtype geometries. The second part of Requirement 32 states that geometries within the WKB of a feature may be different than the geopackage stated geometry, but they must be 'assignable'. If the geopackage specification does not specify subtype geometries are assignable to supertype geometries, how are we to identify this in a manner that will create an accurate and complete test for this requirement?

Geopackage Pull request code:
opengeospatial/ets-gpkg12#62

Contains code that performs the test for Requirement 32 but my confidence in the "is assignable" list is not 100% because of the issues stated above. Hence, this request for clarification of the requirement so that the test code can be shown to be an accurate test for this requirement.

@jyutzler
Copy link
Contributor

Based on feedback from John Herring, I think we need to get rid of the "is assignable" nomenclature. You can put any geometry into a GEOMETRY and otherwise you have to put what it is.

I will need to confirm with other SWG members.

@jyutzler
Copy link
Contributor

Today the SWG agreed on eliminating the "is assignable" nomenclature. This was determined to be an administrative change.

jyutzler added a commit to jyutzler/geopackage that referenced this issue Jun 18, 2018
@jyutzler jyutzler added this to the 1.3.0 milestone Jun 18, 2018
jyutzler added a commit that referenced this issue Jun 25, 2018
eliminate use of "assignable" #445
@ajanett
Copy link
Author

ajanett commented Jun 29, 2018

I don't believe that eliminating the use of "assignable" helps. Or perhaps I am confused. There is no issue with the simple geometry types. This is about the more complicated geometry types.

Background: Remember there is a gpkg_geometry_columns geometry type and there are also related feature instance rows, each with a geometry type inside the geometry BLOB for that feature instance. It is simple enough to check for identical geometry type, but some geometry types are complex and I don't believe they should be identical in that case. Hence, the IsAssignable portion of the test was in the specification for this purpose and removal of it is the wrong thing to do - unless the allowed geometry types are restricted to more tightly then the current specification states.

Example situation: Consider if gpkg_geometry_columns is set to MultiPolygon, then I believe it is valid, or isn't it actually REQUIRED that you then set the feature geometry BLOB type to Polygon? Similarly MultiPoint is required to have the subtype of Point ; and MultiLineString is required to have the subtype geometry of LineString.

The situation is more complex when the extended geometries as per Annex G Table 31 are specified. This issue request is trying to get the specification to tell us what the allowed subtypes are for these geometries. What I would like to see, is another column, added to Annex G. Let the current geometry type column list the geometry type name in gpkg_geometry_columns and the added column list the geometries allowed? required? in the respective feature geometry BLOB records.

If this information isn't well defined in the specification, it is very difficult to create the correct test for it.

@KRyden
Copy link

KRyden commented Oct 16, 2020

Jeff - some further clarification appears to be needed here....

Section 2.1.6.1.2 Table Data Values

this paragraph:

Allowed geometry types are defined in Geometry Types (Normative) and shown in part in Figure 2.
If the geometry type_name value is "GEOMETRY" then the feature table geometry column MAY
contain geometries of any allowed geometry type. This type name is to be used when the geometry
type is unknown or if multiple geometry types will be present in the same table (e.g., POLYGON and
MULTIPOLYGON). If the geometry type_name value is "GEOMETRYCOLLECTION" then the feature
table geometry column MAY contain zero or more geometries of any allowed geometry type.

The last sentence should be modified as follows - the change is after the MAY:

If the geometry type_name value is "GEOMETRYCOLLECTION" then the feature
table geometry column MAY contain geometries of type GeometryCollection containing zero or more geometries of any allowed geometry type.

@jyutzler jyutzler reopened this Oct 16, 2020
@jyutzler
Copy link
Contributor

I'll prepare a PR for Tuesday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants