refactor(engine): split exec.rs into modules with no behaviour change

[jeswr]

Requested by Jesse · project thread

Summary

Before: crates/sparq-engine/src/exec.rs was one 23,844-line file. Any engine change conflicted with every other open engine PR, and agents had to page through the whole file.

After: exec.rs is 851 lines of shared types (Row, Bindings, LocalVocab, budget glue) and module wiring. The rest lives in exec/, 65 files, the largest 3,453 lines. Query behaviour is unchanged.

How:

• The 48 inline mod x { … } blocks (state registries and test modules) move verbatim to exec/x.rs, de-indented one level. Lines inside multi-line string literals are left byte-identical.
• The top-level code moves into 17 modules along the file's existing section banners: entry, dispatch, sargable, spatial_pushdown, bgp, triple_terms, yannakakis, planning, wcoj, compose, antijoin, aggregation, modifiers, expr, compiled, terms, sip_rewrite.
• Each module does use super::*, and exec.rs glob-imports it back at the matching visibility, so every existing path such as crate::exec::eval_select still resolves. Moved private items become pub(super), the narrowest visibility that keeps sibling access.
• yannakakis is now gated at the mod declaration instead of per item.
• A line-multiset comparison against main (ignoring comments, indentation and pub(super)) shows only the mod/use wiring and the yannakakis gates differ. Both sides have 241 #[test].
• Old model tags ([OPUS-x], [FABLE-5]) are stripped from moved and remaining comments.
• scripts/check-vectorized-feature-off.py leg 3 now scans exec.rs, every file under exec/, and the #[path] children outside exec/ (eqjoin.rs, which the old audit also missed, and exists_domain.rs, numeric_capacity.rs and nullable_path_tests.rs). It fails if any mod declared under exec resolves to a file it doesn't scan. After the split the vectorized references live under exec/, so the old leg 3 would have passed while scanning none of them. --self-test gains three tripwires.

The split is scripted, so it was regenerated on main f4e7b56c rather than rebased by hand. The regenerated tree is one merge commit on top of the earlier branch, so no history was rewritten.

Base gate (always required)

• cargo clippy -p sparq-engine --all-targets (default and --all-features) with -D warnings is clean
• cargo check -p sparq-engine --all-targets --features X passes for each of the 32 engine features, plus --no-default-features; sparq-wasm checks for wasm32
• cargo test -p sparq-engine passes (57 suites, 957 passed, 0 failed)

Targeted re-evaluation

• Query execution: no semantic change; local conformance run 1224 pass + 5 documented divergences = 1229, 0 fail, same as main.

Ratchets and conventions

• No ratchet lowered. skills/sparql-query/SKILL.md now points at the moved columnar_filter_seam module.

Benchmark

/mnt/project-files/engine/bench/ab.sh operators suite, main f4e7b56c against this head, 15 interleaved rounds of best-of-10, run in both orders. Row counts match on all 28 queries.

• release (fat LTO, one codegen unit, the shipping profile): geomean 1.000 and 1.001; q12_filter_exists 0.997 and 1.003.
• release-fast: geomean 1.022 (head second) and 0.981 inverted, i.e. about 2% slower; q12_filter_exists about 9% and q06_optional about 7% slower in both orders.

The code is identical and the shipping profile is at noise, so the release-fast gap is codegen-unit partitioning following the new module boundaries.

🤖 Generated with Claude Code

https://claude.ai/code/session_01C1gMf8RJim1LzuKapZNpZR

---

Generated by Claude Code

---

Generated by Claude Code

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head ae0d9996b25c. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [medium] Preserve executor coverage in the vectorized feature audit — crates/sparq-engine/src/exec.rs:564
   Moving vectorized call sites into exec/expr.rs leaves scripts/check-vectorized-feature-off.py --leg3 scanning only the parent exec.rs. The audit now checks just two lib.rs registrations and reports “Structural absence confirmed” without inspecting executor references; an ungated reference in the extracted file also passes. The current guards remain correct, but this silently removes coverage from a required CI invariant.

   Extend the audit to executor child modules, accounting for inherited feature guards, and add a tripwire that rejects an ungated child-file reference.

Verdict: Restore the CI audit coverage before merging.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 5def071c068e. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [low] Same-line attributes hide child modules from the audit — scripts/check-vectorized-feature-off.py:395
   A legal declaration such as #[path = "elsewhere.rs"] mod hidden; is ignored because the matcher requires the line to start with mod. If that external file contains ungated vectorized references, it escapes scanning and the coverage tripwire. Reproduction returns success and reports zero child declarations.
   Handle attributes preceding declarations on the same line, or reject unresolved declarations. Add this case to the tripwire tests.

2. [low] Negated feature conditions are accepted as vectorized-only gates — scripts/check-vectorized-feature-off.py:99
#[cfg(all(not (feature = "vectorized")))] followed by mod child; matches this regex because its exclusion recognizes only not( without whitespace. The audit consequently exempts the entire child file, although Rust enables it specifically when vectorized is disabled. Reproduction with an ungated DataChunk reference returns success.
   Determine whether the cfg expression actually requires vectorized, or conservatively accept only supported positive conditions. Test whitespace variants of negated and alternative conditions.

Verdict: Fix the audit false negatives before merging; no engine behavior regression was found.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head cc0ba22101c1. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [medium] Conditional paths can bypass the audit — scripts/check-vectorized-feature-off.py:535
   For #[cfg_attr(all(), path="elsewhere.rs")] mod child;, Rust compiles src/elsewhere.rs, but the audit resolves exec/child.rs. If that nominal file exists, ungated DataChunk references in elsewhere.rs go unscanned and leg 3 incorrectly succeeds. Resolve conditional path attributes or reject unsupported path-affecting attributes rather than assuming the default path.

2. [medium] A gated alias exempts an ungated module — scripts/check-vectorized-feature-off.py:634
   When two module declarations use #[path="exec/child.rs"], one gated on vectorized and one ungated, the gated declaration marks the entire file as protected. Leg 3 then accepts ungated vectorized references even though the second declaration compiles them without that feature. Require every reachable declaration of a file to imply vectorized before exempting its contents, including descendant modules.

Verdict: Fix these audit failures before merging; no execution-semantic regression was found in the Rust split.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head ae9b512a528a. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [medium] Unsupported path literals silently select the wrong file — scripts/check-vectorized-feature-off.py:557
_PATH_ATTR only recognizes ordinary quoted strings. For valid Rust such as #[path = r"elsewhere.rs"] mod child;, the audit ignores the attribute and falls back to exec/child.rs. If that fallback file exists and is clean, while src/elsewhere.rs contains ungated vectorized references, --leg3 returns success without scanning the file Rust actually compiles. This breaks the new coverage tripwire and its structural-absence claim.

   Parse Rust string literals correctly, or fail closed whenever a path attribute is present but unsupported. Add a regression fixture with both the clean fallback and the ungated actual target.

Verdict: Fix the audit’s false-negative path resolution before merging.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 5167fe5216f2. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [low] Module tripwire silently skips valid declarations — scripts/check-vectorized-feature-off.py:519
   A declaration such as #[path = "elsewhere.rs"] mod r#hidden;, or one with mod and hidden; on separate lines, never matches _MOD_DECL_LOOSE. Consequently, --leg3 returns success even when that unscanned file contains an ungated crate::chunk::DataChunk reference, defeating the new coverage tripwire.
   Parse complete declarations, including raw identifiers, or fail closed when unsupported module syntax is encountered.

Verdict: The Rust split appears behavior-preserving, but fix the audit blind spot before merging.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 2096ad0810dd. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [medium] Module audit silently skips trailing declarations — scripts/check-vectorized-feature-off.py:530
_MOD_DECL.match(rest) matches only the first declaration and ignores the remainder. With mod child; #[path = "elsewhere.rs"] mod hidden; in exec.rs, a clean exec/child.rs, and an ungated vectorized reference in src/elsewhere.rs, leg 3 returns success after checking only child. This defeats the new coverage tripwire while reporting “Structural absence confirmed.”

   Parse every declaration on the line or reject unsupported trailing syntax. Add a regression tripwire for this case.

Verdict: The Rust split appears behavior-preserving, but the audit bypass should be fixed before merging.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head 230fd4c7e0f9. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [medium] Commented path attributes bypass the module audit — scripts/check-vectorized-feature-off.py:555
   A valid declaration such as #[path = "elsewhere.rs"] // rationale followed by mod child; loses its path attribute because the preceding-attribute parser rejects the trailing comment, then silently stops. If a clean exec/child.rs exists, the audit scans that file instead of the actual src/elsewhere.rs. Ungated vectorized references in the actual module go unchecked, and leg 3 incorrectly reports success.

   Strip comments while respecting string literals, or fail closed when an attribute cannot be parsed. Add a regression test with a commented path attribute and a clean default-path file.

Verdict: Fix the audit bypass before merging; no execution-behavior problems were found in the Rust split.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head b98a8c293799. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

1. [medium] Inline module paths can bypass the feature audit — scripts/check-vectorized-feature-off.py:553

   The scanner skips inline module declarations and uses indentation to identify nested declarations. With #[path = "outside"] mod inner { containing an unindented mod child;, Rust loads src/outside/child.rs, but the scanner resolves src/exec/child.rs. If that scanned file exists and is clean, ungated vectorized references in the actual child escape inspection. An in-memory reproduction returned success and “Structural absence confirmed.”

   Track inline module scopes and their path attributes, or reject nested declarations regardless of indentation. Add a regression test covering this false pass.

Verdict: Fix the audit bypass before merging as is.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head c2f6e1e5713f. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

No correctness, security, soundness or design problems found.

Verdict: Safe to merge as is.

[jeswr]

🔎 Codex reviewer — gpt-6.1-sol

Automated review by the Codex reviewer (OpenAI gpt-6.1-sol via Codex CLI) of head a8d8836eb8f6. Scope: correctness, security, soundness and design; style nits omitted. A new head gets a fresh review.

Findings

No correctness, security, soundness or design problems found.

Requested checks

1. Behaviour preservation: Verified against f4e7b56c. The normalized line multiset differs only by module/import wiring, equivalent Yannakakis gates and comment removal. Both sides contain 241 #[test] attributes. A stronger token comparison matched all 48 extracted inline module bodies and all 383 remaining top-level items, preserving string literals and attached attributes. Existing public visibility and all inlining/cold attributes are preserved; added pub(super) visibility supports sibling access. The head is the described merge commit. No policy/Solid code changes.

2. Feature-off audit: Leg 3 passes, scanning 70 files and checking 69 module declarations. It covers exists_domain, numeric_capacity, nullable_path_tests and eqjoin. All 53 synthetic leg-3 cases passed using an in-memory filesystem; injected ungated references in each external child were rejected.

3. Performance: Found no lost inline attributes, changed algorithms, new dispatch indirection or movement across crates. The reported slowdown is consistent with changed codegen partitioning under release-fast’s 16 codegen units. Source review cannot establish that as the sole cause without generated-code or profiling evidence.

Rust builds, query tests and benchmarks were not rerun in the read-only environment.

Verdict: The PR is safe to merge as is.