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

Convert springs to use ST_PointOnSurface and reformat SQL #3233

Merged
merged 1 commit into from
Jan 11, 2019
Merged

Convert springs to use ST_PointOnSurface and reformat SQL #3233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions project.mml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,14 +285,22 @@ Layer:
<<: *osm2pgsql
table: |-
(SELECT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does wrapping this in a superquery accomplish anything at all? It seems to me that leaving lines 287-290 and 302 out would yield exactly the same result?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, since then the query would return all polygons regardless of their natural column.

way, "natural"
FROM planet_osm_polygon
WHERE "natural" IN ('spring')
UNION ALL
SELECT
way, "natural"
FROM planet_osm_point
WHERE "natural" IN ('spring')
way,
"natural"
FROM
(SELECT
ST_PointOnSurface(way) AS way,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: is psql smart enough to only do the ST_PointOnSurface calculation for objects satisfying "natural" IN ('spring'), in this query?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see now it avoids the duplication of WHERE "natural" IN ('spring').

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case the conditions on the tag columns are simple enough a subselect isn't necessary, but on longer queries, these can be over a dozen lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, clear. And what about the question above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Postgres can do the query either way, the results are the same. It will pick whichever is cheapest, which will normally be using the index, filtering by geometry, then calculating st_pointonsurface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So if I understand you correctly, in this query it will also calculate ST_PointOnSurface for polygons that are not natural=spring, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no guarantee one way or the other. It's unlikely to, but with a small database and the right indexes it might.

"natural"
FROM planet_osm_polygon
WHERE way && !bbox!
UNION ALL
SELECT
way,
"natural"
FROM planet_osm_point
WHERE way && !bbox!
) _
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like unwanted space + underscore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would you suggest as an alternative for what to call the inner subselect? _ is a reasonably standard name when you just need a name. s, p, q are also common throw-away names.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know, however I don't like such shortcuts in the big SQL section of a public project. Typical i for loop index might be OK in typical code, but this one is already hard to maintain, underscore is worse than any letter and really looks like a typo, and you can't expect that other people will be real programmers to catch it. Something self-commenting like inner_subselect would be probably good.

BTW: there are more undocumented conventions and sections that I don't understand, it might make sense to add more comments one day.

Copy link
Collaborator

Choose a reason for hiding this comment

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

inner_subselect as a name would do for me.

WHERE "natural" IN ('spring')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we not add another WHERE way && !bbox! in the outer subquery? Because now it will also forward points to Mapnik that are outside of the bounding box that is currently being rendered (as long as they are the label position of a polygon that falls within that bounding box).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could, but the PR is no different than current behavior. I suppose the difference is that filtering on contains is now cheap to do in-db

) AS springs
properties:
minzoom: 14
Expand Down
1 change: 0 additions & 1 deletion water-features.mss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@
#springs {
[natural = 'spring'][zoom >= 14] {
marker-file: url('symbols/spring.svg');
marker-placement: interior;
marker-clip: false;
}
}