Poland buildings R120 acceptance test is a marginal timeout on a healthy query #166

New issue

Open

opened 2026-08-19 06:38:41 +00:00 by art · 1 comment

art commented

2026-08-19 06:38:41 +00:00

Owner

The Poland buildings acceptance test is a marginal timeout on a healthy query

The test layer buildings (pl) / R120: every Poland source part becomes a classified footprint... (conf/plmap.yaml) consistently exceeds its 10 s timeout on this machine:
the query takes ~11 s (10.98 s measured), because sum(ST_NumGeometries(geom)) over
pl_bubd (Poland's largest staging table, 16.35 M parts) plus the three companion
subqueries is just too slow for the bucket. The data is correct — footprints == source
parts == 16 351 829, unclassified == 0 — so this is a timeout tuning problem, not an
import defect.

It surfaces in every make acceptance-all as the single red line and forces a reviewer
to check it isn't a regression. Options:

raise this test's timeout to 100 s (it is genuinely the heaviest query in the suite), or
precompute the source-part count once (the pl_bubd table is static per import) rather than
re-scanning 16 M geometries on every run.

Found while verifying issue #164, unrelated to that change.

The Poland buildings acceptance test is a marginal timeout on a healthy query The test `layer buildings (pl) / R120: every Poland source part becomes a classified footprint...` (conf/plmap.yaml) consistently exceeds its 10 s timeout on this machine: the query takes ~11 s (10.98 s measured), because `sum(ST_NumGeometries(geom))` over `pl_bubd` (Poland's largest staging table, 16.35 M parts) plus the three companion subqueries is just too slow for the bucket. The data is correct — footprints == source parts == 16 351 829, unclassified == 0 — so this is a timeout tuning problem, not an import defect. It surfaces in every `make acceptance-all` as the single red line and forces a reviewer to check it isn't a regression. Options: - raise this test's `timeout` to 100 s (it is genuinely the heaviest query in the suite), or - precompute the source-part count once (the pl_bubd table is static per import) rather than re-scanning 16 M geometries on every run. Found while verifying issue #164, unrelated to that change.

art-bot commented

2026-08-19 22:40:32 +00:00

Collaborator

Reproduced while reviewing #168/#169 (it is not from either branch — it fails identically on unmodified main). Two details that change the options.

The margin is larger than the ~11 s recorded above. Given a 100 s budget the test passes in 16.9 s, with the numbers this issue quotes, so it is the same healthy query:

PASS (16.919s, 1 row(s), footprints=16351829, source_parts=16351829, unclassified=0)

Poland is 46/46 with that budget. Load moves the figure around, but 17 s against a 10 s bucket is not marginal — and it rules out a modest bump: raising the timeout to ~20 s would make the failure intermittent rather than fix it.

Option 1 (raise to 100 s) does not do what it looks like. fastAcceptanceTimeout is 10 and runsIn is mode == "all" || t.Timeout <= fastAcceptanceTimeout (importer/acceptance.go:74,84), so raising this test's timeout past 10 does not make it pass in make acceptance — it makes it stop running there, executing only under -acceptance all. Measured with timeout: 100:

layer buildings (pl) / R120: ... — not run (timeout 100s needs -acceptance all)
46 test(s): 44 passed, 0 failed, 0 skipped, 2 not run (needs -acceptance all)

That trades a red line a reviewer has to check for a check nobody sees, which is the worse of the two.

So option 2 is the one worth doing: the expensive half is sum(ST_NumGeometries(geom)) re-scanning 16.35 M geometries in pl_bubd, which is static per import. Either precompute that count at load time and compare against it, or split the test — a cheap always-on check on the footprint/unclassified counts inside the 10 s bucket, and the full source-part reconciliation as a separate -acceptance all test where a 100 s budget is honest about what it is.

Note for whoever picks this up: after #169 the test lives in conf/pl/15-buildings.yaml, not conf/plmap.yaml.

Reproduced while reviewing #168/#169 (it is not from either branch — it fails identically on unmodified `main`). Two details that change the options. **The margin is larger than the ~11 s recorded above.** Given a 100 s budget the test passes in **16.9 s**, with the numbers this issue quotes, so it is the same healthy query: ``` PASS (16.919s, 1 row(s), footprints=16351829, source_parts=16351829, unclassified=0) ``` Poland is 46/46 with that budget. Load moves the figure around, but 17 s against a 10 s bucket is not marginal — and it rules out a modest bump: raising the timeout to ~20 s would make the failure intermittent rather than fix it. **Option 1 (raise to 100 s) does not do what it looks like.** `fastAcceptanceTimeout` is 10 and `runsIn` is `mode == "all" || t.Timeout <= fastAcceptanceTimeout` (`importer/acceptance.go:74,84`), so raising this test's timeout past 10 does not make it pass in `make acceptance` — it makes it **stop running there**, executing only under `-acceptance all`. Measured with `timeout: 100`: ``` layer buildings (pl) / R120: ... — not run (timeout 100s needs -acceptance all) 46 test(s): 44 passed, 0 failed, 0 skipped, 2 not run (needs -acceptance all) ``` That trades a red line a reviewer has to check for a check nobody sees, which is the worse of the two. So **option 2** is the one worth doing: the expensive half is `sum(ST_NumGeometries(geom))` re-scanning 16.35 M geometries in `pl_bubd`, which is static per import. Either precompute that count at load time and compare against it, or split the test — a cheap always-on check on the footprint/unclassified counts inside the 10 s bucket, and the full source-part reconciliation as a separate `-acceptance all` test where a 100 s budget is honest about what it is. Note for whoever picks this up: after #169 the test lives in `conf/pl/15-buildings.yaml`, not `conf/plmap.yaml`.

art-bot referenced this issue from a commit

2026-08-20 07:48:55 +00:00

config: Greece's mask is a maritime outline — carve the gulfs, not just the land

art-bot referenced this issue

2026-08-20 07:58:06 +00:00

config: Greece pilot — Natural Earth carve (issue #172) #176

art-bot referenced this issue

2026-08-20 08:40:02 +00:00

world: carve Natural Earth with Natural Earth, not with our own country masks (#184) #185

art-bot referenced this issue from a commit

2026-08-20 08:43:17 +00:00

config: Greece's mask is a maritime outline — carve the gulfs, not just the land