Skip to content

fix(query): refuse cross-type multi-hop named traversals - #885

Merged
ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:claude/sharp-lamarr-a0f62a
Oct 8, 2026
Merged

ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:claude/sharp-lamarr-a0f62a

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

What & why

A named traversal whose hop bound allows more than one hop is now refused at
type check when its edge connects two different node types:

$p worksAt{1,2} $c   -- WorksAt: Person -> Company
type error: T5: multi-hop traversal `worksAt{1,2}` requires the same node type at both endpoints, but `WorksAt: Person -> Company` connects different types, so no path continues past one hop
  fix: follow one hop without a bound: `$p worksAt $c`

Before this change, the query type-checked and ran as {1,1}. The planner's
cost model (cost_effective_hops) and the engine BFS (execute_expand_bfs)
both capped a cross-type expand at one hop, so the bound was dropped without
a word. That breaks invariant 8, which requires failures to be loud. A hop
ends on the destination type and the next must start on the source type, so
no such path continues past one hop. An omitted bound and an explicit {1,1}
stay valid.

One rule now covers every traversal form. Edge selections were already
refused under it (recursive edge selection requires the same node type at both endpoints, T5); named edges are now refused the same way.

  • Compiler (typecheck.rs): the check that refused cross-type multi-hop
    selections now covers named edges. Named edges get a specific message that
    names the edge declaration, plus a fix. The selection message is
    unchanged.
  • Engine plan validation (validate_expand_structure, called from both
    ExpandStep::validate and validate_traversal_admission): the cross-type
    multi-hop refusal also covers named expands. A plan that skipped the type
    checker, such as a replayed BoundPlan, is now refused instead of capped.
  • Dead caps removed: with both checks refusing, nothing valid reaches the
    one-hop caps. cost_effective_hops, the same_type parameter of
    executed_hops and ExpandStatistics::same_type are gone, and the BFS runs
    step.max_hops directly. No query that type-checks gets different cost
    inputs: a cross-type traversal always has max_hops == 1 there.

Code: T5, not a new number. This rule is already published as T5 for edge
selections, and T5 holds the other traversal endpoint-type refusals
(mixed-orientation alternation, undeclared wildcard endpoints). A new code
would split one rule across two codes. Moving the selection refusal to a new
code would change a published code's meaning, which codes.rs freezes. T22,
the undirected same-endpoint-type rule, already has its own code.

GQ language version: no bump. GQ_LANGUAGE_VERSION versions the grammar.
Its hash units are the rules of query.pest (compatibility-surfaces RFC),
and a minor bump means the grammar "only accepts more". This change touches
no grammar rule: worksAt{1,2} still parses. It narrows what the type checker
accepts, so a minor bump would misclassify it. The compatibility RFC puts
language behaviour like this in the versionless gq_cases record, which it
says "moves with every bug fix". Precedent: #870 refused previously accepted
sub-millisecond datetime(...) literals at type check after 2.1 was set, and
the version stayed 2.1.

Backing issue / RFC

  • Fixes an accepted issue: none filed (see Notes for reviewers)
  • Is an RFC PR, or implements an accepted RFC
  • Trivial fast-lane

Checklist

  • Change is focused (one logical change)
  • Tests added/updated for behavior changes
  • Public docs updated if user-facing surface changed
  • Reviewed against docs/dev/invariants.md: invariant 8 is strengthened; no deny-list item hit

Local verification

  • cargo test -p omnigraph-compiler --locked: 436 passed (434 at baseline, plus 2 new)
  • cargo test -p omnigraph-planner --locked: all passed, same counts as baseline
  • cargo test -p omnigraph-engine --locked --lib: 415 passed
  • cargo test -p omnigraph-engine --locked --test <t> for traversal_indexed (14), traversal_adaptive (8), engine_v2 (11), engine_v2_plan_replay (21), engine_v2_scrubbed_replay (5), proptest_equivalence (3): all passed
  • cargo test -p omnigraph-gqt --locked: 255 of 255 cases passed. The host was shared and heavily loaded (load average 30 to 44 on 18 cores). In the first run, 24 cases hit their 10 s case timeout ("code":"timeout") and one harness test (dst_worker_freezes_the_engine_baseline_and_replay_identity) failed once. All of them passed when rerun alone, and none involves cross-type traversal.
  • cargo fmt --all --check: clean
  • cargo clippy -p omnigraph-compiler -p omnigraph-planner -p omnigraph-engine --all-targets --locked -- -D warnings -W clippy::dbg_macro: clean
  • python3 scripts/check-docs.py: OK; typos over the changed files: clean
  • Workspace-wide clippy and the canonical workspace test graph: not run locally; left to CI

