Skip to content

Skip Values construct in FROM clause instead of raising NotImplementedError - #43

Merged
flipbit03 merged 4 commits into
mainfrom
fix/values-from-clause
Jun 16, 2026
Merged

Skip Values construct in FROM clause instead of raising NotImplementedError#43
flipbit03 merged 4 commits into
mainfrom
fix/values-from-clause

Conversation

@flipbit03

@flipbit03 flipbit03 commented Jun 16, 2026

Copy link
Copy Markdown
Owner

Summary

SoftDeleteQueryRewriter.analyze_from() raised NotImplementedError whenever a rewritten SELECT had a SQLAlchemy Values construct (sqlalchemy.values()) as a top-level entry in its FROM clause. A VALUES (...) row set has no underlying table, so there is nothing to soft-delete-filter — it should be skipped (returned unchanged), exactly like the existing TableClause/TextClause branch.

This fix adds a Values branch to analyze_from() that returns the statement unchanged.

Why it matters

Two ways to hit this:

  1. Directly — any executed SELECT whose FROM contains a top-level Values.
  2. Indirectly (the common real-world trigger) — an ORM bulk UPDATE (update(Model)…) referencing a Values in its WHERE/SET. The library only rewrites SELECTs, but the ORM emits a synchronize-session SELECT to reconcile the identity map, and that spawned SELECT carries the Values in its FROM — so the rewriter tripped on it. The UPDATE t SET col = v.x FROM (VALUES …) AS v WHERE t.id = v.id pattern is a common single-round-trip bulk-update idiom.

Changes

  • analyze_from() now returns the statement unchanged for a Values FROM entry (no NotImplementedError).
  • Three regression tests in tests/default_config/test_queries.py that execute against the database (PostgreSQL in CI) and assert real behavior:
    • test_select_with_top_level_values_from_is_not_rewritten — a top-level Values SELECT runs and returns its rows.
    • test_orm_update_referencing_values_does_not_trip_rewriter — the ORM UPDATE-with-VALUES executes and the update takes effect.
    • test_table_alongside_values_is_still_soft_delete_filtered — a soft-deletable table alongside a top-level Values still excludes soft-deleted rows (deleted_at IS NULL) while the Values rows pass through.

All three fail on NotImplementedError without the fix and pass with it (verified against PostgreSQL). They skip on SQLite — which only accepts VALUES inside a CTE, not as a top-level FROM — mirroring the existing test_insert_with_returning skip pattern, so the quick make test (SQLite) stays green.

Test plan

  • make test-pg (PostgreSQL) → 73 passed
  • uv run pytest (SQLite quick) → 3 new tests skip cleanly, rest pass
  • ruff check / ruff format --check clean
  • mypy clean

Closes #42

…dError

A sqlalchemy.values() VALUES (...) row set appearing as a top-level FROM
entry has no underlying table to soft-delete-filter, so analyze_from() now
returns the statement unchanged for it, exactly like the TableClause/TextClause
branch. Previously it fell through to the final raise.

This also fixes the common indirect trigger: an ORM bulk UPDATE referencing a
VALUES emits a synchronize-session SELECT carrying that VALUES in its FROM,
which tripped the rewriter on the spawned SELECT.

Closes #42
Rewrite the three regression tests to actually execute the statements (as the
rest of the suite does against PostgreSQL in CI) and assert real behavior:
the VALUES SELECT returns its rows, the ORM UPDATE takes effect, and a real
table alongside a top-level VALUES is still soft-delete filtered.

The statements skip on SQLite (which only accepts VALUES inside a CTE), mirroring
the existing test_insert_with_returning skip pattern.
@flipbit03 flipbit03 self-assigned this Jun 16, 2026
On SQLAlchemy 1.4 the default synchronize strategy evaluates the WHERE criteria
in Python, which cannot evaluate a VALUES column reference and raises
UnevaluatableError before any SQL is emitted -- a SQLAlchemy limitation unrelated
to the rewriter. Forcing 'fetch' reconciles the identity map via a spawned SELECT,
which is exactly the indirect code path this issue is about, and works uniformly
on 1.4 and 2.0.
The root CHANGELOG.md was abandoned (last entry 0.2.1 while releases are at
v0.9.0) and not maintained per-PR; release notes live in GitHub Releases.
Also remove docs/changelog.md, which only existed to include the root file.
@flipbit03
flipbit03 merged commit d9cac7d into main Jun 16, 2026
6 checks passed
@flipbit03
flipbit03 deleted the fix/values-from-clause branch June 17, 2026 14:00
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.

Rewriter raises NotImplementedError on a Values construct in a statement's FROM (should skip it like TextClause)

1 participant