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

ST_AsMVT: add feature id support #274

Closed
wants to merge 7 commits into from

Conversation

@stepankuzmin
Copy link

commented Jul 18, 2018

There is no way to set a feature id using ST_AsMVT (https://trac.osgeo.org/postgis/ticket/4128).

This PR adds following functionality — after keys and values are encoded in layer, encode_feature_ids is called. It will iterate over all features and assign them uint id from id column if there is any.

This is not optimal, IMO. Eg. it would be more convenient if we add optional argument to ST_AsMVT that will hold column name with id values.

I'll appreciate any feedback on this issue.

@Algunenano

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

I feel like this should definitely be optional and it how to implement it need to be thought about. Mainly I see two big questions:

  • Do we want to be able to disable it or not?
  • Do we want to use the value from a column passed as option or use some magic column name (id here)?
  • What do we do if the column isn't a valid int? Leave it empty or generate it sequentially (node-mapnik with postgis does that).
  • If we use a column from the input, do we want to include that column also in the keys/values?

In any case, setting it up should be done during the encoding of the feature (parse_values), not at the end of the process, to avoid reiterating over all the features at the end.

Finally, please note that since 2.5 is feature-closed, this will have to wait for 3.0 to open up (whenever that is).

@stepankuzmin

This comment has been minimized.

Copy link
Author

commented Jul 19, 2018

Thanks for the reply, @Algunenano! I've updated the PR.

ST_AsMVT now accepts id_name as last optional argument. If id_name is set, ST_AsMVT will set feature id from that column on parse_values stage. It will check value to be INT2OID, INT4OID or INT8OID.

@Komzpa

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Travis crashes:


 regress_brin_index_geography .. ok 
 mvt .. failed (diff expected obtained: /tmp/pgis_reg/test_121_diff)
-----------------------------------------------------------------------------
--- mvt_expected	2018-07-19 15:40:03.268320668 +0000
+++ /tmp/pgis_reg/test_121_out	2018-07-19 15:43:56.548914659 +0000
@@ -56,30 +56,15 @@
 TG4|GiYKBHRlc3QSERICAAAYAiIJCQCAQArQD88PGgJjMSICKAEogCB4Ag==
 TG5|GjAKBHRlc3QSGxICAAAYAiITCQL+PwrQD88PCc0Pzg8K0A/PDxoCYzEiAigBKIAgeAI=
 TG6|GjIKBHRlc3QSHRICAAAYAyIVCUbsPxoxEwonPAkPCTEeEhQUCh0PGgJjMSICKAEogCB4Ag==
-TG7|Gj0KBHRlc3QSKBICAAAYAyIgCVCwPxIKFDEdDwkAFCIyHh0eJwkAJw8JKBQSEwkAFA8aAmMxIgIo
-ASiAIHgC
+TG7|Gj8KBHRlc3QSKggBEgIAABgDIiAJULA/EgoUMR0PCQAUIjIeHR4nCQAnDwkoFBITCQAUDxoCYzEi
+AigBKIAgeAI=
 TG8|GiEKBHRlc3QSDBICAAAYASIECTLePxoCYzEiAigBKIAgeAI=
 TG9|GiMKBHRlc3QSDhICAAAYASIGETLeP2VGGgJjMSICKAEogCB4Ag==
 TA1|Gi8KBHRlc3QSDhIEAAABARgBIgQJMt4/GgJjMRoCYzIiAigBIgYKBGFiY2QogCB4Ag==
 TA2|GigKBHRlc3QSDBICAAAYASIECTLePxoCYzEiCRmamZmZmZnxPyiAIHgC
 TA3|GhkKBHRlc3QSCBgBIgQJMt4/GgJjMSiAIHgC
 TA4|GjMKBHRlc3QSDBICAAAYASIECTLePxIMEgIAARgBIgQJMt4/GgJjMSICKAEiAigCKIAgeAI=
-TA5|Gi8KBHRlc3QSDhIEAAABARgBIgQJMt4/GgJjMRoCYzIiAigBIgYKBGFiY2QogCB4Ag==
-TA6|GisKBHRlc3QSDhIEAAABARgBIgQJMt4/GgJjMRoCYzIiAigBIgIwASiAIHgC
-TA7|GmQKBHRlc3QSDhIEAAABARgBIgQJMt4/Eg4SBAAAAQIYASIECTTcPxIOEgQAAwEEGAEiBAk03D8a
-AmMxGgJjMiIGCgR0ZXN0IgIoASICKAIiCwoJb3RoZXJ0ZXN0IgIoAyiAIHgC
-TA8|Gk8KBHRlc3QSDhIEAAABABgBIgQJMt4/Eg4SBAAAAQEYASIECTTcPxIOEgQAAQECGAEiBAk03D8a
-AmMxGgJjMiICKAEiAigCIgIoAyiAIHgC
-TA9|0
-TA10|49
-D1|Gi8KBHRlc3QSDhIEAAABARgBIgQJMt4/GgJjMRoCYzIiAigBIgYKBGFiY2QogCB4Ag==
-D2|Gi8KBHRlc3QSDhIEAAABARgBIgQJMt4/GgJjMRoCYzIiAigBIgYKBGFiY2QogCB4Ag==
-D3|Gi8KBHRlc3QSDhIEAAABARgBIgQJMt4/GgJjMRoCYzIiAigBIgYKBGFiY2QogCB4Ag==
-D4|GjIKB2RlZmF1bHQSDhIEAAABARgBIgQJMt4/GgJjMRoCYzIiAigBIgYKBGFiY2QogCB4Ag==
-D5|POINT(1 4094)
-D6|POINT(1 4094)
-D7|POINT(1 4094)
-TU2
-ERROR:  pgis_asmvt_transfn: parameter row cannot be other than a rowtype
-TU3|
-#3922|91
+server closed the connection unexpectedly
+	This probably means the server terminated abnormally
+	before or while processing the request.
+connection to server was lost
@BBegouin

This comment has been minimized.

Copy link

commented Aug 1, 2018

Hi !

do you have any progress on this feature ?
It would be great for me as well !

need any help ?

@stepankuzmin

This comment has been minimized.

Copy link
Author

commented Aug 2, 2018

Hi @BBegouin! Unfortunately I don't have enough time to check failing tests. I would appreciate any help with this.

@Komzpa

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

Could you please rebase the patch?
I recently added postgres stacktrace reporting into Travis, so a rebase on svn-trunk can shed some light on the crash.

@strk strk closed this in 573e511 Sep 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.