Notes for reviewers

  • Compatibility. A query or stored query with a cross-type hop bound above
    one now fails type checking, and omnigraph queries validate reports it.
    Under the old cap, {1,n} returned the one-hop rows, so dropping its bound
    keeps them. A range starting above one, such as {2,2}, returned none, and
    dropping its bound would return one-hop rows. Its T5 fix and the changelog
    therefore say the pattern matches nothing and offer no rewrite. The changelog
    fragment is changed. A maintainer may want an additional breaking
    fragment for the stored-query consequence, as the sub-millisecond DateTime
    refusal had.
  • Accepted RFC amended. docs/rfcs/2026-09-30-typed-edge-alternation.md
    said "Named cross-type hop ranges retain their existing one-hop cap." Per
    the RFC process (step 4, post-merge amendment), that body sentence is
    rewritten to the current rule, and a dated Decision-log entry names the two
    sentences it supersedes and why the cap was dropped.
  • No backing issue. I can file one if you want it to back this PR.
  • Tests.
    • typecheck_tests.rs: refusal in both directions, with {2,2} and
      inside not { }, checking the code, message and fix. worksAt and
      worksAt{1,1} still type-check.
    • traversal_indexed.rs::cross_type_id_collision_does_not_bleed_into_second_hop:
      worksAt{1,2} is now refused. The id-collision fixture still checks the
      one-hop form across CSR, indexed and auto.
    • proptest_equivalence.rs: employers now uses worksAt. Its answer was
      always the one-hop answer, so collision coverage is unchanged.
    • expand.rs: a replayed named cross-type step with max_hops = 2 is
      refused under the pinned and the budgeted policy.
    • GQT variable_hops_on_a_chain.gqt: cross-type {1,2} and {2,2} are
      refused; the omitted bound and {1,1} return the one-hop row.

A named traversal such as `$p worksAt{1,2} $c` over `WorksAt: Person ->
Company` type-checked, then ran as `{1,1}`: the planner cost model and the
engine BFS both capped a cross-type expand at one hop, so the requested bound
was dropped without a word. A hop ends on the destination type and the next
must start on the source type, so no such path continues past one hop.

The type checker now refuses a hop bound above one when the edge's endpoint
types differ, under T5, the code that already refused the same shape for
edge selections; named edges get a message naming the edge declaration and a
fix. Engine plan validation refuses the same shape for named expands too, so
a replayed plan cannot reach the BFS with it, and the now-unreachable caps
(`cost_effective_hops`, `ExpandStatistics::same_type`, the BFS clamp) are
removed. Omitted bounds and `{1,1}` stay valid.

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: approve, with two optional documentation corrections. I found no blocking runtime defect at 74ace8bdf05fc8e1c4b51ddc80be722cd163d696. This is a COMMENT review because the authenticated account is also the PR author.

What changes, and why

A WorksAt: Person -> Company hop ends at a Company. Repeating that same directed step requires a Person as its next source. The engine therefore cannot continue that path. Today, it accepts wider bounds and caps execution at one hop. This PR instead rejects those bounds with a typed error and a suggested one-hop query.

The compiler rejects the query before planning. Engine validation applies the same restriction to saved plans that bypass the compiler. The planner and BFS then remove their special-case cap. Ordinary cross-type one-hop queries and same-type multi-hop queries retain their behavior.

