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.

Sign up for GitHub

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

Jump to bottom

Mvt serialized #358

Closed
wants to merge 6 commits into from
Closed

Mvt serialized #358

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
638590c
ST_AsMVTGeom: Check resolution before de-serializing the geometry
Algunenano Jan 11, 2019
35ea21c
C89
Algunenano Jan 14, 2019
6aa7c58
ST_AsMVTGeom: Address comments
Algunenano Jan 14, 2019
8f09945
ST_AsMVTGeom: Increase the resolution
Algunenano Jan 14, 2019
dce045b
ST_AsMVTGeom: Improve error messages
Algunenano Jan 14, 2019
7bade58
ST_AsMVTGeom: Use gserialized_peek_gbox_p for shortcut too
Algunenano Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 59 additions & 9 deletions postgis/lwgeom_out_mvt.c
View file
Open in desktop
Expand Up @@ -45,26 +45,76 @@ Datum ST_AsMVTGeom(PG_FUNCTION_ARGS)
elog(ERROR, "Missing libprotobuf-c");
PG_RETURN_NULL();
#else
LWGEOM *lwgeom_in, *lwgeom_out;
GBOX *bounds = NULL;
int32_t extent = 0;
int32_t buffer = 0;
bool clip_geom = true;
GSERIALIZED *geom_in, *geom_out;
GBOX *bounds;
int extent, buffer;
bool clip_geom;
LWGEOM *lwgeom_in, *lwgeom_out;
uint8_t type = 0;

if (PG_ARGISNULL(0))
{
PG_RETURN_NULL();
geom_in = PG_GETARG_GSERIALIZED_P_COPY(0);
lwgeom_in = lwgeom_from_gserialized(geom_in);
}

if (PG_ARGISNULL(1))
{
elog(ERROR, "%s: parameter bounds cannot be null", __func__);
bounds = (GBOX *) PG_GETARG_POINTER(1);
PG_RETURN_NULL();
}
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__);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? why not just return null?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PG_RETURN_NULL();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

PG_RETURN_NULL();
}

buffer = PG_ARGISNULL(3) ? 256 : PG_GETARG_INT32(3);
clip_geom = PG_ARGISNULL(4) ? true : PG_GETARG_BOOL(4);
// NOTE: can both return in clone and in place modification so
// not known if lwgeom_in can be freed

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* We don't check COLLECTIONTYPE since that might be a collection of points */
if (type == LINETYPE || type == POLYGONTYPE || type == MULTILINETYPE || type == MULTIPOLYGONTYPE)
{
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, _peek is only used in trivial cases:

postgis/liblwgeom/g_serialized.c

Line 639 in d1e5a63

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

postgis/liblwgeom/lwgeom.c

Line 1181 in 682db5d

int lwgeom_needs_bbox(const LWGEOM *geom)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
/* Shortcut to drop geometries smaller than the resolution */
double geom_width = gserialized_box.xmax - gserialized_box.xmin;
double geom_height = gserialized_box.ymax - gserialized_box.ymin;
double geom_area = geom_width * geom_height;

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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 */

double min_resolution_area = bounds_width * bounds_height / 2.0;

if (geom_area < min_resolution_area)
{
PG_RETURN_NULL();
}
}
}

lwgeom_in = lwgeom_from_gserialized(geom_in);

lwgeom_out = mvt_geom(lwgeom_in, bounds, extent, buffer, clip_geom);
if (lwgeom_out == NULL)
PG_RETURN_NULL();

