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

Mvt serialized #358

Closed
wants to merge 6 commits into from

Conversation

@Algunenano
Copy link
Member

commented Jan 14, 2019

Implements https://trac.osgeo.org/postgis/ticket/4294

I've also moved input checking to the input function and added tests around it

double bounds_width = (bounds->xmax - bounds->xmin) / extent;
double bounds_height = (bounds->ymax - bounds->ymin) / extent;

/* We use half the area of the grid square as minimun resolution */

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member
Suggested change
/* We use half the area of the grid square as minimun resolution */
/* We use half the area of the grid square as minimum resolution */
bounds = (GBOX *)PG_GETARG_POINTER(1);
if (bounds->xmax - bounds->xmin <= 0 || bounds->ymax - bounds->ymin <= 0)
{
elog(ERROR, "%s: bounds width or height cannot be 0", __func__);

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member

why? why not just return null?

This comment has been minimized.

Copy link
@Algunenano

Algunenano Jan 14, 2019

Author Member

It was already like that (but it was done in mvt_geom). I didn't want to change the behaviour

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member

then still <=0 and =0 mixup at least needs patching :)

This comment has been minimized.

Copy link
@Algunenano

Algunenano Jan 14, 2019

Author Member

I've tried but I haven't found a way to create an invalid box (where max < min). Do you prefer using FP_LTEQ instead?

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member

I see that we're checking for one thing and outputting a message about failing different check. It's never a good idea.

<= 0 are usually checks for math sanity, so there are no things like div0 or negative unsigned area, so FP_* aren't that good for them. If there wasn't such a check here, your later box size shortcut will filter all the geometries, so technically we can just emit a warning here that box is too small but output a valid empty thing instead of crashing the query. WDYT?

This comment has been minimized.

Copy link
@Algunenano

Algunenano Jan 14, 2019

Author Member

so technically we can just emit a warning here that box is too small but output a valid empty thing instead of crashing the query. WDYT?

I don't like it. Passing a tiny (or invalid) bounding box is clearly an error. I'm not saying that it can't be done, but when I use ST_AsMVT functions I iterate over the geometry, not the bounding box, so in that case I'd end up with an empty tile without a way to know that this was a programming error.

I see that we're checking for one thing and outputting a message about failing different check. It's never a good idea.

I'll change it to something along the lines of The geometric bounds are too small.

extent = PG_ARGISNULL(2) ? 4096 : PG_GETARG_INT32(2);
if (extent <= 0)
{
elog(ERROR, "%s: extent cannot be 0", __func__);

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member

check is <=, message is about =0 sharp. can lead to hours of looking for 0 in code :)

geom_in = PG_GETARG_GSERIALIZED_P_COPY(0);
type = gserialized_get_type(geom_in);

/* If possible, peak into the bounding box before deserializating it to discard small geometries

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member
Suggested change
/* If possible, peak into the bounding box before deserializating it to discard small geometries
/* If possible, peek into the bounding box before deserializing it to discard small geometries
{
GBOX gserialized_box;
/* We only apply the optimization if the bounding box is available */
if (gserialized_read_gbox_p(geom_in, &gserialized_box) == LW_SUCCESS)

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member

there was a _peek function that also worked for two-point linestrings without box, maybe use it?

This comment has been minimized.

Copy link
@Algunenano

Algunenano Jan 14, 2019

Author Member

I wanted to avoid repeating work in the common case (where this speedup isn't applied): in that case he bounding box would be calculated first for this check and later when deserializing the geometry.

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member

here, _peek is only used in trivial cases:

int gserialized_get_gbox_p(const GSERIALIZED *g, GBOX *box)

I suggest going first two if-branches, not all three. It feels it can be refactored so that _peek calls _read first instead of failing on present box, and here you just call a _peek.

This comment has been minimized.

Copy link
@Algunenano

Algunenano Jan 14, 2019

Author Member

Is it really that common for geometries to not have a bounding box? I'm expecting basically everything (except points) to have it available, and if not the general path isn't that slow.

This comment has been minimized.

Copy link
@Komzpa

Komzpa Jan 14, 2019

Member

Box is not written out for everything that _peek has code for.

int lwgeom_needs_bbox(const LWGEOM *geom)

This comment has been minimized.

Copy link
@Algunenano

Algunenano Jan 14, 2019

Author Member

I'll make gserialized_peek_gbox_p and use it there too. Thanks!

@strk strk closed this in 789707e Jan 14, 2019

Algunenano added a commit to Algunenano/postgis that referenced this pull request Jan 25, 2019
ST_AsMVTGeom: Do resolution check before deserializing
Closes #4294
Closes postgis#358

git-svn-id: http://svn.osgeo.org/postgis/trunk@17148 b70326c6-7e19-0410-871a-916f4a2858ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.