Opened 2 years ago

Closed 23 months ago

#4128 closed enhancement (fixed)

ST_AsMVT: add feature id support

Reported by: stepankuzmin Owned by: pramsey
Priority: medium Milestone: PostGIS 3.0.0
Component: postgis Version:
Keywords: Cc:

Description

According to Vector Tile Specification v2.1 (https://github.com/mapbox/vector-tile-spec/tree/master/2.1#42-features):

A feature MAY contain an id field. If a feature has an id field, the value of the id SHOULD be unique among the features of the parent layer.

But there is no way to set a feature id using ST_AsMVT.

Change History (13)

comment:1 Changed 2 years ago by Algunenano

Milestone: PostGIS 2.5.0PostGIS 3.0.0
Type: defectenhancement

This is a feature request. Moving it to 3.0.

PR by stepankuzmin with discussion in https://github.com/postgis/postgis/pull/274

comment:2 Changed 23 months ago by komzpa

Resolution: fixed
Status: newclosed

In 16808:

ST_AsMVT: Support for Feature ID.

Patch by Stepan Kuzmin.

Closes #4128
Closes https://github.com/postgis/postgis/pull/274

comment:3 Changed 23 months ago by Algunenano

Resolution: fixed
Status: closedreopened

I have several objections to this change:

  • It doesn't include documentation.
  • The id column is added twice, once in the id field and one as a property. This duplicates information making tiles bigger for no reason.
  • If there are multiple columns with the same name there will be extra work done.
  • IMO it should throw if the type is invalid, not just silently ignore it. It's not documented which types are supported.
  • The check is added in a hot path and it's done for each property for each feature. Instead it should be done, as with the geometry column, once at the start. I expect this to have an important impact on performance, even when not using this feature.

I'm reopening it to have a look into metrics and document this new feature properly.

comment:4 Changed 23 months ago by komzpa

@Algunenano documentation for the new argument was added in this change. Did I miss something else that should have been documented?

comment:5 Changed 23 months ago by Algunenano

@Algunenano documentation for the new argument was added in this change. Did I miss something else that should have been documented?

Yeah, sorry I missed that. I was checking Github PR and not the changeset. The only thing missing there is to define which are the valid types for it.

comment:6 Changed 23 months ago by komzpa

Something is off with this change, it works locally and on Travis, but not on Drone:

[20:37] <strk> mvt tests crash backend
[20:37] <strk> https://drone.osgeo.org/postgis/postgis/1325/3
[20:38] <strk> since r16808

comment:7 Changed 23 months ago by strk

If you have docker installed you can investigate the environment of Dronie with:

docker run -ti --rm \
 docker.kbt.io/postgis/build-test:trisquel2 \
 /bin/bash

I did, to check protoc version:

root@c4bdbe9eb096:/# protoc-c --version
protobuf-c 1.3.0
libprotoc 3.5.1

comment:8 Changed 23 months ago by strk

If you have drone installed (version 0.5) you can reproduce the Dronie's builds locally with changing into PostGIS source tree and running:

  drone exec

comment:9 Changed 23 months ago by robe

debbie is getting an mvt crash as well:

https://debbie.postgis.net/job/PostGIS_Regress/10058/consoleFull

 mvt .. failed (diff expected obtained: /var/lib/jenkins/workspace/postgis/tmp/3_0_pg10w64/test_125_diff)
-----------------------------------------------------------------------------
--- mvt_expected	2018-09-16 16:55:14.518502234 +0000
+++ /var/lib/jenkins/workspace/postgis/tmp/3_0_pg10w64/test_125_out	2018-09-16 17:10:02.610250474 +0000
@@ -47,53 +47,7 @@
 PG43 - ON |MULTIPOLYGON(((5 5,0 0,10 0,5 5)),((0 10,5 5,10 10,0 10)))
 PG43 - OFF|MULTIPOLYGON(((5 5,-1 -1,11 -1,5 5)),((5 5,11 11,-1 11,5 5)))
 TG1|GiEKBHRlc3QSDBICAAAYASIECTLePxoCYzEiAigBKIAgeAI=
-TG2|GiMKBHRlc3QSDhICAAAYASIGETLePwIBGgJjMSICKAEogCB4Ag==
-TG3|GiYKBHRlc3QSERICAAAYAiIJCQCAQArQD88PGgJjMSICKAEogCB4Ag==
-TG4|GiYKBHRlc3QSERICAAAYAiIJCQCAQArQD88PGgJjMSICKAEogCB4Ag==
-TG5|GjAKBHRlc3QSGxICAAAYAiITCQL+PwrQD88PCc0Pzg8K0A/PDxoCYzEiAigBKIAgeAI=
-TG6|GjIKBHRlc3QSHRICAAAYAyIVCUbsPxoxEwonPAkPCTEeEhQUCh0PGgJjMSICKAEogCB4Ag==
-TG7|Gj0KBHRlc3QSKBICAAAYAyIgCVCwPxIKFDEdDwkAFCIyHh0eJwkAJw8JKBQSEwkAFA8aAmMxIgIo
-ASiAIHgC
-TG8|GiEKBHRlc3QSDBICAAAYASIECTLePxoCYzEiAigBKIAgeAI=
-TG9|GiMKBHRlc3QSDhICAAAYASIGETLeP2VGGgJjMSICKAEogCB4Ag==

comment:10 Changed 23 months ago by Algunenano

The id value is being assigned directly from the datum, which is will cause issues such as those crashes. I also see that it doesn't do any bound checking (so negative values ca be assigned) which could cause issues too.

I'll try to have it fixed and fully documented (negative or NULL values, invalid types, column name not found...) tomorrow.

comment:11 Changed 23 months ago by Algunenano

My proposed changes:

  • Obviously fix the bug casting the Datum to the appropriate type.
  • Match the geometry column behaviour: Do the validation once, including type, and fail if not found.
  • Do not include the id column as a property too. If you want the information duplicated you can add the column twice. Document this.
  • Silently ignore (leave them as id not set) NULLs and negative values (not supported by MVT). If you want a default value you can set it up before passing the information to the aggregation function. Document this.
  • The column is chosen as the first column with the correct name and valid types (smallint, integer, bigint). Properties inside JSON values are not supported. Document this.
  • Add tests around all these behaviours.

PR coming.

comment:13 Changed 23 months ago by Raul Marin

Resolution: fixed
Status: reopenedclosed

In 16825:

St_AsMVT: Fix id support, and extra tests and documentation

Closes #4128
Closes https://github.com/postgis/postgis/pull/303

Note: See TracTickets for help on using tickets.