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

MVT combine: Fix value offset bug #263

Closed
wants to merge 1 commit into
base: svn-trunk
from

Conversation

Projects
None yet
2 participants
@Algunenano
Member

Algunenano commented Jul 3, 2018

WIP for CartoDB#20

I still need to find a way of test this automatically within Postgis

@Algunenano Algunenano referenced this pull request Jul 3, 2018

Closed

St_AsMVT: Duplicated keys #20

@Algunenano

This comment has been minimized.

Show comment
Hide comment
@Algunenano
Member

Algunenano commented Jul 3, 2018

@pramsey

This comment has been minimized.

Show comment
Hide comment
@pramsey

pramsey Jul 3, 2018

Member

Could (urg) expose some functions to read a two MVT bytea and return a combined MVT bytea. This is where having the MVT stuff ported into liblwgeom would help (I could pick up that old project and try to complete it, but it's ugly for other reasons and seems to necessarily involve extra copying. Something to talk about at the sprint.)

Anyways, as long as MVT is in ./postgis, the only testing is going to be integration testing, so maybe a _postgis_mvt_combine(bytea,bytea) is the "best" way.

Member

pramsey commented Jul 3, 2018

Could (urg) expose some functions to read a two MVT bytea and return a combined MVT bytea. This is where having the MVT stuff ported into liblwgeom would help (I could pick up that old project and try to complete it, but it's ugly for other reasons and seems to necessarily involve extra copying. Something to talk about at the sprint.)

Anyways, as long as MVT is in ./postgis, the only testing is going to be integration testing, so maybe a _postgis_mvt_combine(bytea,bytea) is the "best" way.

@Algunenano

This comment has been minimized.

Show comment
Hide comment
@Algunenano

Algunenano Jul 4, 2018

Member

Anyways, as long as MVT is in ./postgis, the only testing is going to be integration testing, so maybe a _postgis_mvt_combine(bytea,bytea) is the "best" way.

I'm not a fan of making internal functions public so they can be tested but with what we have it does seem the only/best way to do it. I also abhor testing with bytea since the only thing that you are testing is that it doesn't change binary wise and to check it you (and whoever comes next) have to do it manually.

For the future, I'd like to have functions to work with MVTs (decode, combine, extract layers/features...) but it isn't an easy task and I don't know how useful they'd be apart from testing, as I don't expect people to store final MVTs in a column. Something like MVT as a geometry type.

Another option would be to have a different test framework for MVTs and use external tools to do the validation, which is basically automating what I've been doing manually. This would either force us to bring dependencies in or build the framework out.

This is certainly something that requires more thinking and that I don't expect to achieve in any case for 2.5 so I'll get this fix merged and create a trac issue so this can be considered in the future.

Member

Algunenano commented Jul 4, 2018

Anyways, as long as MVT is in ./postgis, the only testing is going to be integration testing, so maybe a _postgis_mvt_combine(bytea,bytea) is the "best" way.

I'm not a fan of making internal functions public so they can be tested but with what we have it does seem the only/best way to do it. I also abhor testing with bytea since the only thing that you are testing is that it doesn't change binary wise and to check it you (and whoever comes next) have to do it manually.

For the future, I'd like to have functions to work with MVTs (decode, combine, extract layers/features...) but it isn't an easy task and I don't know how useful they'd be apart from testing, as I don't expect people to store final MVTs in a column. Something like MVT as a geometry type.

Another option would be to have a different test framework for MVTs and use external tools to do the validation, which is basically automating what I've been doing manually. This would either force us to bring dependencies in or build the framework out.

This is certainly something that requires more thinking and that I don't expect to achieve in any case for 2.5 so I'll get this fix merged and create a trac issue so this can be considered in the future.

@strk strk closed this in bc3cfed Jul 6, 2018

Algunenano added a commit to Algunenano/postgis that referenced this pull request Jul 18, 2018

MVT combine: Fix bug with invalid property values
Closes #4115
Closes postgis#263

git-svn-id: http://svn.osgeo.org/postgis/trunk@16636 b70326c6-7e19-0410-871a-916f4a2858ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment