tile tool cleanup: the #142 review's non-blocking items #144

New issue

Closed

opened 2026-08-16 22:36:58 +00:00 by art · 1 comment

art commented

2026-08-16 22:36:58 +00:00

Owner

Scope: importer/tiles.go (the #139 tile tool) and bench/results/ — the non-blocking
items from the #142 review (comment 1643, 2026-08-16). The review's three blocking-ish
items were already fixed on the #127 branch and are in main (docs naming deleted flags,
corner-vs-centroid docs, --check now asserts extent == mvtExtent and rejects
geometry-less features). These are the rest, all confirmed still open in main:

--size has no layer filter. --size-zooms exists but no --size-layer, so
measuring one layer runs all twelve, each a full-table pass. This is exactly the
#127 workflow the tool exists for (the reviewer ended up writing raw SQL instead).
Add --size-layer <name> (comma-separated, like import --layer).
candidateTiles has no LIMIT while probeTile samples 200 000 rows. It scans
the whole table for every layer at every zoom in the window — tiles --size conf/semap.yaml walks Sweden's 5.4M-row z13 cover partition. A LIMIT (or a
sensible cap) is needed; the weight ranking is a heuristic anyway.
A failed fetch is recorded as a 0-byte tile. sizeOneZoom appends
measured{cx, cy, 0, 0} on error and the top-N sort discards it silently. A
measurement tool should not turn an error into a plausible-looking zero — count
and report it the way --check's MISSING is.
The committed bench JSON is 475 904 B / 26 654 lines
(bench/results/2026-08-16_478f205_tiles.json); other records are 1–2 kB and
make tile-bench adds one per run. Either gitignore *_tiles.json (and remove
the committed one), drop the SetIndent, or commit the combo summaries and keep
per-tile detail local.
parseIntStrict is not strict: fmt.Sscanf(s, "%d", &n) stops at the first
non-digit and reports success, so --size-zooms 5x silently means 5. Reject
trailing garbage.
checkTileAtZoom re-implements tileTarget.tableAtZoom inline — the same
prefix derivation, ten lines below the method that already does it.
A static: true layer would be silently skipped. tileTargets only skips
l.Shared != nil; a static layer's tableName(maxz) has no _<z> suffix, so the
prefix derivation builds a nonexistent table name and the layer reports EMPTY
rather than being checked or deliberately skipped. Latent — no config declares one
today — but EMPTY is not fatal, so it would never be noticed.
Smaller things: decodeMVTTile never returns a non-nil error but three call
sites check it; probeTile computes n := math.Pow(2, z) and passes it to
Sprintf as an argument the format string never references; benchCombos calls
Sweden's whole-country probe stockholm-whole-country while the others are named
<country>-whole-country.

From the #142 review (comment 1643). All are correctness/hygiene on the new tool; none
changes the R121 check's semantics (which is already green on all five configs).

Scope: `importer/tiles.go` (the #139 tile tool) and `bench/results/` — the non-blocking items from the #142 review (comment 1643, 2026-08-16). The review's three blocking-ish items were already fixed on the #127 branch and are in main (docs naming deleted flags, corner-vs-centroid docs, `--check` now asserts extent == mvtExtent and rejects geometry-less features). These are the rest, all confirmed still open in main: 1. **`--size` has no layer filter.** `--size-zooms` exists but no `--size-layer`, so measuring one layer runs all twelve, each a full-table pass. This is exactly the #127 workflow the tool exists for (the reviewer ended up writing raw SQL instead). Add `--size-layer <name>` (comma-separated, like `import --layer`). 2. **`candidateTiles` has no LIMIT** while `probeTile` samples 200 000 rows. It scans the whole table for every layer at every zoom in the window — `tiles --size conf/semap.yaml` walks Sweden's 5.4M-row z13 cover partition. A LIMIT (or a sensible cap) is needed; the weight ranking is a heuristic anyway. 3. **A failed fetch is recorded as a 0-byte tile.** `sizeOneZoom` appends `measured{cx, cy, 0, 0}` on error and the top-N sort discards it silently. A measurement tool should not turn an error into a plausible-looking zero — count and report it the way `--check`'s MISSING is. 4. **The committed bench JSON is 475 904 B / 26 654 lines** (`bench/results/2026-08-16_478f205_tiles.json`); other records are 1–2 kB and `make tile-bench` adds one per run. Either gitignore `*_tiles.json` (and remove the committed one), drop the `SetIndent`, or commit the combo summaries and keep per-tile detail local. 5. **`parseIntStrict` is not strict**: `fmt.Sscanf(s, "%d", &n)` stops at the first non-digit and reports success, so `--size-zooms 5x` silently means `5`. Reject trailing garbage. 6. **`checkTileAtZoom` re-implements `tileTarget.tableAtZoom` inline** — the same prefix derivation, ten lines below the method that already does it. 7. **A `static: true` layer would be silently skipped.** `tileTargets` only skips `l.Shared != nil`; a static layer's `tableName(maxz)` has no `_<z>` suffix, so the prefix derivation builds a nonexistent table name and the layer reports EMPTY rather than being checked or deliberately skipped. Latent — no config declares one today — but EMPTY is not fatal, so it would never be noticed. 8. **Smaller things**: `decodeMVTTile` never returns a non-nil error but three call sites check it; `probeTile` computes `n := math.Pow(2, z)` and passes it to `Sprintf` as an argument the format string never references; `benchCombos` calls Sweden's whole-country probe `stockholm-whole-country` while the others are named `<country>-whole-country`. From the #142 review (comment 1643). All are correctness/hygiene on the new tool; none changes the R121 check's semantics (which is already green on all five configs).

art added the

claude-wip

label

2026-08-16 22:37:08 +00:00

art referenced this issue from a commit

2026-08-16 22:46:35 +00:00

#144: tile tool cleanup — the #142 review's non-blocking items

art referenced this issue from a commit

2026-08-16 22:46:35 +00:00

#144: commit the first summary-form tile benchmark record

art referenced this issue from a pull request that will close it,

2026-08-16 22:46:36 +00:00