geom_out = geometry_serialize(lwgeom_out);
lwgeom_free(lwgeom_out);
PG_FREE_IF_COPY(geom_in, 0);
Expand Down
16 changes: 0 additions & 16 deletions postgis/mvt.c
View file
Open in desktop
Expand Up @@ -845,28 +845,12 @@ LWGEOM *mvt_geom(LWGEOM *lwgeom, const GBOX *gbox, uint32_t extent, uint32_t buf
if (lwgeom_is_empty(lwgeom))
return NULL;

if (width == 0 || height == 0)
elog(ERROR, "mvt_geom: bounds width or height cannot be 0");

if (extent == 0)
elog(ERROR, "mvt_geom: extent cannot be 0");

resx = width / extent;
resy = height / extent;
res = (resx < resy ? resx : resy)/2;
fx = extent / width;
fy = -(extent / height);

if (basic_type == LINETYPE || basic_type == POLYGONTYPE)
{
// Shortcut to drop geometries smaller than the resolution
const GBOX *lwgeom_gbox = lwgeom_get_bbox(lwgeom);
double bbox_width = lwgeom_gbox->xmax - lwgeom_gbox->xmin;
double bbox_height = lwgeom_gbox->ymax - lwgeom_gbox->ymin;
if (bbox_height * bbox_height + bbox_width * bbox_width < res * res)
return NULL;
}

/* Remove all non-essential points (under the output resolution) */
lwgeom_remove_repeated_points_in_place(lwgeom, res);
lwgeom_simplify_in_place(lwgeom, res, preserve_collapsed);
Expand Down
13 changes: 13 additions & 0 deletions regress/core/mvt.sql
View file
Open in desktop
@@ -1,3 +1,10 @@
-- Input validation
select 'I1', ST_AsMVTGeom(NULL, ST_MakeEnvelope(10, 10, 20, 20), 4096);
select 'I2', ST_AsMVTGeom(ST_Point(1, 2), NULL, 4096);
select 'I3', ST_AsMVTGeom(ST_Point(1, 2), ST_MakeBox2D(ST_Point(0, 0), ST_Point(0, 0)));
select 'I4', ST_AsMVTGeom(ST_Point(1, 2), ST_MakeEnvelope(10, 10, 20, 20), -10);
select 'I5', ST_AsMVTGeom(ST_Point(1, 2), ST_MakeEnvelope(10, 10, 20, 20), 0);

-- geometry preprocessing tests
select 'PG1', ST_AsText(ST_AsMVTGeom(
ST_Point(1, 2),
Expand Down Expand Up @@ -281,6 +288,12 @@ SELECT 'PG46', St_AsEWKT(ST_AsMVTGeom(
16,
true));

-- Geometry fastpath
SELECT 'PG47', ST_AsText(ST_AsMVTGeom(
ST_GeomFromText('LINESTRING(0 0, 0 4, 4 4, 4 0, 0 0)'),
ST_MakeBox2D(ST_Point(0, 0), ST_Point(1000, 1000)),
100, 0, false));

-- geometry encoding tests
SELECT 'TG1', encode(ST_AsMVT(q, 'test', 4096, 'geom'), 'base64') FROM (SELECT 1 AS c1,
ST_AsMVTGeom(ST_GeomFromText('POINT(25 17)'),
Expand Down
6 changes: 6 additions & 0 deletions regress/core/mvt_expected
View file
Open in desktop
@@ -1,3 +1,8 @@
I1|
ERROR: ST_AsMVTGeom: parameter bounds cannot be null
ERROR: ST_AsMVTGeom: bounds width or height cannot be 0
ERROR: ST_AsMVTGeom: extent cannot be 0
ERROR: ST_AsMVTGeom: extent cannot be 0
PG1|POINT(1 4094)
PG2|POINT(0 4095)
PG3|POINT(2 4092)
Expand Down Expand Up @@ -49,6 +54,7 @@ PG43 - OFF|MULTIPOLYGON(((5 5,-1 -1,11 -1,5 5)),((5 5,11 11,-1 11,5 5)))
PG44|
PG45|
PG46|SRID=3857;MULTIPOLYGON(((3245 2224,3262 2030,3253 2158,3245 2224)))
PG47|
TG1|GiEKBHRlc3QSDBICAAAYASIECTLePxoCYzEiAigBKIAgeAI=
TG2|GiMKBHRlc3QSDhICAAAYASIGETLePwIBGgJjMSICKAEogCB4Ag==
TG3|GiYKBHRlc3QSERICAAAYAiIJCQCAQArQD88PGgJjMSICKAEogCB4Ag==
Expand Down