fix challenge list location filtering on explore challenge queries - #1273
Merged
Merged
Conversation
CollinBeczak
force-pushed
the
collin/explore-location-filtering
branch
from
September 8, 2026 17:33
d55c0fa to
81b7e8f
Compare
A keyword names a tag on the *challenge*, but the queries matched it by joining `tags_on_challenges` into a per-task query. That returns every task of a challenge once per requested tag the challenge carries, so anything downstream that counts or clusters rows sees the same task several times. Demonstrated on a 166k-task database by giving an eligible challenge a second real keyword and asking for both: the join returns 22,430 rows for 11,215 tasks, the semi-join 11,215. Any challenge tagged with two of the selected categories is enough, which is ordinary -- challenges routinely carry several keywords and the explore UI lets more than one be selected. What the duplication reached: - Cluster counts on the keyword-filtered explore map were inflated, and cluster centroids were pulled toward multi-tagged challenges, since the duplicate rows inflate the centroid weight as well as the count. - At zoom 12, where features are grouped by location, a task counted twice reported 2 and rendered as an overlap stack rather than a single clickable task, with its id repeated in `task_ids_str`. - queryTaskMarkersWithOverlaps returned each task once per matching tag, and since it groups by location with DBSCAN, the duplicates of one task became a phantom overlap group. exploreChallenges had the same join, hidden behind a SELECT DISTINCT. It moves to the semi-join and drops the DISTINCT, which nothing else needs: the only other join there is many-to-one on the parent project. That also lifts a restriction the DISTINCT imposed -- Postgres requires every ORDER BY expression to appear in the select list under SELECT DISTINCT, so a sort could not order by an expression. The other three keyword call sites in TaskClusterRepository were already protected by SELECT DISTINCT / COUNT(DISTINCT), so they were correct; they move to the semi-join too, to leave one spelling of the question in the file. This is a correctness fix, not a performance one -- the two forms measure the same. Best of 3 at zoom 0 on that database: 9.19ms vs 9.17ms for a keyword matching one challenge of 31, and 38.0ms vs 36.0ms where the join was duplicating every row. Cost there is dominated by the scan over tasks, which neither form avoids at low zoom. Not covered: ProjectRepository.getSearchedClusteredPoints joins the tag tables the same way with no DISTINCT, so it can still return a challenge's clustered point once per matching tag.
A reviewer claiming a bundle locked each member task individually, which left one `locked` row per task. Locking.lockBundle already models a bundle as one row on the primary task with the members in `bundled_tasks`, so the two representations disagreed and the singleOpt lookups in resolveLockHolder/resolveLockBundle could see two rows covering the same task. - claimTaskReview now takes the whole list through lockBundle with reviewClaim = true, so one row covers the bundle. Review claims stay exempt from the one-lock-per-user invariant, which is what the new reviewClaim parameter on lockBundle carries through. - lockBundle checks every task the row will cover, not just the primary, and folds any of our own overlapping rows into that one row. Without this a claim over tasks already covered by another of our rows would leave two rows covering the same task. - claimTaskReview drops leftover rows from a previous claim. The unclaim it runs first only clears task_review, so those rows accumulated and kept their tasks locked. - unclaimTaskReview clears the claim on every member of the bundle, since releasing the single row releases them all, and runs its unlock inside the surrounding transaction. - A lock conflict response now carries parentId alongside parentName, so a client can link to the challenge holding the conflicting lock.
Tile clusters were the grid bins themselves, so a cluster's position was its cell centroid and its size was fixed by the grid. Neighbouring cells holding a handful of tasks each stayed separate markers no matter how far apart their tasks actually were. The repository now runs k-means over the cells feeding a tile and returns the resulting clusters, so markers land on where the tasks are and nearby cells merge into one cluster instead of sitting side by side. A separation pass then collapses centroids closer together than 25 screen pixels, which is what makes zooming out consolidate clusters: k-means returns exactly k clusters however tightly packed its input is, so k is only a ceiling. Evolution 121 coarsens the grid to match: CELL_BITS 4 -> 3, moving the leaf level from slippy zoom 15 to 14 so a display tile holds 8x8 = 64 cells instead of 16x16 = 256, each cell twice as wide. Only the cell<->slippy-zoom mapping changes -- the roll-up is untouched -- so the leaf functions from evolution 107 are redefined at zoom 14 and the pyramid is rebuilt. The Downs restores the zoom 15 leaf and rebuilds again. Cluster weight is the filtered count, not the cell total. A cell's stored sums cover every task in it, so its centroid is the right position either way, but carrying the total into the merge let a cell drag a cluster in proportion to the tasks the filter had just excluded -- a marker reporting a handful of expert tasks could sit on top of a thousand easy ones. This did not arise before, when every cell was its own marker and a mis-weighting could not cross cell boundaries. Measured against the base tables on a 166k-task database under a difficulty filter, mean cluster displacement drops from 563m to 2m at z=8 (worst 2345m -> 4m) and from 68m to 3m at z=11 (worst 298m -> 12m). A new `clusterMarkers` shares the whole src -> k-means -> separation chain with the MVT paths and returns the markers as plain numbers, so a test can assert where one landed without decoding protobuf. The repeated-request test now seeds more cells than MAX_CLUSTERS, since at or below the ceiling k-means returns one cluster per input and the clustering step is an identity.
CollinBeczak
force-pushed
the
collin/explore-location-filtering
branch
from
September 8, 2026 17:59
81b7e8f to
a577821
Compare
Tile clusters were the grid bins themselves, so a cluster's position was its cell centroid and its size was fixed by the grid. Neighbouring cells holding a handful of tasks each stayed separate markers no matter how far apart their tasks actually were, and dense regions came out looking like graph paper. The pyramid becomes the *input* to clustering rather than the output. A tile reads micro-aggregates from a level below its display zoom, runs k-means over them in Web Mercator, then merges any centroids closer together than 25 screen pixels. Markers follow the data instead of the grid, and cluster extent adapts to local density. The merge is what makes zooming out behave: k-means returns exactly k clusters however tightly packed its input is, so k is only a ceiling and consolidation comes from the separation pass. Read depth is DETAIL_BITS = 2 rather than coarsening the grid. Both would bound the k-means input at 4096 cells per tile, but the pyramid stops at MAX_CELL_ZOOM, so near the top the display zoom eats into the depth available and a tile falls back on CELL_BITS alone. Leaving CELL_BITS at 4 keeps z=11 at 256 cells of 16px, where k-means still has more input than MAX_CLUSTERS to partition and the cells are finer than the merge distance. Coarsening to CELL_BITS = 3 would have left 64 cells of 32px there: k equal to the input size makes k-means an identity, and cells wider than the merge distance make the separation pass a no-op, putting the top cluster zoom back to one marker per grid cell. Tuning the read depth instead needs no migration and no pyramid rebuild -- level z+2 already exists. Cluster weight is the filtered count, not the cell total. A cell's stored sums cover every task in it, so its centroid is the right position either way, but carrying the total into the merge let a cell drag a cluster in proportion to the tasks the filter had just excluded -- a marker reporting a handful of expert tasks could sit on top of a thousand easy ones. This did not arise before, when every cell was its own marker and a mis-weighting could not cross cell boundaries. Measured against the base tables on a 166k-task database under a difficulty filter, mean cluster displacement drops from 563m to 2m at z=8 (worst 2345m -> 4m) and from 68m to 3m at z=11 (worst 298m -> 12m). A new `clusterMarkers` shares the whole src -> k-means -> separation chain with the MVT paths and returns the markers as plain numbers, so a test can assert where one landed without decoding protobuf. The repeated-request test seeds more cells than MAX_CLUSTERS, since at or below the ceiling k-means returns one cluster per input and the clustering step is an identity.
A paused challenge's tasks cannot be locked, completed or reviewed until it is resumed, and a finished challenge has no tasks left at all. Both still showed up as available work. - Paused challenges drop out of all four tile paths: this repository's live MVT queries, and the cached pyramid's rebuild_leaf_cell and rebuild_all_tile_cells (evolution 122). That evolution also widens the challenge dirty-marking trigger from evolution 107 to fire on `paused`, without which a pause never reaches the pyramid, and marks the cells of already-paused challenges stale so the scheduled drain recomputes them rather than rebuilding everything. - Paused challenges also drop out of the task cluster queries and out of exploreChallenges, which additionally omits STATUS_FINISHED challenges. NULL status predates the column and counts as unfinished. - Locking a task or a bundle in a paused challenge is rejected: there is no work to hold a lock for. - Evolution 121 reconciles challenges left at READY while showing 100% complete. updateFinishedStatus keeps status in sync as tasks are worked, but paths that never run it (bulk status changes, deletions, restores) could leave a done challenge listed as work.
exploreChallenges filtered on challenges.bounding, the envelope of every task in a challenge. A challenge with tasks on two continents overlaps nearly any box, so searching Wichita returned USA-wide challenges. - The envelope is now only an index prefilter. A challenge matches when it has a task inside the requested area, checked with an EXISTS over tasks. - The POST form of the route takes a GeoJSON Polygon/MultiPolygon as `polygon` in the body and ANDs it with `bounds` on the same task, so a match has to be both in view and inside the place rather than in either. The geometry travels in the body because a city boundary from Nominatim runs to tens of kilobytes, past any practical URL length; the route parses with its own 2MB limit and rejects a non-polygon geometry with a 400. - `global` now defaults to false, matching taskTilesMvt and the UI toggle, which rendered "off" while the list still included global challenges. - sortBy accepts featured, tag_fix and cooperative. These group a kind of challenge to the front rather than ordering by the column, so each falls back to name to keep the ordering inside a group stable across pages.
CollinBeczak
force-pushed
the
collin/explore-location-filtering
branch
from
September 8, 2026 18:54
a577821 to
d97b5f0
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split from #1268
dependent on: #1274