feat(ppd): route subject-property history to the snapshot - #69
Merged
Conversation
`_subject_property_lookup` called the live client directly, bypassing routing.
The reason was real -- an eleven-year snapshot would truncate a property's
history, and a truncated history is indistinguishable from a complete one --
but coverage now runs from 1995, so the reason is gone. Leaving it live left the
feature broken whenever the live source was, which is how a lookup returned
nothing while the record sat in the artifact.
The risk was never the routing. It was the matching. Live filters paon/street
with CONTAINS(LCASE(...)), i.e. SUBSTRING, and the uniqueness guard downstream
exists BECAUSE that is over-broad: it refuses input spanning several buildings.
An exact-equality snapshot filter would look like a tightening while silently
changing behaviour -- the guard would stop firing and vague input would resolve
to one property instead of being refused.
So the adapter gains paon/saon/street as substring filters, matching live.
Measured against DuckDB 1.5.5 before writing it:
contains(lower(paon), lower('27')) -> ['127','27','27A'] like live
contains(...) with '%' -> [] literal
LIKE with '%' -> everything wildcard
Hence parameterised `contains()`, never a LIKE built from caller text: `%` and
`_` are wildcards there, so `paon='%'` would return every row on the street.
Pinned by test.
The filters are pushed into SQL, not applied after, or the adapter's limit+1
completeness basis would stop meaning anything -- a short page has to prove the
SOURCE was exhausted, not that our own post-filter discarded most of it.
Routing follows _fetch_comps exactly: try the adapter, catch SnapshotFailure,
append fallback_warning and fall through to live. GUARANTEED policy rather than
EXPLICIT, because this query names no dates and the honest answer to "the whole
history" is the whole coverage plus a warning saying so, not a refusal.
`resolve_coverage` already words that case well: "unbounded from_date narrowed
to snapshot coverage".
SubjectProperty keeps its own provenance block even now both halves are
snapshot-sourced. The sample limits and completeness bases still differ -- this
asks one property's whole history at limit 50, comps asks a bounded window at
its own limit -- so one label over both would still misstate one of them.
Two existing tests changed, both in setup rather than in what they assert:
* test_mixed_source_comps_declares_both asserted the subject half is SPARQL.
That split is what this removes; renamed and inverted, and it now also
asserts the two provenance blocks are still distinct objects.
* test_subject_property_failure_warns_and_success_does_not induced its two
conditions through fake_live, which the lookup no longer calls. A genuine
absence is now an address the snapshot does not hold, and a failure is the
snapshot query raising -- targeted at the subject query alone, since failing
every search would take comps down and test something else. The assertions
are unchanged: absence is silent, failure warns, neither is ever the other.
./scripts/validate.sh: 2208 passed, 28 skipped.
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
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.
Why
_subject_property_lookupcalled the live client directly, bypassing routing.The reason was real — an 11-year snapshot would truncate a property's history,
and a truncated history is indistinguishable from a complete one. Coverage now
runs from 1995, so the reason is gone, and leaving it live left the feature
broken whenever the live source was.
The risk was the matching, not the routing
Live filters
paon/streetwithCONTAINS(LCASE(...))— substring. Theuniqueness guard downstream exists because that is over-broad. An
exact-equality snapshot filter would look like a tightening while silently
changing behaviour: the guard would stop firing and vague input would resolve to
one property instead of being refused.
Measured against DuckDB 1.5.5 before writing the filter:
Hence parameterised
contains(), never a LIKE built from caller text —paon='%'would otherwise return every row on the street. Pinned by test.Filters are pushed into SQL, not applied after, or the
limit + 1completenessbasis stops meaning anything: a short page must prove the source was
exhausted, not that our own post-filter discarded most of it.
Routing
Follows
_fetch_compsexactly — try the adapter, catchSnapshotFailure, appendfallback_warning, fall through to live.GUARANTEEDrather thanEXPLICIT,because this query names no dates and the honest answer to "the whole history"
is the whole coverage plus a warning, not a refusal.
SubjectPropertykeeps its own provenance block even now both halves aresnapshot-sourced: the sample limits and completeness bases still differ, so one
label over both would misstate one of them.
Two existing tests changed — in setup, not in what they assert
test_mixed_source_comps_declares_bothasserted the subject half is SPARQL.That split is what this removes; renamed, inverted, and it now also asserts the
two provenance blocks remain distinct objects.
test_subject_property_failure_warns_and_success_does_notinduced bothconditions through
fake_live, which the lookup no longer calls. Absence isnow an address the snapshot doesn't hold; failure is the snapshot query
raising, targeted at the subject query alone. The assertions are unchanged
— absence is silent, failure warns, neither is ever the other.
A pre-existing defect this surfaced — not introduced here
Verifying against the real artifact,
5 Alexandra Roadstill returnsNone:DE12 6LL contains a
15 Alexandra Road,contains("5")matches it, and theguard correctly refuses rather than guessing.
Measured across six outcodes: 14% of addresses (1 in 7) are unresolvable
this way —
5/15,1/11/21,7/17.This PR does not change that rate. The live source always had full history
back to 1995, so it always saw
15 Alexandra Roadand always collided.Identical before and after; this work merely made it visible.
Worth fixing separately, and the fix is small: prefer an exact
paonmatchwhen one exists among the candidates. In every case of this class the exact
match is present — that's why it collided — so it resolves the whole 14%
without weakening the refusal-to-guess principle.
./scripts/validate.sh→ 2208 passed, 28 skipped.