refactor(query_plan): migrate the three boundary-crossing transformers onto walk.rs (R2, group 1)

[TimelordUK]

Step 2 of the R2 work (docs/ENGINE_REFACTORING.md), following #31. Three commits, one per transformer. Net −109 lines (327 deleted, 218 added — the added side includes new tests and doc comments).

Why these three first

ilike_to_like, cte_hoister and into_clause_remover are the transformers that currently descend into subquery statements. walk::map_children treats a subquery as a scope boundary and won't cross it, so a naive migration would silently stop each of them working inside nested queries. They need a hybrid — explicit subquery arms, delegate the rest — and getting that pattern right on the risky files first seemed better than saving them for last.

Correction to my earlier framing

I told you step 2 would be "low risk, isolated" and implied a pure refactor. That was wrong for two of these, and — as it turns out — wrong in the opposite direction for the third.

	Behaviour	Demonstrated by
into_clause_remover	Fix	test fails without the change
ilike_to_like	Two fixes	both tests fail without the change
cte_hoister	Pure refactor	could not construct a failing case — see below

The fixes (each with a test verified to fail on the old code)

ilike_to_like, window specs. WindowSpec::order_by holds real expressions and the old walker passed window_spec straight through. An ILIKE inside OVER (ORDER BY CASE WHEN name ILIKE ... END) was left as ILIKE and reached the executor as an unknown operator. Probe output on the old code: found ops: ["ILIKE"].

ilike_to_like + into_clause_remover, tuple subqueries. InSubqueryTuple / NotInSubqueryTuple fell into the catch-all, so WHERE (a, b) IN (SELECT ... INTO #t) kept its INTO, and an ILIKE in a tuple subquery body was never rewritten.

Where the survey was wrong, and I checked

The plan predicted cte_hoister's tuple arms would be a behaviour change too. Probing the parser says otherwise — WITH is rejected everywhere in expression position:

(a, b) IN (WITH c AS (...) SELECT ...)   -> "Tuple IN requires a subquery on the right"
x = UPPER((WITH c AS (...) SELECT ...))  -> parse error
x BETWEEN (WITH ...) AND 5               -> parse error
x IN (1, (WITH ...))                     -> parse error

So nothing reaches the new hoisting arms — and by the same argument the pre-existing ScalarSubquery/InSubquery arms in that function are equally unreachable. I couldn't write a test that fails without the change, so I haven't claimed one. The code comments say "defensive, not a demonstrable fix" rather than implying a bug was fixed, and I dropped the test I'd written once I found it couldn't parse.

The one reachable gain there is in find_cte_refs_in_expression: its tuple arms need only a reference to an already-hoisted CTE, not a nested WITH, so dependency ordering now sees those.

Also

• Deletes dead cte_hoister::hoist_from_condition (R5).
• Drops imports the migrations made redundant.

Process note: a span-based edit silently swallowed hoist_from_where_clause while removing the dead function. The compiler caught it immediately, but I've since diffed the full fn inventory against HEAD to confirm nothing else went missing — only the intended deletion. Worth repeating on the remaining migrations.

Verification

• cargo test: 683 passed. One pre-existing failure, history_protection_integration — unrelated, reads the real roaming history.
• DuckDB parity --check: contract holds, unchanged at 64 AGREE / 1 DIFFER / 7 GAP.
• Examples: all FORMAL tests pass.
• cargo fmt clean.

Next

Group 2 — where_alias_expander (~200 lines), expression_lifter (3 walks that have drifted apart), and having_alias_transformer, which is the biggest correctness win in the set: it handles only FunctionCall/BinaryOp/Not, so HAVING COUNT(*) BETWEEN 1 AND 5 and HAVING CASE WHEN COUNT(*) > 5 ... are silently not rewritten today.

🤖 Generated with Claude Code