For agents that generate bounded queries, an explicit error makes an unsupported request visible. It also creates a compatibility cost: existing query text and saved plans can now fail. This is a stricter language contract. A maximum of two does not itself promise that a two-hop path exists. The old one-hop answer for {1,2} was consistent with that range. The benefit is a uniform static rule and less special-case machinery, rather than different reachability results for valid queries.

Correctness and substrate checks

The compiler still checks positive, finite, ordered bounds before the new rule. It resolves traversal orientation first, so the rule covers both directions and nested patterns. The tests also retain omitted bounds and explicit {1,1}.

The execution entry validates traversal structure before execution. ExpandExec construction checks it again against the catalog. Thus, removing the BFS cap does not expose an unchecked second hop through either named-plan policy. This matters when different node types share an ID string.

Lance 11.0.0 remains the substrate, pinned to source commit ab6b5bbe46009ed78746b444df8db59a8bc5d842. I checked its typed scanner filter entry and OmniGraph's edge probe. Lance filters endpoint values within a dataset. It does not own the graph's legal hop sequence. Enforcing this rule in the compiler and plan validator therefore addresses the cause at the correct boundary. The change adds no storage format or local/S3/Azure-specific path.

Tradeoffs and liability

The trade is compatibility for a smaller accepted plan space. Existing stored queries need validation during upgrade. Removing a bound preserves the old {1,n} result, but it is not a general result-preserving rewrite for minimum depths above one. The first inline note asks the release text to make that distinction clear.

Long-term implementation liability decreases. The PR removes cost_effective_hops, an argument to executed_hops, and ExpandStatistics::same_type. The planner no longer carries a second interpretation of the requested depth. It adds no cache, durable state, storage primitive, or new diagnostic code. The compiler needs a useful user error, while the engine needs a refusal for saved plans. Those are separate boundaries with a shared engine validator, not two execution strategies.

The diff adds 257 lines and removes 57, largely because it adds regression coverage and diagnostic text. The positive line count does not outweigh the removal of special-case execution state. Five similar changes should continue to reject unsupported plans at admission and remove the downstream exceptions they made necessary.

For workloads with repeated small graph probes, this avoids executing queries that the new contract rejects. It does not establish faster valid queries. Their effective depth and storage work remain the same. The existing cost model still chooses indexed scans or CSR for supported traversals.

Validation and limits

  • Fresh local Cargo runs passed: 436 compiler library tests and 93 planner tests across its library and integration targets.
  • The new compiler refusal test failed with the base checker because it accepted worksAt{1,2}. It passed after I restored the PR checker. All temporary source changes are restored.
  • I reused compiled test binaries from a clean checkout at this exact head. The named-plan refusal test passed for pinned and budgeted policies. All 14 indexed-traversal tests and 21 plan-replay tests passed. The GQT hop-range case also passed, covering rows, result types, and refusals.
  • Documentation validation passed for 210 Markdown files. Agent-guide links and diff whitespace checks passed.
  • Exact-head GQT CI passed. Main CI reports successful workspace tests, server tests, clippy, S3, and Azurite checks. The format-fence job was still running at the last check.

I read the repository guidance and reused the applicable complete upstream documentation review with the unchanged Lance pin. I did not run cloud tests or performance benchmarks locally. The local runtime checks reused existing binaries rather than completing a fresh engine rebuild. The optional notes concern release guidance and keeping the accepted RFC current.

Comment thread changelog.d/cross-type-hop-bound-refused.changed.md Outdated
Comment thread docs/user/queries/traversal.md
@ragnorc
ragnorc enabled auto-merge October 8, 2026 18:20
…nation RFC

Dropping the bound keeps the rows of a cross-type range that includes hop
one, but a range starting past it (`{2,2}`) matched nothing under the old
cap, and dropping its bound would return one-hop rows. The T5 fix and the
changelog now say so instead of offering the same rewrite for every range.

The accepted typed-edge-alternation RFC still said named cross-type hop
ranges keep their one-hop cap. Its body sentence is rewritten and a dated
Decision-log entry names the sentences it supersedes.
@ragnorc
ragnorc added this pull request to the merge queue Oct 8, 2026
Merged via the queue into ModernRelay:main with commit 6f97159 Oct 8, 2026
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant