Skip to content

refactor(query_plan): keep from_source and legacy FROM fields in sync (P3 groundwork)#30

Merged
TimelordUK merged 2 commits into
mainfrom
chore/p3-step0-from-source-sync
Jul 18, 2026
Merged

refactor(query_plan): keep from_source and legacy FROM fields in sync (P3 groundwork)#30
TimelordUK merged 2 commits into
mainfrom
chore/p3-step0-from-source-sync

Conversation

@TimelordUK

Copy link
Copy Markdown
Owner

Groundwork for P3 — correlated subqueries (docs/SQL_PARITY.md). No parity behaviour changes here; this makes from_source trustworthy before P3 starts depending on it.

The problem

The executor (query_engine.rs:1217) reads from_source first and only falls back to the deprecated from_table / from_subquery. The parser populates both. Several transformers rewrote only one — so the stale copy is the one that actually executes.

Site Desync
in_operator_lifter.rs:218 from_table → lifted CTE, from_source → original base table (no lifted column)
expression_lifter.rs:187 same
cte_hoister.rs:61 mutates from_subquery; from_source::DerivedTable keeps a pre-hoist clone
into_clause_remover.rs:44 same

The fix

Adds impl SelectStatement (there was none) with set_from_table, set_from_subquery, map_from_subquery, and routes all four sites through them so the representations can only move together.

map_from_subquery replaces the take-then-restore pattern — that left from_source stale in between and dropped the derived-table alias.

Also deletes column_dependency_lifter.rs: absent from query_plan/mod.rs, referenced nowhere, and no longer compiles against the current AST. Removes 9 deprecated-field sites and 14 SqlExpression match arms from the later P3 audit.

Honest scoping

Neither desync currently produces wrong results. The stale copies happen to be valid statements — pre-hoist from_source still carries its own CTEs, so the executor gets a correct answer by a different route. I tried and failed to build an end-to-end query that exposes either. This pins the invariant rather than fixing live breakage.

Both regression tests parse real SQL instead of hand-building an AST — the existing fixtures set from_source: None, an input the parser never produces, which is exactly why the desync survived. Verified both fail without the fix:

from_source holds a stale pre-hoist subquery with 1 CTE(s)
from_source must follow from_table to the lifted CTE

Verification

  • cargo test: 672 passed (+2). One pre-existing failure, history_protection_integration, unrelated — it sets APPDATA to a tempdir but still loads the real roaming history.
  • DuckDB parity --check: contract holds, unchanged at 64 AGREE / 1 DIFFER / 7 GAP.
  • Python: 497 passed / 18 failed — verified identical on a stashed baseline (missing local data files, offline localhost endpoints).
  • Examples: all FORMAL tests pass.
  • cargo fmt --check clean; no new clippy warnings in touched files.

Second commit

tests/integration/test_examples.py hardcoded ./target/release/sql-cli, aborting the whole examples suite on Windows before running a case. Mirrors the resolution already in tests/comparison/engines.py. Unrelated to the refactor — split out so it can be dropped independently.

Not addressed (deliberately)

The wider from_source migration. It is blocked on real AST work — TableSource has no table-function variant (recursive_parser.rs:1132) and TableSource::Table cannot carry an alias — which would drag JoinClause in too. Bigger than P3 itself; revisit separately.

🤖 Generated with Claude Code

TimelordUK and others added 2 commits July 18, 2026 12:28
Groundwork for P3 (correlated subqueries). The executor reads
`from_source` first and only falls back to the deprecated `from_table` /
`from_subquery`, but several transformers rewrote just one of the two.
The parser populates both, so the stale copy is the one that gets
executed.

- in_operator_lifter, expression_lifter: repointed the FROM clause at the
  lifted CTE via `from_table` only, leaving `from_source` on the original
  base table (which has no lifted column).
- cte_hoister, into_clause_remover: mutated `from_subquery` while
  `from_source::DerivedTable` kept a pre-hoist clone.

Adds `impl SelectStatement` (there was none) with `set_from_table`,
`set_from_subquery` and `map_from_subquery` so the two representations
can only move together, and routes the four sites through them.
`map_from_subquery` replaces the take-then-restore pattern, which left
`from_source` stale in between and dropped the derived-table alias.

Both regression tests parse real SQL rather than hand-building an AST:
the existing fixtures set `from_source: None`, an input the parser never
produces, which is why the desync survived. Verified both tests fail
without the fix.

Neither desync currently yields wrong results -- the stale copies happen
to be valid statements -- so this pins the invariant before P3 starts
relying on `from_source` being authoritative.

Also deletes column_dependency_lifter.rs: absent from query_plan/mod.rs,
referenced nowhere, and no longer compiles against the current AST.

cargo test 672 passed (+2); DuckDB parity contract holds unchanged at
64 AGREE / 1 DIFFER / 7 GAP; examples FORMAL tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_examples.py hardcoded ./target/release/sql-cli, so the entire
examples suite aborted on win32 with "not found" before running a single
case. Mirrors the resolution already used by tests/comparison/engines.py.

Note the suite also needs PYTHONIOENCODING=utf-8 PYTHONUTF8=1 on a cp1252
console, same as the parity harness, or it dies on the checkmark glyph.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@TimelordUK
TimelordUK merged commit e03d92d into main Jul 18, 2026
8 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