From b7bcc73c13f4a2a0c9e5c271b3bead6d2bcd1657 Mon Sep 17 00:00:00 2001 From: Prayas Lashkari Date: Sat, 19 Sep 2026 15:49:32 -0400 Subject: [PATCH] fix(engine): bound the flowline layer at both ends GET_FLOWLINE_GEOMETRIES traced a one-sided transitive closure: it seeded from the cells of every resolved anchor, widened each by sfTouches, and followed the network to its end. Nothing in the query referenced the targets, so an anchor sitting near a drainage divide pulled in the whole of the neighbouring basin. On "What facilities are upstream from PFOS samples in York County, ME" that drew 122 segments of the Merrimack and 21 of the Winnipesaukee, ~130km away in a basin no York sample drains from, plus the Presumpscot, the Suncook and the Powwow. Nothing looked broken. The map just had extra rivers on it, and they were real rivers. The closure is now intersected with the flowlines that reach the resolved targets. Measured live with the real answer sets (1,497 facilities, 200 sample points): 3,271 flowlines down to 2,516, in the same 7s. 755 removed, 0 added, so the fix can only take away flowlines that were never on a path from a facility to a sample. Of those, 684 are in a different basin and 71 lie below a sample, which is the one visible behaviour change: the drawn river now stops at the sample instead of running on to the sea. Written as an intersection of two DISTINCT closures. The direct membership test, `?flowline hyf:downstreamFlowPathTC? ?_flTarget`, leaves ?flowline unbound on the left and QLever times out joining on it (31s, "Join on ?flowline"). The reach is a bare TC rather than reflexive: `TC?` was measured and recovers 0 flowlines while costing 8s. The bounded branch gets the same intersection. At 25km it went 3,034 -> 2,516, matching the unbounded set, because connectivity dominates the distance cap at that range; at 5km the cap still bites (2,314), so the two constraints compose. The shipped bounded query still drew a Merrimack segment through its cap. scripts/check-flowline-scope.mts asserts both ends are bound for all 100 shapes that draw the layer, bounded and unbounded, and runs in CI. --- .github/workflows/checks.yml | 3 + package.json | 1 + scripts/check-flowline-scope.mts | 94 ++++++++++++++++++++++++++++ src/engine/planner.ts | 4 ++ src/engine/templates/fusedQueries.ts | 64 ++++++++++++++++++- 5 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 scripts/check-flowline-scope.mts diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 38cc134..c7c38c8 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -42,6 +42,9 @@ jobs: - name: Trace direction run: npm run check-trace-direction + - name: Flowline scope + run: npm run check-flowline-scope + - name: Cache key run: npm run check-cache-key diff --git a/package.json b/package.json index 32ff0f9..80ec5ce 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,7 @@ "check-cache-key": "tsx scripts/check-cache-key.mts", "check-step-labels": "tsx scripts/check-step-labels.mts", "check-trace-direction": "tsx scripts/check-trace-direction.mts", + "check-flowline-scope": "tsx scripts/check-flowline-scope.mts", "check-substance-labels": "tsx scripts/check-substance-labels.mts", "check-query-joins": "tsx scripts/check-query-joins.mts", "query-matrix": "tsx scripts/query-matrix.mts", diff --git a/scripts/check-flowline-scope.mts b/scripts/check-flowline-scope.mts new file mode 100644 index 0000000..cd618f2 --- /dev/null +++ b/scripts/check-flowline-scope.mts @@ -0,0 +1,94 @@ +// Self-check: the supporting flowline layer must be bounded at BOTH ends. +// +// npx tsx scripts/check-flowline-scope.mts +// +// buildFusedFlowlineQuery traces a transitive closure out of the anchor cells. +// Left one-sided it runs to the end of the network, so an anchor near a +// drainage divide drags in the neighbouring basin: "facilities upstream from +// PFOS samples in York County, ME" drew 122 segments of the Merrimack and 21 +// of the Winnipesaukee, in a basin no York sample drains from. Nothing looked +// broken -- the map just had extra rivers on it, and they were real rivers. +// +// The fix intersects that closure with the set of flowlines that actually +// reach the resolved targets. This asserts the intersection is present in +// every shape that draws the layer, bounded and unbounded, so a future edit to +// either branch cannot drop it silently. +// +// No network: the pipeline steps only build query strings. +import assert from 'node:assert/strict'; +import { planPipeline, type PipelineContext } from '../src/engine/planner'; +import type { AnalysisQuestion, EntityType, SpatialRelationship } from '../src/types/query'; + +const ENTITY_TYPES: EntityType[] = [ + 'samples', + 'facilities', + 'waterBodies', + 'wells', + 'aquifers', + 'streams', +]; +const HYDROLOGY: SpatialRelationship['type'][] = ['downstream', 'upstream']; +// Unbounded, and one bounded value to cover the aggregate branch. +const DISTANCES: (number | undefined)[] = [undefined, 25]; + +const context = (): PipelineContext => ({ + question: {} as AnalysisQuestion, + targetIris: ['https://example.org/t1', 'https://example.org/t2'], + anchorIris: ['https://example.org/a1', 'https://example.org/a2'], + results: {}, +}); + +let checks = 0; +let drawn = 0; + +for (const blockA of ENTITY_TYPES) { + for (const blockC of ENTITY_TYPES) { + for (const rel of HYDROLOGY) { + for (const maxDistanceKm of DISTANCES) { + const question: AnalysisQuestion = { + blockA: { type: blockA }, + relationship: { type: rel, maxDistanceKm }, + blockC: { type: blockC }, + }; + const step = planPipeline(question).find((s) => s.type === 'GET_FLOWLINE_GEOMETRIES'); + // Skipped when a side is streams: those flowlines are the answer set. + if (!step) { + assert.ok( + blockA === 'streams' || blockC === 'streams', + `${blockA} ${rel} ${blockC}: no flowline step, but neither side is streams`, + ); + continue; + } + drawn++; + const sparql = step.buildQuery(context()); + const where = `${blockA} ${rel} ${blockC} ${maxDistanceKm ?? 'unbounded'}`; + + // The target IRIs have to reach the query, or the closure is one-sided. + assert.ok( + sparql.includes('https://example.org/t1'), + `${where}: flowline query does not bind the resolved targets`, + ); + // Both ends present: a closure out of the anchor cells, and a closure + // into the targets, joined on ?flowline. + assert.ok( + sparql.includes('?_flTarget spatial:connectedTo ?s2celltarget'), + `${where}: missing the target reach clause`, + ); + assert.match( + sparql, + /\?flowline hyf:downstreamFlowPathTC \?_flTarget|\?_flTarget hyf:downstreamFlowPathTC \?flowline/, + `${where}: target reach clause does not close on ?flowline`, + ); + // The reach clause must be a bare TC. `TC?` was measured: it recovers + // 0 flowlines and costs 8s on the York question. + assert.ok( + !sparql.includes('downstreamFlowPathTC? ?_flTarget'), + `${where}: reflexive target reach, measured to add nothing`, + ); + checks += 4; + } + } + } +} + +console.log(`flowline scope: ${drawn} shapes draw the layer, ${checks} checks passed`); diff --git a/src/engine/planner.ts b/src/engine/planner.ts index b1469f0..129cbcc 100644 --- a/src/engine/planner.ts +++ b/src/engine/planner.ts @@ -328,8 +328,12 @@ function buildFusedSteps(question: AnalysisQuestion): PipelineStep[] { buildQuery: (ctx, scope) => buildFusedFlowlineQuery({ anchor: anchorBlock, + target: targetBlock, direction: traceDirection, anchorIris: scope?.anchorIris ?? ctx.anchorIris, + // Never sliced: the target set bounds the trace, so a partial list + // would silently shrink the drawn network rather than split it. + targetIris: ctx.targetIris, maxDistanceKm: relationship.maxDistanceKm, }), initialScopes: (ctx) => iriScopes(ctx.anchorIris, 'anchor'), diff --git a/src/engine/templates/fusedQueries.ts b/src/engine/templates/fusedQueries.ts index ae66adb..9e74260 100644 --- a/src/engine/templates/fusedQueries.ts +++ b/src/engine/templates/fusedQueries.ts @@ -581,11 +581,63 @@ export function buildFusedWellQuery(opts: FusedWellSideOpts): string { export interface FusedFlowlineOpts { anchor: EntityBlock; + target: EntityBlock; direction: 'downstream' | 'upstream'; anchorIris: string[]; + targetIris: string[]; maxDistanceKm?: number; } +// Restricts the traced closure to flowlines that actually connect the anchors +// to the targets. Without it the trace is one-sided: it runs from the anchor +// cells to the end of the network, so an anchor sitting near a drainage divide +// drags in the whole of the neighbouring basin. On "facilities upstream from +// PFOS samples in York County, ME" that drew 122 segments of the Merrimack and +// 21 of the Winnipesaukee, ~130km away in a basin no York sample drains from. +// +// Written as an intersection of two DISTINCT closures rather than the direct +// `?flowline hyf:downstreamFlowPathTC? ?_flEnd` membership test: that form +// leaves ?flowline unbound on the left and QLever times out joining on it +// (31s, "Join on ?flowline"). The intersection runs in the same 7s the +// one-sided query took. +// +// The closure is strict on both ends, so the segment a target sits on is not +// drawn. Making the target side reflexive (`TC?`) was measured and recovers +// nothing (0 extra flowlines) while costing 8s, because that segment already +// arrives via another target upstream of it. +function targetReachClause(opts: FusedFlowlineOpts): string { + // ponytail: target IRIs inlined whole. Only anchors are sliced today + // (iriScopes/divideIriList in the planner); add target slicing if a question + // ever pushes this past the gateway's body limit. + const targetValues = `VALUES ${entityIriVar(opts.target, 'C')} { ${opts.targetIris + .map(wrapUri) + .join(' ')} }`; + // Filters stripped: the pinned IRIs are already the filtered answer set, so + // every filter clause here is a join that can only re-confirm what the VALUES + // list states. Passing the bare block rather than opts.target keeps this to + // the cell-membership triple. Measured on the York question: re-deriving the + // substance filter cost 1s of the 8 and changed nothing. + const targetBind = bindEntityInCell({ type: opts.target.type }, '?s2celltarget', 'C', false); + const reach = + opts.direction === 'downstream' + ? `?flowline hyf:downstreamFlowPathTC ?_flTarget .` + : `?_flTarget hyf:downstreamFlowPathTC ?flowline .`; + + return `{ + SELECT DISTINCT ?flowline WHERE { + { + SELECT DISTINCT ?s2celltarget WHERE { + ${targetValues} + ?s2celltarget rdf:type kwg-ont:S2Cell_Level13 . + ${targetBind} + } + } + ?_flTarget spatial:connectedTo ?s2celltarget . + ${reach} + } + }`; +} + // Returns flowline geometries traced from the anchor entities the pipeline // already resolved in FIND_ANCHOR_IRIS. // @@ -624,13 +676,20 @@ export function buildFusedFlowlineQuery(opts: FusedFlowlineOpts): string { spatial:connectedTo ?s2cellus . ?flowline hyf:downstreamFlowPathTC ?downstream_flowline .`; + const reachesTarget = targetReachClause(opts); + // Unbounded: the flowline set is the plain transitive closure. if (!opts.maxDistanceKm) { return ` ${PREFIXES} SELECT DISTINCT ?flowline ?flowlineWKT ?fl_type ?streamName WHERE { - ${seedCells} - ${flowlinePattern} + { + SELECT DISTINCT ?flowline WHERE { + ${seedCells} + ${flowlinePattern} + } + } + ${reachesTarget} ?flowline geo:hasGeometry/geo:asWKT ?flowlineWKT ; nhdplusv2:hasFTYPE ?fl_type . OPTIONAL { ?flowline rdfs:label ?streamName } @@ -682,6 +741,7 @@ export function buildFusedFlowlineQuery(opts: FusedFlowlineOpts): string { BIND(xsd:float(?_plen) + xsd:float(?_extra) AS ?_ptotal) } GROUP BY ?flowline } + ${reachesTarget} ?flowline geo:hasGeometry/geo:asWKT ?flowlineWKT ; nhdplusv2:hasFTYPE ?fl_type . OPTIONAL { ?flowline rdfs:label ?streamName }