Make queries per-table by rmarianski · Pull Request #1352 · tilezen/vector-datasource

rmarianski · 2017-07-28T14:37:44Z

Connects to https://github.com/mapzen/tile-tasks/issues/267

nvkelso · 2017-07-31T17:08:32Z


 # small campground in point zoom 13
 # http://www.openstreetmap.org/way/417405356
+# min zoom matches in landuse layer, but not in pois for 13


Is there anything left for us here? If not, take out the comment to reduce confusion.

Removed in f703a2c.

nvkelso · 2017-07-31T17:22:22Z

    area-inclusion-threshold: 1
  buildings:
    template: buildings.jinja2
-    start_zoom: 13


Where is this handled going forward, or is it only on a per-feature min_zoom basis now?

In this particular case, per-feature min_zoom. The sources themselves now have a start zoom for when querying should begin, but there are now cases like this one where we are relying more on the min zoom for filtering.

nvkelso · 2017-07-31T17:22:50Z

    area-inclusion-threshold: 1
  pois:
    template: pois.jinja2
-    start_zoom: 4


This will probably yield a few more park labels at zooms 2 and 3.

If that's the case, I think we should update the yaml to clamp these values. Should we do it in this pr? Or a subsequent one?

It's always been an undocumented bug/feature that they don't show up earlier. If it turns out to be not ideal we can file a followup issue for it.

nvkelso · 2017-07-31T17:23:02Z

    area-inclusion-threshold: 1
  landuse:
    template: landuse.jinja2
-    start_zoom: 4


This will probably yield a few more park polygons at zooms 2 and 3.

(Which is fine)

Ok, ditto on the clamp in yaml.

nvkelso · 2017-07-31T17:27:09Z

    area-inclusion-threshold: 1
  roads:
    template: roads.jinja2
-    start_zoom: 5


Looks like we source the Natural Earth scale_rank for min_zoom directly, and that starts at 3:

min_zoom: { col: scalerank }

So this change would introduce roads at zoom 3 unless paired with a clamp over in the roads layer YAML?

(I'd link but Github is mostly down for me, except these PR comments!)

Ok, I'd like to pair with a clamp over in roads yaml.

Let's followup in #1353 after this PR is merged.

nvkelso · 2017-07-31T17:28:53Z

    area-inclusion-threshold: 1
  transit:
    template: transit.jinja2
-    start_zoom: 5


The smallest feature min_zoom is already 5 so relying on feature min_zoom alone (not guarding here in the layer config) should be fine.

nvkelso · 2017-07-31T17:35:11Z

+   mz_transit_level IS NOT NULL OR
+   mz_water_min_zoom IS NOT NULL
+   )
+{% else %}


Suspect we need to have the old earth, road, water layer start_zoom at 8 logic near here now?

(Landuse was start 4 but it's fine at 0, should fix in layer min_zoom YAML if an issue.)

(Transit is already handled i the layer min_zoom YAML.)

Followup via #1354.

nvkelso · 2017-07-31T17:44:21Z

+       mz_transit_level IS NOT NULL OR
+       mz_water_min_zoom IS NOT NULL))
+{% else %}
+    (mz_boundary_min_zoom < {{ zoom + 1 }} AND {{ bounds['line']|bbox_overlaps('way', 3857) }}) OR


Suspect we need to have the old earth, road, water layer start_zoom at 8 logic near here now? (Road is probably overkill.)

(Landuse and POI was start 4 but it's fine at 0, should fix in layer min_zoom YAML if an issue.)

(Building and transit is already handled i the layer min_zoom YAML.)

Followup via #1354.

nvkelso · 2017-07-31T17:48:46Z

+     mz_poi_min_zoom IS NOT NULL OR
+     mz_water_min_zoom IS NOT NULL
+     )
+{% else %}


Suspect we need to have the old earth, road, water layer start_zoom at 8 logic near here now?

(Building, poi is already handled in the layer min_zoom YAML.)

(Places is mostly handled in the layer min_zoom YAML, and you've carried over the layer duplicate weeding logic into the new NE table logic so we're good there.)

Followup via #1354.

rmarianski · 2017-07-31T18:17:21Z

Going to merge this in for now. Let's make new issues for the follow ups.

nvkelso · 2017-07-31T18:23:41Z

Follow-up issues in #1353 and #1354.

nvkelso · 2017-08-01T18:18:39Z

Some other followup: #1356 and #1358 for JINJA to YAML migration.

Looks like this was done already as part of #1352, so this PR just updates the test to have the behaviour that we want; that all building geometry is within the tile bounds. Fixes #1142.

rmarianski force-pushed the query-by-table branch from 037193b to 5a4c47f Compare July 28, 2017 20:16

rmarianski added 10 commits July 28, 2017 16:28

Don't need kind parameter in landuse macro

fa09322

Add initial queries for osm point, line, polygon

ad64336

Add ne queries

3a321a0

Add wof queries

808dbfd

Add osmdata queries

0fc515b

Correct queries

e310dd8

Configure yaml to include sources

fd16045

Update test for new name behavior

fef62d3

Use padded intersection for polygon queries

b6b87d8

Add comment to test

5224533

rmarianski force-pushed the query-by-table branch from 5a4c47f to 5224533 Compare July 28, 2017 20:29

rmarianski mentioned this pull request Jul 28, 2017

Makes queries per-table tilezen/tilequeue#227

Merged

nvkelso requested review from nvkelso and zerebubuth July 28, 2017 22:18

Remove no longer used config and queries

3b66f85

zerebubuth approved these changes Jul 31, 2017

View reviewed changes

nvkelso reviewed Jul 31, 2017

View reviewed changes

Remove comment to prevent confusion

f703a2c

nvkelso reviewed Jul 31, 2017

View reviewed changes

rmarianski changed the title ~~WIP Make queries per-table~~ Make queries per-table Jul 31, 2017

rmarianski merged commit d4916fd into master Jul 31, 2017

rmarianski deleted the query-by-table branch July 31, 2017 18:17

This was referenced Jul 31, 2017

REGRESSION: clamp NE roads to min_zoom 5 #1353

Closed

REGRESSION: duplicate NE and OSM representations at low zooms #1354

Open

zerebubuth mentioned this pull request Dec 1, 2017

Clip buildings to tiles. #1446

Merged

nvkelso mentioned this pull request Apr 30, 2018

Organize queries by table #1130

Closed

Conversation

rmarianski commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvkelso Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmarianski commented Jul 31, 2017

Uh oh!

nvkelso commented Jul 31, 2017

Uh oh!

nvkelso commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nvkelso Jul 31, 2017 •

edited

Loading

nvkelso commented Aug 1, 2017 •

edited

Loading