From b4bb11eeec793e3f430b4f54799f2888246f1837 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 15:04:15 +1000 Subject: [PATCH 01/23] docs: design spec for generic-default dialect operations (upsert + rename_table) Complete reusable upsert across three SQL families (ON CONFLICT / MERGE / ON DUPLICATE KEY) + portable rename_table, dispatched via DialectSpec upsert_style, with sqlglot-rendered generic defaults and an override seam. Live-tested on sqlite/duckdb/postgres/mysql (Docker), golden-SQL for the full 21-dialect registry. Builds on the shipped add_columns pattern. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...neric-default-dialect-operations-design.md | 411 ++++++++++++++++++ 1 file changed, 411 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md diff --git a/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md new file mode 100644 index 0000000..a2d64be --- /dev/null +++ b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md @@ -0,0 +1,411 @@ +# Generic-Default Dialect Operations — Design Spec + +> **Status:** DESIGN (2026-06-29). Approved scope: build the *complete, +> reusable* write-operation surface for `IbisBackend` — portable `upsert` +> across every upsert family the dialect registry can name, plus a portable +> `rename_table` — not a minimum that unblocks one consumer. Successor to +> `2026-06-27-dialect-aware-add-columns-design.md` (shipped, PR #90); reuses +> and consolidates its sqlglot rendering pattern. + +## 1. Purpose + +`IbisBackend` exposes write/DDL operations through a `DialectSpec` hook +registry. Two of them are under-supported for no good reason: + +- **`upsert`** is registered for only `sqlite`/`duckdb`/`motherduck` (3 of 21 + dialects); every other dialect — including `postgres` — raises + `NotImplementedError`. +- **`rename_table`** has **zero** registered hooks; it raises for *every* + dialect. + +This package is strategic, reusable infrastructure — physical access to +backend data services — not a wearables helper. The unit of design is the +package's domain responsibility (upsert/rename across the SQL backends it +claims to serve), not any one consumer's current backend. This spec makes +both operations *work everywhere the SQL is expressible*, via generic +defaults rendered through sqlglot — the same engine Ibis itself uses — with +an override hook retained for genuine exceptions. + +## 2. The Discriminator Principle + +`IbisBackend` capability operations fall into two classes; dispatch must +reflect the class: + +- **Uniform / family-uniform SQL** — the statement is the same across a + family of dialects; only type rendering, identifier quoting, and a bounded + statement-family choice vary. These get a **generic default** (sqlglot + rendered) + an **optional override hook**. Default: *works everywhere it + can be expressed.* +- **Capability-divergent** — the operation does not exist on some engines, + or reads dialect-specific system catalogs. These stay **hook-required**; + absent a hook, `NotImplementedError`. Default: *honestly unsupported.* + +`upsert` and `rename_table` are the former. `create_index` / `drop_index` +and the index-catalog queries are the latter and are **out of scope** (no +secondary indexes on BigQuery/Snowflake — a generic default would emit DDL +the engine rejects while presenting as supported). + +## 3. Goals / Non-Goals + +**Goals** +- `rename_table` works on every dialect whose rename is expressible, with no + per-dialect code (sqlglot renders `sp_rename` for SQL Server, `RENAME` for + MySQL, `ALTER TABLE … RENAME TO` elsewhere). +- `upsert` works across **three** upsert families — `ON CONFLICT`, + `MERGE`, and MySQL's `ON DUPLICATE KEY UPDATE` — preserving the full + existing semantics (composite keys, `conflict_action`, `update_columns`, + `update_condition`). Public signature unchanged. +- A single shared **rendering-primitives helper** backs `upsert`, + `rename_table`, and (consolidated) `add_columns`. +- Portable staging: the source frame is materialised as a compiled subquery + (Ibis's own mechanism), not a backend-specific temp-table registration. +- Live integration tests on `sqlite`, `duckdb`, `postgres`, `mysql` + (Docker); golden-SQL render assertions for the full registry; a documented + support matrix. + +**Non-Goals** +- `create_index` / `drop_index` / index-catalog queries — stay hook-required. +- A bespoke statement-renderer framework — sqlglot **is** the renderer; we + build its AST, we do not wrap it in a parallel layer. +- A second hand-maintained type map — types render through the connection's + own `compiler.type_mapper`, exactly as `add_columns` does. +- Live testing of warehouse engines (Snowflake/BigQuery/MSSQL/…) — covered by + golden-SQL render assertions, not execution. + +## 4. API Surface + +### 4.1 `upsert` (signature unchanged) + +```python +def upsert( + self, + name: str, + obj: t.Any, # frame or ibis Table + *, + conflict_columns: list[str] | str, + update_columns: list[str] | str | None = None, + conflict_action: str = "UPDATE", # "UPDATE" | "NOTHING" + update_condition: str | None = None, + database: str | None = None, + schema: str | None = None, +) -> IbisBackend: ... +``` + +Fluent. Composite `conflict_columns` is supported (unlike Ibis 12's native +`upsert`, whose `on` is a single column — a key reason we render our own). + +### 4.2 `rename_table` (signature unchanged) + +```python +def rename_table(self, old_name: str, new_name: str) -> IbisBackend: ... +``` + +### 4.3 Dispatch (both operations) + +```python +# rename_table +hook = self._spec.rename_table_hook +if hook is not None: + hook(conn._ibis_conn, old_name, new_name) +else: + _generic_rename_table(conn._ibis_conn, old_name, new_name) +return self + +# upsert +hook = self._spec.upsert_hook +if hook is not None: + hook(conn._ibis_conn, name, obj, conflict_columns=..., ...) +else: + _generic_upsert(conn._ibis_conn, name, obj, style=self._spec.upsert_style, ...) +return self +``` + +`_generic_upsert` raises `NotImplementedError` when `style is None`, naming +the dialect — the same honest-unsupported contract the hook path had. + +## 5. Architecture + +### 5.1 Registry: `upsert_style` + +`DialectSpec` gains one field: + +```python +class UpsertStyle(str, enum.Enum): + ON_CONFLICT = "on_conflict" # INSERT … ON CONFLICT DO UPDATE/NOTHING + MERGE = "merge" # MERGE INTO … WHEN MATCHED / NOT MATCHED + ON_DUPLICATE_KEY = "on_duplicate_key" # INSERT … ON DUPLICATE KEY UPDATE (MySQL family) + +@dataclass(frozen=True) +class DialectSpec: + ... + upsert_hook: t.Optional[UpsertHook] = None # override seam (unchanged) + upsert_style: t.Optional[UpsertStyle] = None # NEW — generic-default selector + rename_table_hook: t.Optional[RenameTableHook] = None + add_columns_hook: t.Optional[AddColumnsHook] = None +``` + +The existing `upsert_hook` registrations on `sqlite`/`duckdb`/`motherduck` +are **removed**; those dialects instead carry `upsert_style=ON_CONFLICT` and +flow through the generic renderer. `upsert_hook` remains as the override +seam (now used by zero dialects — like `add_columns_hook`), reserved for a +dialect that genuinely needs a quirk. + +### 5.2 Rendering-primitives helper (consolidation) + +`add_columns` rendered its own quoting inline. Extract the shared primitives +into one new sibling module, `backends/ibis/_render.py` (a flat module +alongside `operations.py`, **not** a restructure of `operations.py` into a +package), with a single responsibility — turn names/types into +dialect-correct SQL fragments off a live connection: + +```python +def dialect_of(ibis_conn) -> str: ... # ibis_conn.compiler.dialect +def quote_identifier(name: str, dialect) -> str: ... # exp.to_identifier(name, quoted=True).sql(dialect) +def qualified_name(parts: list[str], dialect) -> str: ... # ".".join(quote_identifier(p, dialect) …) +def render_type(type_mapper, dtype) -> str: ... # type_mapper.to_string(dtype) — create_table parity +def compiled_source(ibis_conn, obj) -> str: ... # ibis.memtable(obj) → ibis_conn.compile(…) subquery SQL +``` + +`_generic_add_columns`, `_generic_rename_table`, and `_generic_upsert` all +consume these. `add_columns` is migrated to the helper with **no behaviour +change** (the inline `_quote` is replaced by `quote_identifier`); its tests +remain green unchanged. + +### 5.3 `_generic_rename_table` + +```python +def _generic_rename_table(ibis_conn, old_name: str, new_name: str) -> None: + _validate_simple_identifier(old_name, kind="old_name") + _validate_simple_identifier(new_name, kind="new_name") + dialect = dialect_of(ibis_conn) + stmt = exp.Alter( + this=exp.to_table(quote_identifier(old_name, dialect)), + kind="TABLE", + actions=[exp.AlterRename(this=exp.to_identifier(new_name, quoted=True))], + ).sql(dialect=dialect) + ibis_conn.raw_sql(stmt) +``` + +sqlglot renders this as `ALTER TABLE … RENAME TO …` for most dialects, +`EXEC sp_rename …` for `tsql`, and `ALTER TABLE … RENAME …` for MySQL — +the *output* is verified by transpile probe (§9). The exact sqlglot +expression classes shown (`exp.Alter` / `exp.AlterRename`) are illustrative +and pinned during implementation against the installed sqlglot (30.x); the +equivalent transpile path (`sqlglot.transpile(, write=…)`) is +an accepted fallback if AST construction is more brittle. No override hooks +required initially. + +### 5.4 `_generic_upsert` and the three renderers + +```python +def _generic_upsert( + ibis_conn, name, obj, *, style, conflict_columns, update_columns, + conflict_action, update_condition, database, schema, +) -> None: + if style is None: + raise NotImplementedError(f"Dialect does not support upsert") + _validate_simple_identifier(name, kind="name") + # resolve target/conflict/update column sets, validate existence, + # validate conflict_action ∈ {UPDATE, NOTHING} + source_sql = compiled_source(ibis_conn, obj) # subquery, no temp table + if style is UpsertStyle.ON_CONFLICT: + stmt = _render_on_conflict(...) + elif style is UpsertStyle.MERGE: + stmt = _render_merge(...) + elif style is UpsertStyle.ON_DUPLICATE_KEY: + stmt = _render_on_duplicate_key(...) + ibis_conn.raw_sql(stmt) +``` + +Each renderer builds a sqlglot AST and renders with the live dialect. The +`MERGE` renderer mirrors Ibis 12's `_build_upsert_from_table` (`sge.merge`) +**extended** to composite `on` and our `conflict_action`. + +### 5.5 Staging — compiled subquery, not temp table + +The existing `duckdb_family_upsert` registers a temp staging table via the +DuckDB-specific `ibis_conn.con.register(...)`. That is the portability +blocker, not the SQL. The generic path instead compiles the source frame as +a subquery — `compiled_source()` returns `(SELECT … )` — used as the +`INSERT … SELECT … FROM ()` source or the MERGE `USING +() AS src`. This is exactly Ibis 12's mechanism. No staging table, +no cleanup, portable by construction. + +## 6. Semantics Mapping + +The public semantics map onto each family as follows. All three honour the +identical public contract. + +| Public param | `ON CONFLICT` | `MERGE` | `ON DUPLICATE KEY` | +|---|---|---|---| +| `conflict_columns` (composite) | `ON CONFLICT (c1,c2)` | `ON tgt.c1=src.c1 AND tgt.c2=src.c2` | unique-key implied; key cols excluded from SET | +| `conflict_action="UPDATE"` | `DO UPDATE SET …` | `WHEN MATCHED THEN UPDATE SET …` + `WHEN NOT MATCHED THEN INSERT …` | `ON DUPLICATE KEY UPDATE …` | +| `conflict_action="NOTHING"` | `DO NOTHING` | omit `WHEN MATCHED` (insert-if-absent only) | `… UPDATE c1=c1` (no-op self-assign) | +| `update_columns` (subset) | restrict `SET` list | restrict `WHEN MATCHED … SET` list | restrict `UPDATE` list | +| `update_condition` | `DO UPDATE SET … WHERE ` | `WHEN MATCHED AND THEN UPDATE` | unsupported → `ValueError` (MySQL has no per-row update predicate) | +| default `update_columns` | all non-key columns | all non-key columns | all non-key columns | + +`update_condition` on `ON_DUPLICATE_KEY` raises `ValueError` (honest: the +family cannot express it) rather than silently dropping it. + +## 7. Coverage Matrix (all 21 registry dialects) + +Confidence: **live** = executed against a real engine in CI/local; **render** += golden-SQL assertion only (no credentials/engine available); **n/a** = +`upsert_style=None`, honest `NotImplementedError`. + +| Dialect | `upsert_style` | Confidence | Notes | +|---|---|---|---| +| sqlite | on_conflict | **live** | in-memory | +| duckdb | on_conflict | **live** | in-memory; also exercises MERGE renderer (duckdb supports MERGE) | +| motherduck | on_conflict | render | duckdb engine | +| postgres | on_conflict | **live** | Docker; also exercises MERGE renderer (PG15+) | +| mysql | on_duplicate_key | **live** | Docker (mariadb, per Ibis) | +| singlestoredb | on_duplicate_key | render | MySQL-compatible | +| snowflake | merge | render | | +| bigquery | merge | render | | +| mssql | merge | render | sp_rename for rename | +| oracle | merge | render | | +| databricks | merge | render | Delta MERGE | +| exasol | merge | render | | +| trino | merge | render | connector-dependent at runtime | +| redshift | merge | render | postgres protocol but no `ON CONFLICT`; MERGE (2023+) | +| risingwave | on_conflict | render | postgres-wire table upsert | +| clickhouse | None | n/a | ReplacingMergeTree / ALTER UPDATE — divergent model | +| impala | None | n/a | MERGE only for Iceberg targets | +| materialize | None | n/a | restricts INSERT to write-only txns | +| druid | None | n/a | append-only analytics store | +| pyspark | None | n/a | MERGE only for Delta/Iceberg; Ibis marks notyet | + +`rename_table`: generic sqlglot default for **all** dialects (render-verified +across the registry; live on sqlite/duckdb/postgres/mysql). The override hook +stays available for any engine later found to diverge beyond sqlglot's +rendering. + +The matrix is shipped as a documented table in the module and asserted by the +golden-SQL tests (per-dialect rendered statement), so "render" coverage is +real verification of the emitted SQL, not an assumption. + +## 8. Testing + +### 8.1 Live backends — Docker + +A minimal `compose.yaml` at repo root, modeled on Ibis's `docker/` services +but using **stock images** (we need no PostGIS/pgvector/plpython): + +- `postgres`: `postgres:18-alpine`, env `POSTGRES_USER/PASSWORD/DB`, + healthcheck `pg_isready`, port 5432. +- `mysql`: `mariadb:12.1.2` (Ibis's choice; ON-DUPLICATE-KEY compatible), + healthcheck `mariadb-admin ping`, port 3306. + +Connection parameters read from environment with Ibis-compatible defaults +(`IBIS_TEST_POSTGRES_*` / `PG*`, `IBIS_TEST_MYSQL_*`) so the same env works +locally and in CI. + +### 8.2 Skip-if-unreachable fixtures + +`postgres` / `mysql` fixtures attempt a connection; on failure they +`pytest.skip(...)` (not fail), so a developer without Docker and a CI job +without the service still pass green. The live tests are marked +`@pytest.mark.integration` (existing marker). + +### 8.3 CI + +The existing `python-run-pytest.yml` gains GitHub Actions `services:` +containers for postgres and mariadb (native service-container support; no +compose needed in CI). A `make test-live` / hatch script brings the compose +services up locally. + +### 8.4 Test layers + +1. **Unit / golden-SQL** (`tests/test_unit/backends/ibis/test_upsert_render.py`, + `test_rename_table_render.py`): for every registry dialect, assert the + exact rendered statement per `upsert_style` and for rename. No live engine. + This is where the full matrix is verified. +2. **Live integration** (`tests/test_integration/test_write_ops_live.py`): + sqlite + duckdb (in-memory) + postgres + mysql — round-trip upsert + (insert + update + NOTHING + composite key + update_condition where + supported) and rename, asserting data outcomes. +3. **Consolidation regression**: existing `test_add_columns.py` stays green + unchanged after the helper extraction. + +## 9. Verification already performed (sqlglot transpile probe, ibis 12 env) + +- `ALTER TABLE … ADD COLUMN`, `RENAME`, and `MERGE` render correctly + per-dialect from sqlglot (`sp_rename` for tsql, `RENAME` for mysql). +- `INSERT … ON CONFLICT` renders natively for postgres/duckdb/sqlite and is + **not** transpiled to MERGE (confirming the family split is structural, not + a rendering gap). +- Ibis 12 `SQLBackend.upsert` exists (MERGE-based) but its `on` is a single + column — insufficient for the composite natural keys real consumers use — + which is why we render our own, using Ibis's `sge.merge` shape as template. + +## 10. Error Handling & Edge Cases + +- `conflict_action` ∉ {UPDATE, NOTHING} → `ValueError`. +- `conflict_action="UPDATE"` with no updatable columns (all columns are keys + and no `update_columns`) → `ValueError` (preserves current behaviour). +- `conflict_action="NOTHING"` with `update_columns`/`update_condition` → + warn-and-ignore (preserves current behaviour), except MERGE where NOTHING + simply omits the matched clause. +- `update_condition` on `on_duplicate_key` → `ValueError` (family cannot + express it). +- `upsert_style=None` → `NotImplementedError` naming the dialect. +- Target table absent → `ValueError` (preserves current behaviour). +- Identifiers: `name`/`database`/rename names validated simple (non-dotted) + via the existing `_validate_simple_identifier`; quoted per dialect. Dotted + qualified names out of scope (consistent with `add_columns`). + +## 11. Known Limitations + +- **Not concurrency-safe / not atomic** beyond the engine's own statement + atomicity — single-statement upsert is atomic where the engine makes it so; + no cross-statement transaction is added. +- **Subquery staging** inlines the source data as a compiled `VALUES`/`SELECT` + subquery (Ibis's mechanism). Very large frames produce large SQL; callers + with bulk loads should use a staging table + `upsert`-from-table directly. + Acceptable and matches upstream Ibis behaviour. +- **Render-only dialects** (matrix §7) are verified at the SQL-emission layer, + not by execution; first real use on such an engine may surface + engine-specific quirks, handled then via the `upsert_hook` override seam. +- **`update_condition` semantics** differ subtly across families (pre- vs + post-match predicate); documented per family in §6. + +## 12. Dependency + +- Bump the pin to **`ibis-framework>=12.0.0`** (the env reality; `add_columns` + already shipped against it; lets us reference Ibis's `sge.merge` shape). The + renderers use `sqlglot.expressions` directly, so 12 is not strictly required + for rendering, but aligning removes version drift. +- Correct the stale `ibis-framework == 10.4.0` reference in `CLAUDE.md` to the + actual `>=12.0.0`. + +## 13. Files Changed + +- `dialects/_registry.py` — `UpsertStyle` enum, `upsert_style` field; assign + styles per the matrix; remove the three `upsert_hook=duckdb_family_upsert` + registrations. +- `backends/ibis/_render.py` (new) — shared rendering primitives. +- `backends/ibis/operations.py` — `_generic_rename_table`, `_generic_upsert`, + `_render_on_conflict`, `_render_merge`, `_render_on_duplicate_key`; migrate + `_generic_add_columns` to the helper; **delete** `duckdb_family_upsert` — + the duckdb family routes through `_render_on_conflict` via + `upsert_style=ON_CONFLICT`. +- `backends/ibis/backend.py` — `upsert`/`rename_table` dispatch updated to the + hook-or-generic shape. +- `compose.yaml` (new) — postgres + mariadb stock services. +- `tests/test_unit/backends/ibis/test_upsert_render.py`, + `test_rename_table_render.py` (new) — golden-SQL per dialect. +- `tests/test_integration/test_write_ops_live.py` (new) — live round-trips. +- `tests/conftest.py` / fixtures — skip-if-unreachable postgres/mysql. +- `.github/workflows/python-run-pytest.yml` — service containers. +- `pyproject.toml` — ibis pin `>=12.0.0`. +- `CLAUDE.md` — correct ibis version note. + +## 14. Out of Scope (tracked elsewhere) + +- `create_index` / `drop_index` / `get_index_exists_sql` / + `get_list_indexes_sql` — capability-divergent, stay hook-required. +- Consumer migration (mountainash-wearables backend swap to postgres) — in + that repo after this ships. This spec's `upsert`-on-postgres is its enabler. +- Live warehouse testing — requires credentials/infra; deferred. From 620cbee295a7424ef7695942733d6de0536f1016 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 15:55:38 +1000 Subject: [PATCH 02/23] docs: apply Codex adversarial review to generic-default dialect ops spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 10 findings: MySQL ON DUPLICATE KEY conflict-targeting + NOTHING side-effects (documented divergences, §6.2); update_condition cross-family alias contract (§6.1); duplicate-source-row engine divergence (§11); explicit column ordering + target-type casting in subquery staging (§5.5); 21->20 dialect count + registry-iterating golden tests (§7/§8.4); render-vs-execution honesty + verification-basis labels (§7); CI fail-closed on unreachable live services (§8.2). Co-Authored-By: Claude Opus 4.8 (1M context) --- ...neric-default-dialect-operations-design.md | 218 +++++++++++++----- 1 file changed, 157 insertions(+), 61 deletions(-) diff --git a/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md index a2d64be..dd51e2c 100644 --- a/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md +++ b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md @@ -231,60 +231,127 @@ a subquery — `compiled_source()` returns `(SELECT … )` — used as the () AS src`. This is exactly Ibis 12's mechanism. No staging table, no cleanup, portable by construction. +**Column ordering (mandatory).** Every family renders an **explicit target +column list** and projects the source subquery columns **in target-column +order** — never `INSERT … SELECT *` or positional value lists. A source whose +columns are ordered `[name, id]` against a target `[id, name]` must not swap +values. The renderer derives the column list from the source schema, +intersected with the target schema, and projects both `INSERT` and the MERGE +`WHEN NOT MATCHED … INSERT (cols) VALUES (src.cols)` in that exact order. + +**Type alignment across the subquery boundary (mandatory).** The source +projection **casts each column to the target table's column type** via the +connection's `compiler.type_mapper` (the same type-parity mechanism +`add_columns` uses). This is required because an all-null source column +compiles as an untyped `NULL` literal that warehouse `MERGE` engines reject +or mis-coerce against a typed/non-nullable target, and because temporal / +decimal types are backend-sensitive. Columns present in the target but absent +from the source are omitted from the column list (engine default / NULL +applies); columns present in the source but absent from the target raise +`ValueError`. + ## 6. Semantics Mapping -The public semantics map onto each family as follows. All three honour the -identical public contract. +The public semantics map onto each family as below. The `ON CONFLICT` and +`MERGE` families honour an identical public contract; `ON DUPLICATE KEY` +honours it with two **documented divergences** (conflict targeting and the +`NOTHING` action), spelled out under the table — it is *not* claimed +byte-equivalent. | Public param | `ON CONFLICT` | `MERGE` | `ON DUPLICATE KEY` | |---|---|---|---| -| `conflict_columns` (composite) | `ON CONFLICT (c1,c2)` | `ON tgt.c1=src.c1 AND tgt.c2=src.c2` | unique-key implied; key cols excluded from SET | +| `conflict_columns` (composite) | `ON CONFLICT (c1,c2)` | `ON tgt.c1=src.c1 AND tgt.c2=src.c2` | **detection is by the table's unique indexes, not this list** — see §6.1 | | `conflict_action="UPDATE"` | `DO UPDATE SET …` | `WHEN MATCHED THEN UPDATE SET …` + `WHEN NOT MATCHED THEN INSERT …` | `ON DUPLICATE KEY UPDATE …` | -| `conflict_action="NOTHING"` | `DO NOTHING` | omit `WHEN MATCHED` (insert-if-absent only) | `… UPDATE c1=c1` (no-op self-assign) | +| `conflict_action="NOTHING"` | `DO NOTHING` | omit `WHEN MATCHED` (insert-if-absent only) | `… UPDATE k0=k0` self-assign — **not a true no-op**, see §6.2 | | `update_columns` (subset) | restrict `SET` list | restrict `WHEN MATCHED … SET` list | restrict `UPDATE` list | -| `update_condition` | `DO UPDATE SET … WHERE ` | `WHEN MATCHED AND THEN UPDATE` | unsupported → `ValueError` (MySQL has no per-row update predicate) | +| `update_condition` | `DO UPDATE SET … WHERE ` | `WHEN MATCHED AND THEN UPDATE` | unsupported → `ValueError` | | default `update_columns` | all non-key columns | all non-key columns | all non-key columns | -`update_condition` on `ON_DUPLICATE_KEY` raises `ValueError` (honest: the -family cannot express it) rather than silently dropping it. - -## 7. Coverage Matrix (all 21 registry dialects) - -Confidence: **live** = executed against a real engine in CI/local; **render** -= golden-SQL assertion only (no credentials/engine available); **n/a** = -`upsert_style=None`, honest `NotImplementedError`. - -| Dialect | `upsert_style` | Confidence | Notes | -|---|---|---|---| -| sqlite | on_conflict | **live** | in-memory | -| duckdb | on_conflict | **live** | in-memory; also exercises MERGE renderer (duckdb supports MERGE) | -| motherduck | on_conflict | render | duckdb engine | -| postgres | on_conflict | **live** | Docker; also exercises MERGE renderer (PG15+) | -| mysql | on_duplicate_key | **live** | Docker (mariadb, per Ibis) | -| singlestoredb | on_duplicate_key | render | MySQL-compatible | -| snowflake | merge | render | | -| bigquery | merge | render | | -| mssql | merge | render | sp_rename for rename | -| oracle | merge | render | | -| databricks | merge | render | Delta MERGE | -| exasol | merge | render | | -| trino | merge | render | connector-dependent at runtime | -| redshift | merge | render | postgres protocol but no `ON CONFLICT`; MERGE (2023+) | -| risingwave | on_conflict | render | postgres-wire table upsert | -| clickhouse | None | n/a | ReplacingMergeTree / ALTER UPDATE — divergent model | -| impala | None | n/a | MERGE only for Iceberg targets | -| materialize | None | n/a | restricts INSERT to write-only txns | -| druid | None | n/a | append-only analytics store | -| pyspark | None | n/a | MERGE only for Delta/Iceberg; Ibis marks notyet | - -`rename_table`: generic sqlglot default for **all** dialects (render-verified -across the registry; live on sqlite/duckdb/postgres/mysql). The override hook -stays available for any engine later found to diverge beyond sqlglot's -rendering. - -The matrix is shipped as a documented table in the module and asserted by the -golden-SQL tests (per-dialect rendered statement), so "render" coverage is -real verification of the emitted SQL, not an assumption. +### 6.1 `update_condition` alias contract (cross-family) + +`update_condition` is a raw SQL boolean expression. A bare string cannot be +both ON-CONFLICT-valid and MERGE-valid (`EXCLUDED.x` vs `src.x`), so the +package **fixes the alias contract**: the condition references the **incoming +row as `EXCLUDED.`** and the **existing row by the bare target table +name** — the ON CONFLICT convention. The MERGE renderer makes the same string +valid by aliasing the source subquery **`AS EXCLUDED`** and the target **`AS +`**, so `EXCLUDED.updated_at > orders.updated_at` renders +correctly in both families. The matched/not-matched semantics are equivalent: +a row matching the key but failing the condition stays matched and is neither +updated nor re-inserted (no duplicate). `on_duplicate_key` cannot express a +per-row update predicate → `update_condition` raises `ValueError` rather than +silently dropping it. + +### 6.2 `ON DUPLICATE KEY` documented divergences + +- **Conflict targeting:** MySQL/MariaDB `ON DUPLICATE KEY UPDATE` fires on a + collision with **any** unique or primary key, not a named subset. + `conflict_columns` therefore governs only which columns are *excluded from + the UPDATE set* (and is validated non-empty); it does **not** select the + detection key. Callers must ensure `conflict_columns` corresponds to the + table's intended unique constraint — on a table with multiple unique + constraints the engine may update on a different collision. Documented + limitation (§11); the `upsert_hook` override is the escape hatch for + stricter needs. We do **not** introspect constraints in this iteration. +- **`NOTHING` is not a true no-op:** rendered as `ON DUPLICATE KEY UPDATE + k0=k0` (self-assigning the first key column). Depending on table definition + this may advance `ON UPDATE CURRENT_TIMESTAMP` columns, fire UPDATE + triggers, take update locks, and alter affected-row counts — unlike `ON + CONFLICT DO NOTHING`. We deliberately do **not** use `INSERT IGNORE` (it + also silently suppresses unrelated type / NOT NULL / FK errors). Documented + (§11); strict-skip callers use the override hook. + +## 7. Coverage Matrix (all 20 registry dialects) + +The registry currently has **20** dialects; every one appears below. The +matrix is not hand-counted in tests — the golden-SQL suite **iterates the live +`DIALECTS` registry** (§8.4) and asserts each entry renders or is explicitly +`None`, so adding a 21st dialect *forces* a matrix decision (no hardcoded +count to drift). + +Columns: **Style** = assigned `upsert_style`. **Exec** = engine-execution +confidence: **live** (round-tripped against a real engine) / **render** +(rendered SQL asserted; engine acceptance and semantic support **not** +verified) / **n/a** (`upsert_style=None` → honest `NotImplementedError`). +**Basis** = how the style assignment was established: **verified** (live or +vendor-doc confirmed) / **inferred** (protocol/family compatibility) / +**unverified** (hypothesis pending docs/tests). + +| Dialect | Style | Exec | Basis | Notes | +|---|---|---|---|---| +| sqlite | on_conflict | **live** | verified | in-memory | +| duckdb | on_conflict | **live** | verified | in-memory; also exercises MERGE renderer (duckdb supports MERGE) | +| motherduck | on_conflict | render | inferred | duckdb engine | +| postgres | on_conflict | **live** | verified | Docker; also exercises MERGE renderer (PG15+) | +| mysql | on_duplicate_key | **live** | verified | Docker (mariadb, per Ibis) | +| singlestoredb | on_duplicate_key | render | inferred | MySQL-compatible wire/syntax | +| snowflake | merge | render | verified | vendor MERGE | +| bigquery | merge | render | verified | vendor MERGE | +| mssql | merge | render | verified | vendor MERGE; sp_rename for rename | +| oracle | merge | render | verified | vendor MERGE | +| databricks | merge | render | verified | Delta MERGE | +| exasol | merge | render | inferred | MERGE documented | +| trino | merge | render | inferred | MERGE is **connector-dependent** at runtime — may reject | +| redshift | merge | render | verified | postgres protocol but no `ON CONFLICT`; MERGE (AWS docs, 2023+) | +| risingwave | on_conflict | render | **unverified** | postgres-wire; ON CONFLICT on tables assumed, not confirmed | +| clickhouse | None | n/a | verified | ReplacingMergeTree / ALTER UPDATE — divergent model | +| impala | None | n/a | verified | MERGE only for Iceberg targets | +| materialize | None | n/a | verified | restricts INSERT to write-only txns | +| druid | None | n/a | verified | append-only analytics store | +| pyspark | None | n/a | verified | MERGE only for Delta/Iceberg; Ibis marks notyet | + +`rename_table`: generic sqlglot default for **all** dialects (rendered SQL +asserted across the registry; engine-executed live on +sqlite/duckdb/postgres/mysql). The override hook stays available for any +engine later found to diverge beyond sqlglot's rendering. + +**Render coverage is syntax-emission verification only** — it confirms the +exact SQL string sqlglot emits per dialect, *not* that the engine accepts or +semantically supports it. The public-facing support matrix carries the +**Exec** and **Basis** caveats above so consumers do not mistake a green +golden-SQL test for runtime support (e.g. Trino MERGE may render cleanly yet +be rejected by the connector). `unverified` rows are flagged as hypotheses +until a live test or vendor doc upgrades them. ## 8. Testing @@ -302,26 +369,37 @@ Connection parameters read from environment with Ibis-compatible defaults (`IBIS_TEST_POSTGRES_*` / `PG*`, `IBIS_TEST_MYSQL_*`) so the same env works locally and in CI. -### 8.2 Skip-if-unreachable fixtures +### 8.2 Skip-if-unreachable fixtures — but fail-closed in CI + +`postgres` / `mysql` fixtures attempt a connection; on failure behaviour is +**environment-gated** to avoid silently-green CI: + +- **Locally** (default): unreachable service → `pytest.skip(...)`, so a + developer without Docker still passes green. +- **In CI**: the workflow sets `MOUNTAINASH_REQUIRE_LIVE_DB=1`; under that flag + an unreachable required service **fails** (not skips). A misconfigured + service password/port then breaks the build instead of silently skipping the + promised live matrix. -`postgres` / `mysql` fixtures attempt a connection; on failure they -`pytest.skip(...)` (not fail), so a developer without Docker and a CI job -without the service still pass green. The live tests are marked -`@pytest.mark.integration` (existing marker). +Live tests are marked `@pytest.mark.integration` (existing marker). ### 8.3 CI The existing `python-run-pytest.yml` gains GitHub Actions `services:` containers for postgres and mariadb (native service-container support; no -compose needed in CI). A `make test-live` / hatch script brings the compose -services up locally. +compose needed in CI) and sets `MOUNTAINASH_REQUIRE_LIVE_DB=1` for the live +job. A `hatch` script (`test-live`) brings the local compose services up. ### 8.4 Test layers 1. **Unit / golden-SQL** (`tests/test_unit/backends/ibis/test_upsert_render.py`, - `test_rename_table_render.py`): for every registry dialect, assert the - exact rendered statement per `upsert_style` and for rename. No live engine. - This is where the full matrix is verified. + `test_rename_table_render.py`): the suite **iterates the live `DIALECTS` + registry** (not a hand-listed set) and, for each dialect, asserts the exact + rendered statement for its `upsert_style` — or, for `None`, asserts + `NotImplementedError` — and the rendered `rename_table`. Because it iterates + the registry, a newly added dialect with no decision fails the suite, + keeping the §7 matrix complete by construction. This layer verifies + **emitted SQL only**, not engine acceptance (§7). 2. **Live integration** (`tests/test_integration/test_write_ops_live.py`): sqlite + duckdb (in-memory) + postgres + mysql — round-trip upsert (insert + update + NOTHING + composite key + update_condition where @@ -358,16 +436,34 @@ services up locally. ## 11. Known Limitations +- **Duplicate source rows are the caller's responsibility.** If the source + frame contains two rows with the same conflict key, behaviour is + engine-divergent: `MERGE` raises a cardinality-violation error on most + engines (e.g. Redshift) when multiple source rows match one target row, + whereas `ON CONFLICT` / `ON DUPLICATE KEY` apply conflicts row-by-row with + order-dependent last-write-wins. The renderer does **not** deduplicate. + Callers must deduplicate the source on the conflict key for deterministic, + portable behaviour. Documented, not handled (an optional `deduplicate=` flag + is a future extension, not in this iteration). +- **MySQL-family conflict targeting** detects on *any* unique index, not + `conflict_columns` (§6.2) — unenforceable on tables with multiple unique + constraints; use the override hook for stricter needs. +- **MySQL-family `conflict_action="NOTHING"` is not a true no-op** (§6.2) — may + fire `ON UPDATE` timestamps / triggers / locks. Documented; not `INSERT + IGNORE`. - **Not concurrency-safe / not atomic** beyond the engine's own statement atomicity — single-statement upsert is atomic where the engine makes it so; no cross-statement transaction is added. - **Subquery staging** inlines the source data as a compiled `VALUES`/`SELECT` - subquery (Ibis's mechanism). Very large frames produce large SQL; callers - with bulk loads should use a staging table + `upsert`-from-table directly. - Acceptable and matches upstream Ibis behaviour. -- **Render-only dialects** (matrix §7) are verified at the SQL-emission layer, - not by execution; first real use on such an engine may surface - engine-specific quirks, handled then via the `upsert_hook` override seam. + subquery (Ibis's mechanism), with every column cast to the target type + (§5.5). Very large frames produce large SQL; callers with bulk loads should + use a staging table + `upsert`-from-table directly. Acceptable and matches + upstream Ibis behaviour. +- **Render-only dialects** (matrix §7) are verified at the SQL-emission layer + only, **not** by execution or semantic-support confirmation; first real use + on such an engine may surface engine-specific quirks (or outright rejection, + e.g. connector-dependent Trino MERGE), handled then via the `upsert_hook` + override seam. `unverified`-basis rows (e.g. risingwave) are hypotheses. - **`update_condition` semantics** differ subtly across families (pre- vs post-match predicate); documented per family in §6. From ea88152b48b099a24415963d043119252dafa962 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 17:10:42 +1000 Subject: [PATCH 03/23] docs: settle three open semantics Qs in dialect-ops spec - Q1 (MySQL conflict targeting): upgrade document-only -> introspect unique indexes + fail-closed ValueError on ambiguity (silent data corruption risk). - Q2 (update_condition): redesign from raw-SQL-string to an ibis-expression predicate (incoming, existing) -> ibis bool, rendered per-family via join->sqlglot-AST->alias-remap. Mechanism verified end-to-end by probe across duckdb/postgres/mysql/mssql/snowflake/bigquery/oracle/trino; portable in row refs, functions, and types; validated, no injection. - Q3 (MySQL NOTHING): unchanged (self-assign + documented side effects). Co-Authored-By: Claude Opus 4.8 (1M context) --- ...neric-default-dialect-operations-design.md | 146 ++++++++++++++---- 1 file changed, 113 insertions(+), 33 deletions(-) diff --git a/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md index dd51e2c..e398a9e 100644 --- a/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md +++ b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md @@ -77,6 +77,11 @@ the engine rejects while presenting as supported). ### 4.1 `upsert` (signature unchanged) ```python +# A conditional-update predicate: receives two ibis tables (the incoming +# source row-set and the existing target row-set, both with the target schema) +# and returns an ibis boolean. Rendered per-dialect through ibis/sqlglot. +ConditionPredicate = t.Callable[[ir.Table, ir.Table], ir.BooleanValue] + def upsert( self, name: str, @@ -85,7 +90,7 @@ def upsert( conflict_columns: list[str] | str, update_columns: list[str] | str | None = None, conflict_action: str = "UPDATE", # "UPDATE" | "NOTHING" - update_condition: str | None = None, + update_condition: ConditionPredicate | None = None, database: str | None = None, schema: str | None = None, ) -> IbisBackend: ... @@ -94,6 +99,17 @@ def upsert( Fluent. Composite `conflict_columns` is supported (unlike Ibis 12's native `upsert`, whose `on` is a single column — a key reason we render our own). +`update_condition` is an **ibis-expression predicate**, not a raw SQL string — +e.g. `lambda incoming, existing: incoming.updated_at > existing.updated_at` +("merge only when the incoming row is newer"). This is the conditional-upsert +capability that powers idempotent late-arriving-data merges and optimistic +last-write-wins. Expressed as ibis, it renders portably per dialect — row +references, **functions, and types alike** — is type-validated, and carries no +SQL-injection surface, consistent with the package's render-through-ibis +discipline. The previous raw-string form is dropped (pre-release clean break); +callers needing raw SQL use the `upsert_hook` override. See §6.1 for the +rendering mechanism (verified by probe, §9). + ### 4.2 `rename_table` (signature unchanged) ```python @@ -267,32 +283,67 @@ byte-equivalent. | `update_condition` | `DO UPDATE SET … WHERE ` | `WHEN MATCHED AND THEN UPDATE` | unsupported → `ValueError` | | default `update_columns` | all non-key columns | all non-key columns | all non-key columns | -### 6.1 `update_condition` alias contract (cross-family) - -`update_condition` is a raw SQL boolean expression. A bare string cannot be -both ON-CONFLICT-valid and MERGE-valid (`EXCLUDED.x` vs `src.x`), so the -package **fixes the alias contract**: the condition references the **incoming -row as `EXCLUDED.`** and the **existing row by the bare target table -name** — the ON CONFLICT convention. The MERGE renderer makes the same string -valid by aliasing the source subquery **`AS EXCLUDED`** and the target **`AS -`**, so `EXCLUDED.updated_at > orders.updated_at` renders -correctly in both families. The matched/not-matched semantics are equivalent: -a row matching the key but failing the condition stays matched and is neither -updated nor re-inserted (no duplicate). `on_duplicate_key` cannot express a -per-row update predicate → `update_condition` raises `ValueError` rather than -silently dropping it. +### 6.1 `update_condition` — ibis-expression predicate (cross-family rendering) + +`update_condition` is an ibis-expression predicate, not raw SQL (§4.1). The +caller receives two ibis tables — `incoming` (the source row-set) and +`existing` (the target row-set), both bound to the target's resolved schema — +and returns an ibis boolean. The renderer turns it into the per-family clause +through ibis + sqlglot, verified by probe (§9): + +1. Form `existing.join(incoming, predicate)` and obtain its **sqlglot AST** + via `ibis_conn.compiler.to_sqlglot(...)`. ibis already knows how to render + a two-table predicate as a join condition (it refused a bare two-table + boolean — the join is the supported path). +2. Identify ibis's auto-assigned join aliases by walking `exp.Table` nodes and + reading each alias's **underlying real table name** (deterministic — not + positional): `{ibis_alias → "incoming"/"existing"}`. +3. Extract the join's `ON` sub-AST and `.transform()` the column qualifiers to + the family's aliases: + - **ON CONFLICT** → incoming columns to `EXCLUDED`, existing columns to the + bare target table name → spliced into `DO UPDATE SET … WHERE `. + - **MERGE** → incoming to the source alias `src`, existing to the target + alias `tgt` → spliced into `WHEN MATCHED AND THEN UPDATE`. +4. Render the remapped sub-AST with the live dialect. + +Because the predicate is ibis, **functions and types render per-dialect too** +(`incoming.name.upper()` → `UPPER(...)`), not just column references — probe +output confirmed correct rendering across duckdb/postgres/mysql/mssql/ +snowflake/bigquery/oracle/trino. The matched/not-matched semantics are +equivalent across the two families: a row matching the key but failing the +condition stays matched and is neither updated nor re-inserted (no duplicate). + +`on_duplicate_key` cannot express a per-row update predicate → a non-`None` +`update_condition` raises `ValueError` rather than silently dropping it. + +> **Implementation note (dialect names).** The remapped sub-AST is rendered +> with the live connection's *sqlglot* dialect (`ibis_conn.compiler.dialect`), +> not ibis's backend name — these differ (ibis `mssql` ↔ sqlglot `tsql`). The +> probe used the live compiler dialect throughout. The alias-remap helper +> lives in `_render.py` (§5.2). ### 6.2 `ON DUPLICATE KEY` documented divergences -- **Conflict targeting:** MySQL/MariaDB `ON DUPLICATE KEY UPDATE` fires on a - collision with **any** unique or primary key, not a named subset. - `conflict_columns` therefore governs only which columns are *excluded from - the UPDATE set* (and is validated non-empty); it does **not** select the - detection key. Callers must ensure `conflict_columns` corresponds to the - table's intended unique constraint — on a table with multiple unique - constraints the engine may update on a different collision. Documented - limitation (§11); the `upsert_hook` override is the escape hatch for - stricter needs. We do **not** introspect constraints in this iteration. +- **Conflict targeting (introspect + fail-closed):** MySQL/MariaDB `ON + DUPLICATE KEY UPDATE` fires on a collision with **any** unique or primary + key, not a named subset — so a silent contract violation is possible + (`conflict_columns=["external_id"]` updating a row that collided on + `email`). Because that failure **silently corrupts the data the caller + intended**, the `on_duplicate_key` renderer does **not** ship the + document-only contract — it introspects the target's unique/PK indexes + (`information_schema.STATISTICS` — cheap, and `upsert` already issues + existence checks) and validates: + - exactly one unique/PK index, equal to `conflict_columns` → proceed; + - additional unique constraints exist that make detection ambiguous w.r.t. + `conflict_columns` → **raise `ValueError`** naming them, pointing at the + `upsert_hook` override; + - no unique index matches `conflict_columns` → raise (it would never + conflict-detect on those columns anyway). + + `conflict_columns` also governs which columns are excluded from the UPDATE + set (validated non-empty). This is the one MySQL divergence we *fail closed* + on rather than document, because — unlike the `NOTHING` side effects below — + the failure is data corruption, not a cosmetic effect. - **`NOTHING` is not a true no-op:** rendered as `ON DUPLICATE KEY UPDATE k0=k0` (self-assigning the first key column). Depending on table definition this may advance `ON UPDATE CURRENT_TIMESTAMP` columns, fire UPDATE @@ -417,6 +468,18 @@ job. A `hatch` script (`test-live`) brings the local compose services up. - Ibis 12 `SQLBackend.upsert` exists (MERGE-based) but its `on` is a single column — insufficient for the composite natural keys real consumers use — which is why we render our own, using Ibis's `sge.merge` shape as template. +- **Conditional-predicate rendering (§6.1) probed end-to-end.** A bare + two-table ibis boolean is rejected by ibis (`RelationError: … multiple base + table references`), but `existing.join(incoming, predicate)` → + `compiler.to_sqlglot` → extract the `ON` sub-AST → remap ibis's auto-aliases + (identified by each `exp.Table`'s underlying name, not positionally) → + `.sql(dialect=…)` produces correct, portable output. Verified: a compound + predicate with a function (`incoming.name.upper() != existing.name.upper()`) + rendered to `"EXCLUDED"."…" > "target"."…" AND UPPER(…) <> UPPER(…)` for the + ON CONFLICT alias mapping and `"src"…/"tgt"…` for MERGE, with correct + per-dialect quoting/functions across duckdb/postgres/mysql/mssql/snowflake/ + bigquery/oracle/trino. Implementation depth is bounded to one `_render.py` + helper. ## 10. Error Handling & Edge Cases @@ -426,8 +489,14 @@ job. A `hatch` script (`test-live`) brings the local compose services up. - `conflict_action="NOTHING"` with `update_columns`/`update_condition` → warn-and-ignore (preserves current behaviour), except MERGE where NOTHING simply omits the matched clause. -- `update_condition` on `on_duplicate_key` → `ValueError` (family cannot - express it). +- `update_condition` (a predicate) on `on_duplicate_key` → `ValueError` + (family cannot express a per-row update predicate). +- `on_duplicate_key` where `conflict_columns` is ambiguous or unmatched + against the table's unique/PK indexes → `ValueError` (introspection + fail-closed, §6.2). +- `update_condition` predicate that references a column absent from the target + schema → `ValueError` (caught when binding the predicate to the resolved + `incoming`/`existing` tables — ibis raises on the unknown field). - `upsert_style=None` → `NotImplementedError` naming the dialect. - Target table absent → `ValueError` (preserves current behaviour). - Identifiers: `name`/`database`/rename names validated simple (non-dotted) @@ -446,8 +515,11 @@ job. A `hatch` script (`test-live`) brings the local compose services up. portable behaviour. Documented, not handled (an optional `deduplicate=` flag is a future extension, not in this iteration). - **MySQL-family conflict targeting** detects on *any* unique index, not - `conflict_columns` (§6.2) — unenforceable on tables with multiple unique - constraints; use the override hook for stricter needs. + `conflict_columns` (§6.2). Rather than silently mis-target, the renderer + introspects unique/PK indexes and **raises `ValueError`** when + `conflict_columns` is ambiguous or unmatched — fail-closed, because the + failure would be data corruption. The `upsert_hook` override is the escape + hatch for tables that legitimately need the broader behaviour. - **MySQL-family `conflict_action="NOTHING"` is not a true no-op** (§6.2) — may fire `ON UPDATE` timestamps / triggers / locks. Documented; not `INSERT IGNORE`. @@ -464,8 +536,12 @@ job. A `hatch` script (`test-live`) brings the local compose services up. on such an engine may surface engine-specific quirks (or outright rejection, e.g. connector-dependent Trino MERGE), handled then via the `upsert_hook` override seam. `unverified`-basis rows (e.g. risingwave) are hypotheses. -- **`update_condition` semantics** differ subtly across families (pre- vs - post-match predicate); documented per family in §6. +- **`update_condition`** renders to different *clauses* per family (`DO UPDATE + … WHERE` vs `WHEN MATCHED AND`), but the observable semantics are equivalent + (§6.1): a key-matched row failing the predicate is left untouched and not + re-inserted. The predicate is ibis-expressed, so functions/types are + portable; the alias-remap mechanism is the one piece of real implementation + depth (verified, §9). ## 12. Dependency @@ -481,14 +557,18 @@ job. A `hatch` script (`test-live`) brings the local compose services up. - `dialects/_registry.py` — `UpsertStyle` enum, `upsert_style` field; assign styles per the matrix; remove the three `upsert_hook=duckdb_family_upsert` registrations. -- `backends/ibis/_render.py` (new) — shared rendering primitives. +- `backends/ibis/_render.py` (new) — shared rendering primitives **and** the + conditional-predicate compiler (`ConditionPredicate` type; join → sqlglot + AST → alias-remap → per-family clause, §6.1). - `backends/ibis/operations.py` — `_generic_rename_table`, `_generic_upsert`, - `_render_on_conflict`, `_render_merge`, `_render_on_duplicate_key`; migrate + `_render_on_conflict`, `_render_merge`, `_render_on_duplicate_key` (incl. the + unique-index introspection + fail-closed validation, §6.2); migrate `_generic_add_columns` to the helper; **delete** `duckdb_family_upsert` — the duckdb family routes through `_render_on_conflict` via `upsert_style=ON_CONFLICT`. - `backends/ibis/backend.py` — `upsert`/`rename_table` dispatch updated to the - hook-or-generic shape. + hook-or-generic shape; `upsert`'s `update_condition` param retyped from + `str | None` to `ConditionPredicate | None`. - `compose.yaml` (new) — postgres + mariadb stock services. - `tests/test_unit/backends/ibis/test_upsert_render.py`, `test_rename_table_render.py` (new) — golden-SQL per dialect. From 88caab2e844b57b2c4d35d8b61b8de3bcd09b16e Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 17:27:36 +1000 Subject: [PATCH 04/23] docs: apply Codex 2nd-pass review to dialect-ops spec (11 findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1 sentinel-named binding (__ma_incoming__/__ma_existing__) + alias identity, not underlying-name. H2 target aliased in ON CONFLICT (INSERT..AS tgt) + per-dialect capability flag. H3 accepted predicate grammar + validator (reject aggregate/window/subquery). H4 refine §2 principle: bounded preflight validation allowed in generic defaults, distinct from catalog-defined ops. M5 extraction-faithfulness test-guarded. M6/M7 strict prove-safe-or-raise preflight fails closed on prefix/functional/nullable unique indexes. M8 DDL race documented. M9 condition grammar boundary -> upsert_hook (no raw string). L10 cross-ref fix. L11 explicit validation precedence (§10). Co-Authored-By: Claude Opus 4.8 (1M context) --- ...neric-default-dialect-operations-design.md | 205 +++++++++++++----- 1 file changed, 147 insertions(+), 58 deletions(-) diff --git a/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md index e398a9e..74776d9 100644 --- a/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md +++ b/docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md @@ -36,15 +36,24 @@ reflect the class: statement-family choice vary. These get a **generic default** (sqlglot rendered) + an **optional override hook**. Default: *works everywhere it can be expressed.* -- **Capability-divergent** — the operation does not exist on some engines, - or reads dialect-specific system catalogs. These stay **hook-required**; - absent a hook, `NotImplementedError`. Default: *honestly unsupported.* +- **Capability-divergent** — the operation's *primary mechanism* does not + exist on some engines, or **is** a dialect-specific catalog read (e.g. + `list_indexes`). These stay **hook-required**; absent a hook, + `NotImplementedError`. Default: *honestly unsupported.* `upsert` and `rename_table` are the former. `create_index` / `drop_index` and the index-catalog queries are the latter and are **out of scope** (no secondary indexes on BigQuery/Snowflake — a generic default would emit DDL the engine rejects while presenting as supported). +**Preflight validation vs catalog-defined operations.** The distinction is the +operation's *primary mechanism*, not whether it touches a catalog at all. A +generic default MAY issue a bounded **preflight validation** query to *fail +closed* on a case it cannot safely render (e.g. the MySQL unique-index check in +§6.2) — that is a safety gate, not the operation. What stays hook-required is +an operation whose primary purpose *is* the catalog read. This keeps §6.2's +fail-closed validation consistent with the discriminator principle. + ## 3. Goals / Non-Goals **Goals** @@ -276,7 +285,7 @@ byte-equivalent. | Public param | `ON CONFLICT` | `MERGE` | `ON DUPLICATE KEY` | |---|---|---|---| -| `conflict_columns` (composite) | `ON CONFLICT (c1,c2)` | `ON tgt.c1=src.c1 AND tgt.c2=src.c2` | **detection is by the table's unique indexes, not this list** — see §6.1 | +| `conflict_columns` (composite) | `ON CONFLICT (c1,c2)` | `ON tgt.c1=src.c1 AND tgt.c2=src.c2` | **detection is by the table's unique indexes, not this list** — see §6.2 | | `conflict_action="UPDATE"` | `DO UPDATE SET …` | `WHEN MATCHED THEN UPDATE SET …` + `WHEN NOT MATCHED THEN INSERT …` | `ON DUPLICATE KEY UPDATE …` | | `conflict_action="NOTHING"` | `DO NOTHING` | omit `WHEN MATCHED` (insert-if-absent only) | `… UPDATE k0=k0` self-assign — **not a true no-op**, see §6.2 | | `update_columns` (subset) | restrict `SET` list | restrict `WHEN MATCHED … SET` list | restrict `UPDATE` list | @@ -291,21 +300,45 @@ caller receives two ibis tables — `incoming` (the source row-set) and and returns an ibis boolean. The renderer turns it into the per-family clause through ibis + sqlglot, verified by probe (§9): +0. **Bind the two tables under reserved sentinel names.** `incoming` and + `existing` are constructed as ibis tables named `__ma_incoming__` / + `__ma_existing__` — names the package reserves and that no real target can + carry (a target colliding with a sentinel raises `ValueError`). This is how + step 2 stays unambiguous even if the user's target is named `incoming`, + `excluded`, etc. (Codex finding: never identify sides positionally or by a + user-controllable name.) 1. Form `existing.join(incoming, predicate)` and obtain its **sqlglot AST** - via `ibis_conn.compiler.to_sqlglot(...)`. ibis already knows how to render - a two-table predicate as a join condition (it refused a bare two-table - boolean — the join is the supported path). + via `ibis_conn.compiler.to_sqlglot(...)`. ibis renders a two-table + predicate only as a join condition (it refuses a bare two-table boolean) — + the join is the supported path. 2. Identify ibis's auto-assigned join aliases by walking `exp.Table` nodes and - reading each alias's **underlying real table name** (deterministic — not - positional): `{ibis_alias → "incoming"/"existing"}`. + matching each alias's underlying name to the **sentinels** from step 0: + `{ibis_alias → incoming|existing}`. Sentinel matching is unambiguous by + construction. 3. Extract the join's `ON` sub-AST and `.transform()` the column qualifiers to - the family's aliases: - - **ON CONFLICT** → incoming columns to `EXCLUDED`, existing columns to the - bare target table name → spliced into `DO UPDATE SET … WHERE `. - - **MERGE** → incoming to the source alias `src`, existing to the target - alias `tgt` → spliced into `WHEN MATCHED AND THEN UPDATE`. + **explicit, controlled aliases** (never a bare table name): + - **ON CONFLICT** → the INSERT is rendered `INSERT INTO AS tgt …` + so the existing row has a dedicated alias; incoming columns map to + `EXCLUDED`, existing columns to `tgt` → spliced into `DO UPDATE SET … + WHERE `. (Postgres/SQLite support `INSERT … AS alias`; a dialect + that cannot alias the target in ON CONFLICT is recorded in §7 and routes + to the `upsert_hook`.) + - **MERGE** → incoming to source alias `src`, existing to target alias + `tgt` → spliced into `WHEN MATCHED AND THEN UPDATE`. 4. Render the remapped sub-AST with the live dialect. +**Accepted predicate grammar (validated).** The predicate must be a **scalar +row predicate** over `incoming`/`existing` columns, literals, and +ibis-modelled scalar functions/operators. Before rendering, the renderer walks +the ibis expression and **raises `ValueError`** if it contains an aggregation, +window function, or any subquery/`EXISTS`/third-table reference — these compile +into the join but are invalid or semantically wrong inside `DO UPDATE … WHERE` +/ `WHEN MATCHED AND`. Conditions outside this grammar (e.g. correlated +`EXISTS`, a third-table lookup, a vendor function ibis does not model) are +**out of the generic API's scope by design** and serviced by the `upsert_hook` +override; we do not re-introduce a raw-SQL condition string (it would restore +exactly the portability/injection problems the predicate form removes). + Because the predicate is ibis, **functions and types render per-dialect too** (`incoming.name.upper()` → `UPPER(...)`), not just column references — probe output confirmed correct rendering across duckdb/postgres/mysql/mssql/ @@ -314,7 +347,8 @@ equivalent across the two families: a row matching the key but failing the condition stays matched and is neither updated nor re-inserted (no duplicate). `on_duplicate_key` cannot express a per-row update predicate → a non-`None` -`update_condition` raises `ValueError` rather than silently dropping it. +`update_condition` raises `ValueError` (validated **before** any +`conflict_action` handling — see §10). > **Implementation note (dialect names).** The remapped sub-AST is rendered > with the live connection's *sqlglot* dialect (`ibis_conn.compiler.dialect`), @@ -324,26 +358,33 @@ condition stays matched and is neither updated nor re-inserted (no duplicate). ### 6.2 `ON DUPLICATE KEY` documented divergences -- **Conflict targeting (introspect + fail-closed):** MySQL/MariaDB `ON +- **Conflict targeting (prove-safe-or-raise preflight):** MySQL/MariaDB `ON DUPLICATE KEY UPDATE` fires on a collision with **any** unique or primary key, not a named subset — so a silent contract violation is possible (`conflict_columns=["external_id"]` updating a row that collided on `email`). Because that failure **silently corrupts the data the caller - intended**, the `on_duplicate_key` renderer does **not** ship the - document-only contract — it introspects the target's unique/PK indexes - (`information_schema.STATISTICS` — cheap, and `upsert` already issues - existence checks) and validates: - - exactly one unique/PK index, equal to `conflict_columns` → proceed; - - additional unique constraints exist that make detection ambiguous w.r.t. - `conflict_columns` → **raise `ValueError`** naming them, pointing at the - `upsert_hook` override; - - no unique index matches `conflict_columns` → raise (it would never - conflict-detect on those columns anyway). + intended**, the `on_duplicate_key` renderer runs a **bounded preflight + validation** (a generic-default safety gate, not a catalog-defined + operation — §2) over `information_schema.STATISTICS` and renders **only when + it can prove the safe case**, failing closed otherwise: + - **exactly one** unique/PK index whose column set **equals** + `conflict_columns`, **and** every such column is `NOT NULL`, **and** the + index has **no prefix** (`SUB_PART IS NULL`) and is **not** a + functional/expression index → proceed; + - any of: a second unique/PK index, a prefix index (`SUB_PART`), a + functional/expression index, or a **nullable** conflict column → **raise + `ValueError`** naming the offending index/column and pointing at the + `upsert_hook` override. (Prefix indexes detect on a truncated value; + nullable unique columns are NULL-distinct in MySQL, so duplicates insert + instead of updating — both would silently violate the contract, so we + refuse rather than guess.) `conflict_columns` also governs which columns are excluded from the UPDATE set (validated non-empty). This is the one MySQL divergence we *fail closed* on rather than document, because — unlike the `NOTHING` side effects below — - the failure is data corruption, not a cosmetic effect. + the failure is data corruption. A DDL change between preflight and execution + (another session adding/dropping a unique index) is an accepted + schema-concurrency race, documented in §11. - **`NOTHING` is not a true no-op:** rendered as `ON DUPLICATE KEY UPDATE k0=k0` (self-assigning the first key column). Depending on table definition this may advance `ON UPDATE CURRENT_TIMESTAMP` columns, fire UPDATE @@ -396,6 +437,15 @@ asserted across the registry; engine-executed live on sqlite/duckdb/postgres/mysql). The override hook stays available for any engine later found to diverge beyond sqlglot's rendering. +**ON-CONFLICT target aliasing (§6.1, only relevant when `update_condition` is +supplied):** referencing the *existing* row in the condition requires aliasing +the target in the INSERT (`INSERT INTO t AS tgt …`). Postgres and SQLite +support this; the implementation verifies it per `on_conflict`-family dialect +and records a per-dialect capability flag. A dialect that cannot alias its +target *and* is given an `update_condition` raises `ValueError` pointing at the +`upsert_hook` (unconditional upserts are unaffected). This flag is set from +live/golden verification, not assumed. + **Render coverage is syntax-emission verification only** — it confirms the exact SQL string sqlglot emits per dialect, *not* that the engine accepts or semantically supports it. The public-facing support matrix carries the @@ -451,11 +501,21 @@ job. A `hatch` script (`test-live`) brings the local compose services up. the registry, a newly added dialect with no decision fails the suite, keeping the §7 matrix complete by construction. This layer verifies **emitted SQL only**, not engine acceptance (§7). -2. **Live integration** (`tests/test_integration/test_write_ops_live.py`): +2. **Conditional-predicate rendering** (`test_upsert_condition_render.py`): + asserts the §6.1 mechanism is faithful and bounded — a constant predicate, + a compound predicate, a null-check, and a function predicate each render to + the expected ON-CONFLICT and MERGE clauses with correct sentinel→alias + remapping; and out-of-grammar predicates (aggregate, window, subquery/ + `EXISTS`) each raise `ValueError`. Guards against ibis rewriting a predicate + non-faithfully (§11). +3. **MySQL preflight** (`test_upsert_mysql_preflight.py`, live): single-PK + table proceeds; multi-unique, prefix-index, and nullable-unique tables each + raise `ValueError`. +4. **Live integration** (`tests/test_integration/test_write_ops_live.py`): sqlite + duckdb (in-memory) + postgres + mysql — round-trip upsert - (insert + update + NOTHING + composite key + update_condition where - supported) and rename, asserting data outcomes. -3. **Consolidation regression**: existing `test_add_columns.py` stays green + (insert + update + NOTHING + composite key + conditional update via the + predicate form) and rename, asserting data outcomes. +5. **Consolidation regression**: existing `test_add_columns.py` stays green unchanged after the helper extraction. ## 9. Verification already performed (sqlglot transpile probe, ibis 12 env) @@ -483,25 +543,31 @@ job. A `hatch` script (`test-live`) brings the local compose services up. ## 10. Error Handling & Edge Cases -- `conflict_action` ∉ {UPDATE, NOTHING} → `ValueError`. -- `conflict_action="UPDATE"` with no updatable columns (all columns are keys - and no `update_columns`) → `ValueError` (preserves current behaviour). -- `conflict_action="NOTHING"` with `update_columns`/`update_condition` → - warn-and-ignore (preserves current behaviour), except MERGE where NOTHING - simply omits the matched clause. -- `update_condition` (a predicate) on `on_duplicate_key` → `ValueError` - (family cannot express a per-row update predicate). -- `on_duplicate_key` where `conflict_columns` is ambiguous or unmatched - against the table's unique/PK indexes → `ValueError` (introspection - fail-closed, §6.2). -- `update_condition` predicate that references a column absent from the target - schema → `ValueError` (caught when binding the predicate to the resolved - `incoming`/`existing` tables — ibis raises on the unknown field). -- `upsert_style=None` → `NotImplementedError` naming the dialect. -- Target table absent → `ValueError` (preserves current behaviour). -- Identifiers: `name`/`database`/rename names validated simple (non-dotted) - via the existing `_validate_simple_identifier`; quoted per dialect. Dotted - qualified names out of scope (consistent with `add_columns`). +**Validation precedence (ordered — earlier checks fire first):** + +1. `upsert_style is None` → `NotImplementedError` naming the dialect. +2. Target table absent → `ValueError` (preserves current behaviour). +3. `name`/`database`/rename names not simple (dotted) → `ValueError` via + `_validate_simple_identifier`; dotted qualified names out of scope + (consistent with `add_columns`). +4. `conflict_action` ∉ {UPDATE, NOTHING} → `ValueError`. +5. **`update_condition` is validated unconditionally, before any + `conflict_action`-specific handling** (resolves the precedence ambiguity): + - on `on_duplicate_key` → always `ValueError` (family cannot express a + per-row update predicate), regardless of `conflict_action`; + - violates the accepted predicate grammar (aggregate / window / + subquery / `EXISTS` / third-table — §6.1) → `ValueError`; + - references a column absent from the target schema → `ValueError` (ibis + raises when the predicate binds to the resolved `incoming`/`existing`). +6. `on_duplicate_key` preflight: `conflict_columns` ambiguous / unmatched / + prefix / functional / nullable against the unique-index introspection → + `ValueError` (fail-closed, §6.2). +7. `conflict_action="UPDATE"` with no updatable columns (all columns are keys + and no `update_columns`) → `ValueError` (preserves current behaviour). +8. `conflict_action="NOTHING"` with `update_columns` → warn-and-ignore + (preserves current behaviour); on MERGE, NOTHING simply omits the matched + clause. (`update_condition` is already rejected/handled at step 5, so there + is no NOTHING-plus-condition ambiguity.) ## 11. Known Limitations @@ -515,11 +581,27 @@ job. A `hatch` script (`test-live`) brings the local compose services up. portable behaviour. Documented, not handled (an optional `deduplicate=` flag is a future extension, not in this iteration). - **MySQL-family conflict targeting** detects on *any* unique index, not - `conflict_columns` (§6.2). Rather than silently mis-target, the renderer - introspects unique/PK indexes and **raises `ValueError`** when - `conflict_columns` is ambiguous or unmatched — fail-closed, because the - failure would be data corruption. The `upsert_hook` override is the escape - hatch for tables that legitimately need the broader behaviour. + `conflict_columns` (§6.2). The renderer runs a prove-safe-or-raise preflight + and **raises `ValueError`** on ambiguous / unmatched / prefix / functional / + nullable unique-index cases — fail-closed, because the failure would be data + corruption. A DDL change between preflight and execution is an accepted + schema-concurrency race (same class as the general non-atomicity below). The + `upsert_hook` override is the escape hatch for tables that legitimately need + the broader behaviour. +- **Conditional-predicate extraction faithfulness.** §6.1 extracts the join's + `ON` sub-AST as the rendered predicate; ibis could in principle normalise or + fold the predicate during compilation. This is treated as a **test-guarded + assumption**: §8.4 requires predicate-rendering tests across constant, + compound, null-check, and function shapes, plus rejection tests for the + out-of-grammar shapes (aggregate/window/subquery). If a future ibis release + rewrites a predicate non-faithfully, those tests fail loudly. +- **Conditional-predicate grammar boundary.** `update_condition` supports + scalar row predicates over incoming/existing columns, literals, and + ibis-modelled functions (§6.1). Conditions needing correlated `EXISTS`, a + third-table lookup, or a vendor function ibis does not model are **out of the + generic API by design** and serviced by the `upsert_hook` override; a + raw-SQL condition string is deliberately not offered (it would restore the + portability/injection problems the predicate form removes). - **MySQL-family `conflict_action="NOTHING"` is not a true no-op** (§6.2) — may fire `ON UPDATE` timestamps / triggers / locks. Documented; not `INSERT IGNORE`. @@ -557,9 +639,12 @@ job. A `hatch` script (`test-live`) brings the local compose services up. - `dialects/_registry.py` — `UpsertStyle` enum, `upsert_style` field; assign styles per the matrix; remove the three `upsert_hook=duckdb_family_upsert` registrations. -- `backends/ibis/_render.py` (new) — shared rendering primitives **and** the - conditional-predicate compiler (`ConditionPredicate` type; join → sqlglot - AST → alias-remap → per-family clause, §6.1). +- `backends/ibis/_render.py` (new) — shared rendering primitives; the + conditional-predicate compiler (`ConditionPredicate` type; sentinel-named + binding → join → sqlglot AST → sentinel-keyed alias-remap → per-family + clause, §6.1); the **predicate-grammar validator** (rejects aggregate / + window / subquery shapes); and the reserved sentinel names + (`__ma_incoming__` / `__ma_existing__`). - `backends/ibis/operations.py` — `_generic_rename_table`, `_generic_upsert`, `_render_on_conflict`, `_render_merge`, `_render_on_duplicate_key` (incl. the unique-index introspection + fail-closed validation, §6.2); migrate @@ -572,6 +657,10 @@ job. A `hatch` script (`test-live`) brings the local compose services up. - `compose.yaml` (new) — postgres + mariadb stock services. - `tests/test_unit/backends/ibis/test_upsert_render.py`, `test_rename_table_render.py` (new) — golden-SQL per dialect. +- `tests/test_unit/backends/ibis/test_upsert_condition_render.py` (new) — + conditional-predicate rendering + out-of-grammar rejection (§8.4). +- `tests/test_integration/test_upsert_mysql_preflight.py` (new, live) — MySQL + unique-index preflight (proceed / multi-unique / prefix / nullable raises). - `tests/test_integration/test_write_ops_live.py` (new) — live round-trips. - `tests/conftest.py` / fixtures — skip-if-unreachable postgres/mysql. - `.github/workflows/python-run-pytest.yml` — service containers. From 106edb3aeb480212842fc4906c8bb5fe9136393b Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 21:24:34 +1000 Subject: [PATCH 05/23] docs: implementation plan for generic-default dialect operations 10 TDD tasks sequenced for continuous green: _render.py primitives + add_columns migration; docker test infra (postgres+mariadb, skip/fail-closed fixtures, ibis>=12); UpsertStyle field + per-matrix assignment; generic rename_table; conditional-predicate compiler (sentinel join->AST->remap + grammar validator); three upsert renderers (ON CONFLICT / MERGE / ON DUPLICATE KEY + MySQL preflight); cutover retiring duckdb_family_upsert; full regression. Golden-SQL iterates the live DIALECTS registry; live round-trips on sqlite/duckdb/postgres/mysql. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...6-29-generic-default-dialect-operations.md | 1406 +++++++++++++++++ 1 file changed, 1406 insertions(+) create mode 100644 docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md diff --git a/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md b/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md new file mode 100644 index 0000000..5b9d760 --- /dev/null +++ b/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md @@ -0,0 +1,1406 @@ +# Generic-Default Dialect Operations Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `IbisBackend.upsert` work across three SQL upsert families (`ON CONFLICT`, `MERGE`, MySQL `ON DUPLICATE KEY UPDATE`) and `IbisBackend.rename_table` work on every dialect whose rename is expressible — via sqlglot-rendered generic defaults dispatched off a new `DialectSpec.upsert_style`, with the override hooks retained. + +**Architecture:** A shared `_render.py` helper renders identifiers/types/sources and compiles `update_condition` predicates off the live connection's own sqlglot compiler. `upsert`/`rename_table` dispatch hook-or-generic; the generic upsert branches on `upsert_style` into three sqlglot-AST renderers. Source rows are staged as a compiled subquery (Ibis's own mechanism), not a temp table. Live-tested on sqlite/duckdb/postgres/mysql (Docker); golden-SQL render assertions cover the full 20-dialect registry. + +**Tech Stack:** Python 3.12, ibis-framework >= 12.0.0 (env has 12.0.0), sqlglot 30.x (`sqlglot.expressions as sge` / `from sqlglot import exp`), polars, pytest, hatch + uv, Docker Compose (postgres + mariadb). + +**Spec:** `docs/superpowers/specs/2026-06-29-generic-default-dialect-operations-design.md` + +## Global Constraints + +- **Run everything in the hatch test env:** `hatch run test:test-target-quick ` (quick, no coverage). NEVER the stale `.venv`. +- **Lint:** `hatch run ruff:check src` and (for new test files) `hatch run ruff:check ` — the `ruff:check` script is hardcoded to `./src`, so test paths must be appended explicitly. Every commit ruff-clean. +- **Types:** `hatch run mypy:check` stays `Success` (0 errors). Resolve new findings in touched files only; do not touch pre-existing `iceberg.*` debt. +- **Render off the LIVE connection** — types via `ibis_conn.compiler.type_mapper.to_string(dtype)` (create_table parity); dialect via `ibis_conn.compiler.dialect` (a sqlglot dialect, NOT ibis's backend name — ibis `mssql` ↔ sqlglot `tsql`). Never a hand-written type map. +- **One generic default + optional override.** `upsert_hook` / `rename_table_hook` default `None`; when `None` the generic path runs. `upsert_style=None` → honest `NotImplementedError`. +- **Simple identifiers only** — `name`/`database`/rename names validated via the existing `_validate_simple_identifier`; dotted names raise `ValueError` (consistent with `add_columns`). +- **Explicit column lists + target-type casts** in every upsert family (no `SELECT *`, no positional values); source columns projected in target-column order. +- **Reserved sentinels** `__ma_incoming__` / `__ma_existing__` for the condition compiler; a target colliding with a sentinel raises `ValueError`. +- **20 registry dialects** — golden tests iterate the live `DIALECTS` registry, never a hardcoded list/count. +- **Targeted local test backends:** in-memory `sqlite`/`duckdb` always; `postgres`/`mysql` via Docker, skip-if-unreachable locally, fail-closed in CI (`MOUNTAINASH_REQUIRE_LIVE_DB=1`). + +--- + +## File Structure + +- `src/mountainash_data/backends/ibis/_render.py` (new) — rendering primitives + the conditional-predicate compiler + predicate-grammar validator + sentinel names. One responsibility: turn ibis/names/types into dialect-correct SQL fragments off a live connection. +- `src/mountainash_data/backends/ibis/operations.py` (modify) — add `_generic_rename_table`, `_generic_upsert`, `_render_on_conflict`, `_render_merge`, `_render_on_duplicate_key`; migrate `_generic_add_columns` to `_render.py`; delete `duckdb_family_upsert` at cutover. +- `src/mountainash_data/backends/ibis/dialects/_registry.py` (modify) — `UpsertStyle` enum, `upsert_style` field, per-dialect style assignment, remove `upsert_hook=duckdb_family_upsert` registrations at cutover. +- `src/mountainash_data/backends/ibis/backend.py` (modify) — `upsert`/`rename_table` dispatch to hook-or-generic; retype `update_condition`. +- `compose.yaml` (new, repo root) — stock postgres + mariadb services. +- `tests/conftest.py` / `tests/fixtures/` (modify) — `postgres_backend` / `mysql_backend` skip-if-unreachable fixtures. +- `tests/test_unit/backends/ibis/test_render_primitives.py`, `test_rename_table_render.py`, `test_upsert_render.py`, `test_upsert_condition_render.py` (new) — golden-SQL / unit. +- `tests/test_integration/test_write_ops_live.py`, `test_upsert_mysql_preflight.py` (new) — live round-trips. +- `.github/workflows/python-run-pytest.yml` (modify) — service containers. +- `pyproject.toml` (modify) — ibis pin `>=12.0.0`. `CLAUDE.md` (modify) — correct stale ibis note. + +--- + +### Task 1: `_render.py` rendering primitives + migrate `add_columns` + +**Files:** +- Create: `src/mountainash_data/backends/ibis/_render.py` +- Modify: `src/mountainash_data/backends/ibis/operations.py` (`_generic_add_columns` uses `quote_identifier`) +- Test: `tests/test_unit/backends/ibis/test_render_primitives.py` (create) + +**Interfaces:** +- Produces: + - `dialect_of(ibis_conn) -> t.Any` — returns `ibis_conn.compiler.dialect` (sqlglot dialect). + - `quote_identifier(name: str, dialect: t.Any) -> str` — `exp.to_identifier(name, quoted=True).sql(dialect=dialect)`. + - `qualified_name(parts: list[str], dialect: t.Any) -> str` — `".".join(quote_identifier(p, dialect) for p in parts)`. + - `render_type(type_mapper: t.Any, dtype: t.Any) -> str` — `type_mapper.to_string(dtype)`. + +- [ ] **Step 1: Write the failing test** + +Create `tests/test_unit/backends/ibis/test_render_primitives.py`: + +```python +"""Unit tests for the shared sqlglot rendering primitives.""" + +import ibis + +from mountainash_data.backends.ibis._render import ( + dialect_of, + qualified_name, + quote_identifier, + render_type, +) + + +class TestRenderPrimitives: + def test_quote_identifier_duckdb(self): + d = dialect_of(ibis.duckdb.connect()) + assert quote_identifier("new col", d) == '"new col"' + + def test_quote_identifier_mysql_backticks(self): + d = dialect_of(ibis.mysql.connect.__self__) if False else None + # mysql connect needs a server; render via a sqlglot dialect string instead + assert quote_identifier("c", "mysql") == "`c`" + + def test_qualified_name_two_parts(self): + assert qualified_name(["db", "t"], "duckdb") == '"db"."t"' + + def test_render_type_matches_create_table_mapper(self): + con = ibis.duckdb.connect() + tm = con.compiler.type_mapper + assert render_type(tm, ibis.dtype("int64")) == tm.to_string(ibis.dtype("int64")) +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_render_primitives.py -v` +Expected: FAIL — `ModuleNotFoundError: No module named 'mountainash_data.backends.ibis._render'`. + +- [ ] **Step 3: Write minimal implementation** + +Create `src/mountainash_data/backends/ibis/_render.py`: + +```python +"""Shared sqlglot rendering primitives for dialect-agnostic write ops. + +Everything renders off a *live* ibis connection's own compiler, so identifier +quoting and type rendering match what ibis emits for create_table. +""" + +from __future__ import annotations + +import typing as t + +from sqlglot import exp + + +def dialect_of(ibis_conn: t.Any) -> t.Any: + """The live connection's sqlglot dialect (NOT ibis's backend name).""" + return ibis_conn.compiler.dialect + + +def quote_identifier(name: str, dialect: t.Any) -> str: + """Quote a single identifier for `dialect` via sqlglot.""" + return exp.to_identifier(name, quoted=True).sql(dialect=dialect) + + +def qualified_name(parts: list[str], dialect: t.Any) -> str: + """Quote each part and join with '.' (e.g. database.table).""" + return ".".join(quote_identifier(p, dialect) for p in parts) + + +def render_type(type_mapper: t.Any, dtype: t.Any) -> str: + """Render an ibis dtype to SQL via the connection's type-mapper.""" + return type_mapper.to_string(dtype) +``` + +- [ ] **Step 4: Migrate `_generic_add_columns` to the helper (no behaviour change)** + +In `operations.py`, add to the imports near the top: + +```python +from mountainash_data.backends.ibis._render import quote_identifier +``` + +In `_generic_add_columns`, delete the inline `_quote` closure and use the helper. Replace: + +```python + def _quote(identifier: str) -> str: + return exp.to_identifier(identifier, quoted=True).sql(dialect=dialect) + + table_parts = [database, table_name] if database else [table_name] + qualified = ".".join(_quote(part) for part in table_parts) +``` + +with: + +```python + table_parts = [database, table_name] if database else [table_name] + qualified = ".".join(quote_identifier(part, dialect) for part in table_parts) +``` + +and in the loop replace `_quote(col_name)` with `quote_identifier(col_name, dialect)`. + +- [ ] **Step 5: Run tests to verify they pass** + +Run: +```bash +hatch run test:test-target-quick tests/test_unit/backends/ibis/test_render_primitives.py -v +hatch run test:test-target-quick tests/test_unit/backends/ibis/test_add_columns.py -v +``` +Expected: primitives PASS; all `test_add_columns.py` still PASS (no behaviour change). + +- [ ] **Step 6: Lint, types, commit** + +```bash +hatch run ruff:check src +hatch run ruff:check tests/test_unit/backends/ibis/test_render_primitives.py +hatch run mypy:check +git add src/mountainash_data/backends/ibis/_render.py src/mountainash_data/backends/ibis/operations.py tests/test_unit/backends/ibis/test_render_primitives.py +git commit -m "feat(ibis): extract shared _render.py primitives; migrate add_columns" +``` + +--- + +### Task 2: Test infrastructure (Docker services, fixtures, ibis pin) + +**Files:** +- Create: `compose.yaml` (repo root) +- Modify: `tests/fixtures/database_fixtures.py` (live fixtures), `tests/conftest.py` (re-export if needed) +- Modify: `pyproject.toml` (ibis pin), `CLAUDE.md` (stale-note fix), `hatch.toml` (`test-live` script), `.github/workflows/python-run-pytest.yml` (services) +- Test: `tests/test_integration/test_live_smoke.py` (create) + +**Interfaces:** +- Produces: + - `postgres_backend` / `mysql_backend` pytest fixtures yielding a connected `IbisBackend`, skipping locally when unreachable and failing when `MOUNTAINASH_REQUIRE_LIVE_DB=1` and unreachable. + +- [ ] **Step 1: Write `compose.yaml`** + +```yaml +services: + postgres: + image: postgres:18-alpine + environment: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: ibis_testing + healthcheck: + test: ["CMD", "pg_isready", "-U", "postgres"] + interval: 1s + retries: 20 + ports: + - "5432:5432" + mysql: + image: mariadb:12.1.2 + environment: + MYSQL_ALLOW_EMPTY_PASSWORD: "true" + MYSQL_DATABASE: ibis_testing + MYSQL_USER: ibis + MYSQL_PASSWORD: ibis + healthcheck: + test: ["CMD", "mariadb-admin", "ping", "-h", "localhost"] + interval: 1s + retries: 20 + ports: + - "3306:3306" +``` + +- [ ] **Step 2: Write the failing live-smoke test** + +Create `tests/test_integration/test_live_smoke.py`: + +```python +"""Smoke test that the live-db fixtures connect or skip correctly.""" + +import pytest + + +@pytest.mark.integration +def test_postgres_smoke(postgres_backend): + assert isinstance(postgres_backend.list_tables(), list) + + +@pytest.mark.integration +def test_mysql_smoke(mysql_backend): + assert isinstance(mysql_backend.list_tables(), list) +``` + +- [ ] **Step 3: Run to verify it fails** + +Run: `hatch run test:test-target-quick tests/test_integration/test_live_smoke.py -v` +Expected: FAIL — `fixture 'postgres_backend' not found`. + +- [ ] **Step 4: Implement the fixtures** + +Add to `tests/fixtures/database_fixtures.py` (and ensure it is imported by `tests/conftest.py` like the other fixture modules): + +```python +import os + +import pytest + +from mountainash_data import IbisBackend + +_PG = dict( + host=os.environ.get("IBIS_TEST_POSTGRES_HOST", os.environ.get("PGHOST", "localhost")), + port=int(os.environ.get("IBIS_TEST_POSTGRES_PORT", os.environ.get("PGPORT", "5432"))), + user=os.environ.get("IBIS_TEST_POSTGRES_USER", os.environ.get("PGUSER", "postgres")), + password=os.environ.get("IBIS_TEST_POSTGRES_PASSWORD", os.environ.get("PGPASSWORD", "postgres")), + database=os.environ.get("IBIS_TEST_POSTGRES_DATABASE", os.environ.get("PGDATABASE", "ibis_testing")), +) +_MY = dict( + host=os.environ.get("IBIS_TEST_MYSQL_HOST", "localhost"), + port=int(os.environ.get("IBIS_TEST_MYSQL_PORT", "3306")), + user=os.environ.get("IBIS_TEST_MYSQL_USER", "ibis"), + password=os.environ.get("IBIS_TEST_MYSQL_PASSWORD", "ibis"), + database=os.environ.get("IBIS_TEST_MYSQL_DATABASE", "ibis_testing"), +) + + +def _live_or_skip(dialect: str, params: dict): + require = os.environ.get("MOUNTAINASH_REQUIRE_LIVE_DB") == "1" + try: + be = IbisBackend(dialect=dialect, **params) + be.connect() + return be + except Exception as exc: # noqa: BLE001 - service availability gate + msg = f"{dialect} service unreachable: {exc}" + if require: + pytest.fail(msg) + pytest.skip(msg) + + +@pytest.fixture +def postgres_backend(): + be = _live_or_skip("postgres", _PG) + try: + yield be + finally: + be.close() + + +@pytest.fixture +def mysql_backend(): + be = _live_or_skip("mysql", _MY) + try: + yield be + finally: + be.close() +``` + +- [ ] **Step 5: Bump ibis pin + fix CLAUDE.md + add hatch script** + +In `pyproject.toml`, change every `ibis-framework...>=11.0.0` floor to `>=12.0.0` (core dep + each extra). In `CLAUDE.md`, change the `ibis-framework[...] == 10.4.0` line to `ibis-framework[polars,pandas,sqlite,duckdb] >= 12.0.0`. In `hatch.toml` `[envs.test.scripts]`, add: + +```toml +test-live = "docker compose up -d --wait && pytest -m integration {args}" +``` + +- [ ] **Step 6: Update CI workflow** + +In `.github/workflows/python-run-pytest.yml`, add `services:` for postgres (`postgres:18-alpine`) and mariadb (`mariadb:12.1.2`) with the same env/ports as `compose.yaml`, and set `MOUNTAINASH_REQUIRE_LIVE_DB: "1"` in the job `env:`. (Mirror the env-var names the fixtures read.) + +- [ ] **Step 7: Verify (services up locally), then commit** + +```bash +docker compose up -d --wait +hatch run test:test-target-quick tests/test_integration/test_live_smoke.py -v # both PASS +docker compose down +hatch run test:test-target-quick tests/test_integration/test_live_smoke.py -v # both SKIP (no service) +hatch run ruff:check tests/test_integration/test_live_smoke.py +git add compose.yaml tests/fixtures/database_fixtures.py tests/conftest.py tests/test_integration/test_live_smoke.py pyproject.toml CLAUDE.md hatch.toml .github/workflows/python-run-pytest.yml +git commit -m "test(infra): docker postgres+mariadb services, live fixtures, ibis>=12 pin" +``` + +--- + +### Task 3: `UpsertStyle` enum + `upsert_style` field + per-dialect assignment + +**Files:** +- Modify: `src/mountainash_data/backends/ibis/dialects/_registry.py` +- Test: `tests/test_unit/backends/ibis/test_upsert_style_registry.py` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: `UpsertStyle` (`str, enum.Enum`: `ON_CONFLICT="on_conflict"`, `MERGE="merge"`, `ON_DUPLICATE_KEY="on_duplicate_key"`); `DialectSpec.upsert_style: t.Optional[UpsertStyle] = None`. + +This task is **additive** — it assigns styles but does NOT remove the existing `upsert_hook=duckdb_family_upsert` registrations (cutover is Task 9), so existing upsert behaviour stays green. + +- [ ] **Step 1: Write the failing test** + +Create `tests/test_unit/backends/ibis/test_upsert_style_registry.py`: + +```python +"""The upsert_style assignment must match the spec's §7 coverage matrix.""" + +from mountainash_data.backends.ibis.dialects._registry import ( + DIALECTS, + DialectSpec, + UpsertStyle, +) + +# Spec §7 coverage matrix — the single source of truth for this assertion. +EXPECTED_STYLE = { + "sqlite": UpsertStyle.ON_CONFLICT, + "duckdb": UpsertStyle.ON_CONFLICT, + "motherduck": UpsertStyle.ON_CONFLICT, + "postgres": UpsertStyle.ON_CONFLICT, + "risingwave": UpsertStyle.ON_CONFLICT, + "mysql": UpsertStyle.ON_DUPLICATE_KEY, + "singlestoredb": UpsertStyle.ON_DUPLICATE_KEY, + "snowflake": UpsertStyle.MERGE, + "bigquery": UpsertStyle.MERGE, + "mssql": UpsertStyle.MERGE, + "oracle": UpsertStyle.MERGE, + "databricks": UpsertStyle.MERGE, + "exasol": UpsertStyle.MERGE, + "trino": UpsertStyle.MERGE, + "redshift": UpsertStyle.MERGE, + "clickhouse": None, + "impala": None, + "materialize": None, + "druid": None, + "pyspark": None, +} + + +class TestUpsertStyleField: + def test_field_defaults_none(self): + spec = DialectSpec( + ibis_backend_name="duckdb", + connection_mode="connection_string", + connection_string_scheme="duckdb://", + ) + assert spec.upsert_style is None + + def test_every_registry_dialect_has_an_explicit_decision(self): + # Iterates the live registry — a new dialect with no matrix entry fails. + assert set(DIALECTS) == set(EXPECTED_STYLE), ( + "registry dialects and the §7 matrix have diverged" + ) + + def test_assigned_styles_match_matrix(self): + for name, expected in EXPECTED_STYLE.items(): + assert DIALECTS[name].upsert_style == expected, name +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_style_registry.py -v` +Expected: FAIL — `ImportError: cannot import name 'UpsertStyle'`. + +- [ ] **Step 3: Add the enum + field** + +In `_registry.py`, after the imports add: + +```python +import enum + + +class UpsertStyle(str, enum.Enum): + ON_CONFLICT = "on_conflict" + MERGE = "merge" + ON_DUPLICATE_KEY = "on_duplicate_key" +``` + +Add the field to `DialectSpec` after `upsert_hook`: + +```python + upsert_hook: t.Optional[UpsertHook] = None + upsert_style: t.Optional[UpsertStyle] = None +``` + +- [ ] **Step 4: Assign `upsert_style` to each dialect** + +Add `upsert_style=UpsertStyle.` to each `DialectSpec(...)` entry per `EXPECTED_STYLE` above. Leave the `None` dialects without the field (defaults to `None`). Leave the three existing `upsert_hook=duckdb_family_upsert` lines in place for now. + +- [ ] **Step 5: Run test to verify it passes** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_style_registry.py -v` +Expected: PASS (3 tests). + +- [ ] **Step 6: Lint, types, commit** + +```bash +hatch run ruff:check src +hatch run ruff:check tests/test_unit/backends/ibis/test_upsert_style_registry.py +hatch run mypy:check +git add src/mountainash_data/backends/ibis/dialects/_registry.py tests/test_unit/backends/ibis/test_upsert_style_registry.py +git commit -m "feat(ibis): add UpsertStyle enum + upsert_style field; assign per matrix" +``` + +--- + +### Task 4: `_generic_rename_table` + dispatch + +**Files:** +- Modify: `src/mountainash_data/backends/ibis/operations.py` (add `_generic_rename_table`), `src/mountainash_data/backends/ibis/backend.py` (dispatch) +- Test: `tests/test_unit/backends/ibis/test_rename_table_render.py` (create), `tests/test_integration/test_write_ops_live.py` (create, rename portion) + +**Interfaces:** +- Consumes: `dialect_of`, `quote_identifier` (Task 1); `_validate_simple_identifier` (existing). +- Produces: `_generic_rename_table(ibis_conn, old_name: str, new_name: str) -> None`. + +- [ ] **Step 1: Write the failing tests** + +Create `tests/test_unit/backends/ibis/test_rename_table_render.py`: + +```python +"""rename_table works via the sqlglot generic default on every dialect.""" + +import ibis +import polars as pl +import pytest + +from mountainash_data import IbisBackend +from mountainash_data.backends.ibis.operations import _generic_rename_table + + +class TestGenericRenameTable: + def test_renames_on_duckdb(self): + con = ibis.duckdb.connect() + con.create_table("old", pl.DataFrame({"id": [1]})) + _generic_rename_table(con, "old", "new") + names = con.list_tables() + assert "new" in names and "old" not in names + + def test_renames_on_sqlite(self): + con = ibis.sqlite.connect() + con.create_table("old", pl.DataFrame({"id": [1]})) + _generic_rename_table(con, "old", "new") + assert "new" in con.list_tables() + + def test_rejects_dotted_names(self): + con = ibis.duckdb.connect() + con.create_table("old", pl.DataFrame({"id": [1]})) + with pytest.raises(ValueError, match="simple"): + _generic_rename_table(con, "a.old", "new") + + def test_backend_rename_table_returns_self(self): + with IbisBackend(dialect="duckdb", database=":memory:") as be: + be.create_table("old", pl.DataFrame({"id": [1]})) + assert be.rename_table("old", "new") is be + assert "new" in be.list_tables() +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_rename_table_render.py -v` +Expected: FAIL — `ImportError: cannot import name '_generic_rename_table'`. + +- [ ] **Step 3: Implement `_generic_rename_table`** + +In `operations.py`, add (importing the helpers and `exp` at top if not already present: `from sqlglot import exp` is already imported for add_columns; add `from mountainash_data.backends.ibis._render import dialect_of, quote_identifier`): + +```python +def _generic_rename_table(ibis_conn: t.Any, old_name: str, new_name: str) -> None: + """Rename a table via a sqlglot-rendered ALTER, portable across dialects. + + sqlglot renders `ALTER TABLE … RENAME TO …` for most dialects, `EXEC + sp_rename …` for SQL Server, and `ALTER TABLE … RENAME …` for MySQL. + """ + _validate_simple_identifier(old_name, kind="old_name") + _validate_simple_identifier(new_name, kind="new_name") + dialect = dialect_of(ibis_conn) + stmt = exp.Alter( + this=exp.to_table(quote_identifier(old_name, dialect)), + kind="TABLE", + actions=[exp.AlterRename(this=exp.to_identifier(new_name, quoted=True))], + ).sql(dialect=dialect) + ibis_conn.raw_sql(stmt) +``` + +> If `exp.Alter`/`exp.AlterRename` are not the exact class names in the installed sqlglot 30.x, use the verified-equivalent transpile fallback: `sqlglot.transpile(f'ALTER TABLE {quote_identifier(old_name, "")} RENAME TO {quote_identifier(new_name, "")}', read="duckdb", write=dialect)[0]`. Confirm via a one-line probe in the test env before settling. + +- [ ] **Step 4: Wire dispatch in `backend.py`** + +Replace the body of `rename_table` (currently raises when `rename_table_hook is None`) with hook-or-generic. Add the import near the other operations imports (`from mountainash_data.backends.ibis.operations import _generic_rename_table`) and: + +```python + def rename_table(self, old_name: str, new_name: str) -> IbisBackend: + conn = self._require_connected() + hook = self._spec.rename_table_hook + if hook is not None: + hook(conn._ibis_conn, old_name, new_name) + else: + _generic_rename_table(conn._ibis_conn, old_name, new_name) + return self +``` + +- [ ] **Step 5: Add the live rename round-trip** + +Create `tests/test_integration/test_write_ops_live.py` with the rename portion: + +```python +"""Live round-trip tests for generic write ops (postgres + mysql).""" + +import polars as pl +import pytest + + +@pytest.mark.integration +def test_rename_table_live_postgres(postgres_backend): + be = postgres_backend + be.create_table("ren_old", pl.DataFrame({"id": [1]}), overwrite=True) + be.rename_table("ren_old", "ren_new") + assert "ren_new" in be.list_tables() + be.drop_table("ren_new", force=True) + + +@pytest.mark.integration +def test_rename_table_live_mysql(mysql_backend): + be = mysql_backend + be.create_table("ren_old", pl.DataFrame({"id": [1]}), overwrite=True) + be.rename_table("ren_old", "ren_new") + assert "ren_new" in be.list_tables() + be.drop_table("ren_new", force=True) +``` + +- [ ] **Step 6: Run, lint, types, commit** + +```bash +hatch run test:test-target-quick tests/test_unit/backends/ibis/test_rename_table_render.py -v +docker compose up -d --wait && hatch run test:test-target-quick tests/test_integration/test_write_ops_live.py -v ; docker compose down +hatch run ruff:check src +hatch run ruff:check tests/test_unit/backends/ibis/test_rename_table_render.py tests/test_integration/test_write_ops_live.py +hatch run mypy:check +git add src/mountainash_data/backends/ibis/operations.py src/mountainash_data/backends/ibis/backend.py tests/test_unit/backends/ibis/test_rename_table_render.py tests/test_integration/test_write_ops_live.py +git commit -m "feat(ibis): generic sqlglot rename_table (works on every dialect)" +``` + +--- + +### Task 5: Conditional-predicate compiler (`_render.py`) + +**Files:** +- Modify: `src/mountainash_data/backends/ibis/_render.py` +- Test: `tests/test_unit/backends/ibis/test_upsert_condition_render.py` (create) + +**Interfaces:** +- Consumes: nothing from later tasks. +- Produces: + - `INCOMING_SENTINEL = "__ma_incoming__"`, `EXISTING_SENTINEL = "__ma_existing__"`. + - `validate_predicate(expr: ir.BooleanValue) -> None` — raises `ValueError` if the predicate contains an aggregation, window, or subquery/EXISTS op. + - `compile_condition(ibis_conn, target_schema, predicate, *, incoming_alias, existing_alias) -> exp.Expression` — returns the remapped `ON` sub-AST with incoming columns → `incoming_alias`, existing columns → `existing_alias`. Raises `ValueError` if the target schema would collide with a sentinel name (caller passes the real target name to check) or the grammar is violated. + +The mechanism is the §6.1/§9 probe path: bind two sentinel-named ibis tables, `existing.join(incoming, predicate)`, `compiler.to_sqlglot`, extract the join `ON`, remap aliases keyed by sentinel. + +- [ ] **Step 1: Write the failing tests** + +Create `tests/test_unit/backends/ibis/test_upsert_condition_render.py`: + +```python +"""The update_condition ibis-expression predicate compiler (§6.1).""" + +import ibis +import pytest + +from mountainash_data.backends.ibis._render import ( + compile_condition, + dialect_of, + validate_predicate, +) + +_SCHEMA = ibis.schema({"id": "int64", "updated_at": "timestamp", "v": "string"}) + + +def _render(con, predicate, *, incoming_alias, existing_alias): + ast = compile_condition( + con, _SCHEMA, predicate, + incoming_alias=incoming_alias, existing_alias=existing_alias, + ) + return ast.sql(dialect=dialect_of(con)) + + +class TestCompileCondition: + def test_on_conflict_alias_mapping_duckdb(self): + con = ibis.duckdb.connect() + sql = _render( + con, + lambda inc, exi: inc.updated_at > exi.updated_at, + incoming_alias="EXCLUDED", existing_alias="tgt", + ) + assert '"EXCLUDED"."updated_at"' in sql + assert '"tgt"."updated_at"' in sql + + def test_merge_alias_mapping_duckdb(self): + con = ibis.duckdb.connect() + sql = _render( + con, + lambda inc, exi: inc.updated_at > exi.updated_at, + incoming_alias="src", existing_alias="tgt", + ) + assert '"src"."updated_at"' in sql and '"tgt"."updated_at"' in sql + + def test_function_predicate_renders_per_dialect(self): + con = ibis.duckdb.connect() + sql = _render( + con, + lambda inc, exi: inc.v.upper() != exi.v.upper(), + incoming_alias="src", existing_alias="tgt", + ) + assert "UPPER(" in sql.upper() + + def test_rejects_aggregate_predicate(self): + with pytest.raises(ValueError, match="aggregat|window|subquer"): + validate_predicate( + ibis.table(_SCHEMA, name="x").v.count() > 0 # aggregation + ) +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_condition_render.py -v` +Expected: FAIL — `ImportError: cannot import name 'compile_condition'`. + +- [ ] **Step 3: Implement the compiler** + +Append to `_render.py`: + +```python +import ibis # noqa: E402 (kept with the other third-party imports at top in practice) +import ibis.expr.operations as ops +import ibis.expr.types as ir + +INCOMING_SENTINEL = "__ma_incoming__" +EXISTING_SENTINEL = "__ma_existing__" + +# ops whose presence makes a predicate invalid in a WHERE / WHEN MATCHED splice +_FORBIDDEN_OPS = (ops.Reduction, ops.WindowFunction) + + +def validate_predicate(expr: ir.BooleanValue) -> None: + """Reject predicates that cannot live in a row-level WHERE/WHEN MATCHED.""" + node = expr.op() + for n in node.find(_FORBIDDEN_OPS): # type: ignore[arg-type] + raise ValueError( + "update_condition must be a scalar row predicate; found " + f"{type(n).__name__} (aggregation/window). Use the upsert_hook " + "override for conditions outside this grammar." + ) + # subqueries / EXISTS surface as relational ops embedded in the predicate + for n in node.find((ops.Relation,)): # type: ignore[arg-type] + raise ValueError( + "update_condition may not contain subqueries/EXISTS/third-table " + "references; use the upsert_hook override." + ) + + +def compile_condition( + ibis_conn: t.Any, + target_schema: t.Any, + predicate: t.Callable[[ir.Table, ir.Table], ir.BooleanValue], + *, + incoming_alias: str, + existing_alias: str, +) -> exp.Expression: + """Render an (incoming, existing) -> bool predicate to a sqlglot ON sub-AST, + with incoming columns aliased to `incoming_alias` and existing to + `existing_alias`. See spec §6.1.""" + incoming = ibis.table(target_schema, name=INCOMING_SENTINEL) + existing = ibis.table(target_schema, name=EXISTING_SENTINEL) + pred = predicate(incoming, existing) + validate_predicate(pred) + + joined = existing.join(incoming, pred, how="inner") + ast = ibis_conn.compiler.to_sqlglot(joined) + ast = ast if isinstance(ast, exp.Expression) else ast[0] + + alias_to_side = {} + for tbl in ast.find_all(exp.Table): + if tbl.name == INCOMING_SENTINEL: + alias_to_side[tbl.alias_or_name] = incoming_alias + elif tbl.name == EXISTING_SENTINEL: + alias_to_side[tbl.alias_or_name] = existing_alias + + join = next(ast.find_all(exp.Join), None) + if join is None or join.args.get("on") is None: + raise ValueError("could not extract join ON predicate") + on = join.args["on"].copy() + + def _remap(n: exp.Expression) -> exp.Expression: + if isinstance(n, exp.Column) and n.table in alias_to_side: + n.set("table", exp.to_identifier(alias_to_side[n.table], quoted=True)) + return n + + return on.transform(_remap) +``` + +> Pin the exact ibis op classes (`ops.Reduction`, `ops.WindowFunction`, `ops.Relation`) against the installed ibis 12 during this step — run a quick probe to confirm `expr.op().find((ops.Reduction,))` exists and behaves as used; adjust the forbidden-op tuple if the names differ. The behaviour contract (reject aggregate/window/subquery) is fixed by the tests. + +- [ ] **Step 4: Run to verify it passes** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_condition_render.py -v` +Expected: PASS (4 tests). + +- [ ] **Step 5: Lint, types, commit** + +```bash +hatch run ruff:check src +hatch run ruff:check tests/test_unit/backends/ibis/test_upsert_condition_render.py +hatch run mypy:check +git add src/mountainash_data/backends/ibis/_render.py tests/test_unit/backends/ibis/test_upsert_condition_render.py +git commit -m "feat(ibis): conditional-predicate compiler (sentinel join->AST->remap)" +``` + +--- + +### Task 6: `_render_on_conflict` + `_generic_upsert` (ON_CONFLICT branch) + +**Files:** +- Modify: `src/mountainash_data/backends/ibis/_render.py` (add `compiled_source`), `src/mountainash_data/backends/ibis/operations.py` (add `_generic_upsert`, `_render_on_conflict`, helpers) +- Test: `tests/test_unit/backends/ibis/test_upsert_render.py` (create) + +**Interfaces:** +- Consumes: `quote_identifier`, `qualified_name`, `compile_condition`, `validate_predicate` (Tasks 1, 5); `_normalize_columns`, `_validate_simple_identifier` (existing). +- Produces: + - `compiled_source(ibis_conn, obj, target_schema) -> tuple[str, list[str]]` — `(subquery_sql, source_columns)`; columns cast to the target type and projected in target order. + - `_generic_upsert(ibis_conn, name, obj, *, style, conflict_columns, update_columns, conflict_action, update_condition, database, schema) -> None`. + - `_render_on_conflict(...) -> str`. + +This task wires `_generic_upsert` and the ON_CONFLICT branch only; MERGE/ON_DUPLICATE raise `NotImplementedError("unimplemented style")` as placeholders until Tasks 7/8. Dispatch is NOT flipped yet (Task 9), so this is exercised directly against a raw connection. + +- [ ] **Step 1: Write the failing tests** + +Create `tests/test_unit/backends/ibis/test_upsert_render.py`: + +```python +"""Generic upsert — ON CONFLICT family (sqlite/duckdb).""" + +import ibis +import polars as pl +import pytest + +from mountainash_data.backends.ibis.dialects._registry import UpsertStyle +from mountainash_data.backends.ibis.operations import _generic_upsert + + +def _seed(con): + con.create_table("t", pl.DataFrame({"id": [1, 2], "v": ["a", "b"]})) + + +class TestOnConflictUpsert: + def test_insert_and_update_duckdb(self): + con = ibis.duckdb.connect() + _seed(con) + _generic_upsert( + con, "t", pl.DataFrame({"id": [2, 3], "v": ["B", "c"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns=["id"], + update_columns=None, conflict_action="UPDATE", + update_condition=None, database=None, schema=None, + ) + rows = dict(con.table("t").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "a", 2: "B", 3: "c"} + + def test_do_nothing_duckdb(self): + con = ibis.duckdb.connect() + _seed(con) + _generic_upsert( + con, "t", pl.DataFrame({"id": [2], "v": ["X"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns="id", + update_columns=None, conflict_action="NOTHING", + update_condition=None, database=None, schema=None, + ) + assert con.table("t").filter(ibis._.id == 2).execute()["v"].iloc[0] == "b" + + def test_composite_key_sqlite(self): + con = ibis.sqlite.connect() + con.create_table("t", pl.DataFrame({"a": [1], "b": [1], "v": ["x"]})) + # needs a composite unique index for ON CONFLICT to detect + con.raw_sql("CREATE UNIQUE INDEX ux ON t (a, b)") + _generic_upsert( + con, "t", pl.DataFrame({"a": [1], "b": [1], "v": ["y"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns=["a", "b"], + update_columns=None, conflict_action="UPDATE", + update_condition=None, database=None, schema=None, + ) + assert con.table("t").execute()["v"].iloc[0] == "y" + + def test_conditional_update_only_when_newer_duckdb(self): + con = ibis.duckdb.connect() + con.create_table("t", pl.DataFrame({"id": [1], "ver": [5], "v": ["old"]})) + con.raw_sql("CREATE UNIQUE INDEX ux ON t (id)") + _generic_upsert( + con, "t", pl.DataFrame({"id": [1], "ver": [3], "v": ["stale"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns=["id"], + update_columns=None, conflict_action="UPDATE", + update_condition=lambda inc, exi: inc.ver > exi.ver, + database=None, schema=None, + ) + # incoming ver(3) is NOT newer than existing(5) -> unchanged + assert con.table("t").execute()["v"].iloc[0] == "old" + + def test_unknown_style_raises_notimplemented(self): + con = ibis.duckdb.connect() + _seed(con) + with pytest.raises(NotImplementedError): + _generic_upsert( + con, "t", pl.DataFrame({"id": [9], "v": ["z"]}), + style=None, conflict_columns=["id"], update_columns=None, + conflict_action="UPDATE", update_condition=None, + database=None, schema=None, + ) +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_render.py -v` +Expected: FAIL — `ImportError: cannot import name '_generic_upsert'`. + +- [ ] **Step 3: Implement `compiled_source` in `_render.py`** + +```python +def compiled_source( + ibis_conn: t.Any, obj: t.Any, target_schema: t.Any +) -> tuple[str, list[str]]: + """Compile `obj` to a SELECT subquery, casting each column to the target + type and projecting in target-column order. Returns (sql, columns). + + Columns present in the target but absent from the source are omitted; + columns present in the source but absent from the target raise ValueError. + """ + src = obj if isinstance(obj, ir.Table) else ibis.memtable(obj) + src_cols = set(src.columns) + extra = src_cols - set(target_schema.names) + if extra: + raise ValueError(f"source columns absent from target: {sorted(extra)}") + cols = [c for c in target_schema.names if c in src_cols] + projected = src.select( + [src[c].cast(target_schema[c]).name(c) for c in cols] + ) + return ibis_conn.compile(projected), cols +``` + +- [ ] **Step 4: Implement `_generic_upsert` + `_render_on_conflict` in `operations.py`** + +Add imports at top: `from mountainash_data.backends.ibis._render import (compile_condition, compiled_source, qualified_name, quote_identifier, dialect_of)` and `from mountainash_data.backends.ibis.dialects._registry import UpsertStyle`. Then: + +```python +def _generic_upsert( + ibis_conn: t.Any, + name: str, + obj: t.Any, + *, + style: t.Any, + conflict_columns: t.Any, + update_columns: t.Any, + conflict_action: str, + update_condition: t.Any, + database: str | None, + schema: str | None, +) -> None: + if style is None: + raise NotImplementedError( + f"Dialect (connection {type(ibis_conn).__name__}) does not support upsert" + ) + _validate_simple_identifier(name, kind="name") + if database is not None: + _validate_simple_identifier(database, kind="database") + if conflict_action not in ("UPDATE", "NOTHING"): + raise ValueError(f"conflict_action must be UPDATE or NOTHING, got {conflict_action!r}") + + target_schema = ibis_conn.table(name, database=database).schema() + conflict = _normalize_columns(conflict_columns) + if update_columns is None: + update = [c for c in target_schema.names if c not in conflict] + else: + update = _normalize_columns(update_columns) + if conflict_action == "UPDATE" and not update: + raise ValueError("no columns to update; provide update_columns or non-key columns") + + if style is UpsertStyle.ON_CONFLICT: + stmt = _render_on_conflict( + ibis_conn, name, obj, target_schema=target_schema, conflict=conflict, + update=update, conflict_action=conflict_action, + update_condition=update_condition, database=database, schema=schema, + ) + elif style is UpsertStyle.MERGE: + raise NotImplementedError("unimplemented style: MERGE") # Task 7 + elif style is UpsertStyle.ON_DUPLICATE_KEY: + raise NotImplementedError("unimplemented style: ON_DUPLICATE_KEY") # Task 8 + else: + raise NotImplementedError(f"unknown upsert_style: {style!r}") + ibis_conn.raw_sql(stmt) + + +def _render_on_conflict( + ibis_conn, name, obj, *, target_schema, conflict, update, + conflict_action, update_condition, database, schema, +) -> str: + dialect = dialect_of(ibis_conn) + source_sql, cols = compiled_source(ibis_conn, obj, target_schema) + parts = [p for p in (database, schema, name) if p] + target = qualified_name(parts, dialect) + col_list = ", ".join(quote_identifier(c, dialect) for c in cols) + conflict_list = ", ".join(quote_identifier(c, dialect) for c in conflict) + + if conflict_action == "NOTHING": + action = f"ON CONFLICT ({conflict_list}) DO NOTHING" + else: + set_sql = ", ".join( + f"{quote_identifier(c, dialect)} = " + f'{quote_identifier("EXCLUDED", dialect)}.{quote_identifier(c, dialect)}' + for c in update + ) + where = "" + if update_condition is not None: + cond = compile_condition( + ibis_conn, target_schema, update_condition, + incoming_alias="EXCLUDED", existing_alias=name, + ).sql(dialect=dialect) + where = f" WHERE {cond}" + action = f"ON CONFLICT ({conflict_list}) DO UPDATE SET {set_sql}{where}" + + return f"INSERT INTO {target} ({col_list}) SELECT {col_list} FROM ({source_sql}) AS __src {action}" +``` + +> Note: when `update_condition` is supplied, the existing row is referenced by the bare table `name` (the default ON CONFLICT convention); if a future dialect needs `INSERT INTO t AS tgt` aliasing (spec §7), thread an alias through here. For the live-tested dialects (duckdb/sqlite/postgres) the bare-name form is valid. + +- [ ] **Step 5: Run to verify it passes** + +Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_render.py -v` +Expected: PASS (5 tests). If a backend rejects the subquery cast or the `__src` alias, adjust `compiled_source`/alias to the form the live engine accepts (the tests are the contract). + +- [ ] **Step 6: Lint, types, commit** + +```bash +hatch run ruff:check src +hatch run ruff:check tests/test_unit/backends/ibis/test_upsert_render.py +hatch run mypy:check +git add src/mountainash_data/backends/ibis/_render.py src/mountainash_data/backends/ibis/operations.py tests/test_unit/backends/ibis/test_upsert_render.py +git commit -m "feat(ibis): generic upsert ON CONFLICT branch + compiled-subquery staging" +``` + +--- + +### Task 7: `_render_merge` (MERGE family) + +**Files:** +- Modify: `src/mountainash_data/backends/ibis/operations.py` +- Test: `tests/test_unit/backends/ibis/test_upsert_render.py` (append) + +**Interfaces:** +- Consumes: `compiled_source`, `compile_condition`, `qualified_name`, `quote_identifier` (Tasks 1/5/6). +- Produces: `_render_merge(...) -> str`; wired into the `_generic_upsert` MERGE branch (replacing the placeholder). + +Model on Ibis 12 `SQLBackend._build_upsert_from_table` (`sge.merge`), extended to composite `on` and `conflict_action`. duckdb supports MERGE, so this is live-testable in-memory. + +- [ ] **Step 1: Write the failing tests** (append to `test_upsert_render.py`) + +```python +class TestMergeUpsert: + def test_merge_insert_and_update_duckdb(self): + con = ibis.duckdb.connect() + con.create_table("m", pl.DataFrame({"id": [1, 2], "v": ["a", "b"]})) + from mountainash_data.backends.ibis.operations import _generic_upsert as gu + gu( + con, "m", pl.DataFrame({"id": [2, 3], "v": ["B", "c"]}), + style=UpsertStyle.MERGE, conflict_columns=["id"], update_columns=None, + conflict_action="UPDATE", update_condition=None, database=None, schema=None, + ) + rows = dict(con.table("m").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "a", 2: "B", 3: "c"} + + def test_merge_nothing_omits_matched_duckdb(self): + con = ibis.duckdb.connect() + con.create_table("m", pl.DataFrame({"id": [1], "v": ["a"]})) + from mountainash_data.backends.ibis.operations import _generic_upsert as gu + gu( + con, "m", pl.DataFrame({"id": [1, 2], "v": ["X", "b"]}), + style=UpsertStyle.MERGE, conflict_columns=["id"], update_columns=None, + conflict_action="NOTHING", update_condition=None, database=None, schema=None, + ) + rows = dict(con.table("m").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "a", 2: "b"} # id=1 NOT updated, id=2 inserted +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `hatch run test:test-target-quick "tests/test_unit/backends/ibis/test_upsert_render.py::TestMergeUpsert" -v` +Expected: FAIL — `NotImplementedError: unimplemented style: MERGE`. + +- [ ] **Step 3: Implement `_render_merge`** (replace the MERGE-branch placeholder with `stmt = _render_merge(...)` and add): + +```python +def _render_merge( + ibis_conn, name, obj, *, target_schema, conflict, update, + conflict_action, update_condition, database, schema, +) -> str: + dialect = dialect_of(ibis_conn) + source_sql, cols = compiled_source(ibis_conn, obj, target_schema) + parts = [p for p in (database, schema, name) if p] + target = qualified_name(parts, dialect) + q = lambda c: quote_identifier(c, dialect) # noqa: E731 + + on = " AND ".join(f"tgt.{q(c)} = src.{q(c)}" for c in conflict) + not_matched = ( + f"WHEN NOT MATCHED THEN INSERT ({', '.join(q(c) for c in cols)}) " + f"VALUES ({', '.join(f'src.{q(c)}' for c in cols)})" + ) + clauses = [] + if conflict_action == "UPDATE": + set_sql = ", ".join(f"{q(c)} = src.{q(c)}" for c in update) + cond = "" + if update_condition is not None: + cond = " AND " + compile_condition( + ibis_conn, target_schema, update_condition, + incoming_alias="src", existing_alias="tgt", + ).sql(dialect=dialect) + clauses.append(f"WHEN MATCHED{cond} THEN UPDATE SET {set_sql}") + clauses.append(not_matched) + + return ( + f"MERGE INTO {target} AS tgt USING ({source_sql}) AS src " + f"ON {on} " + " ".join(clauses) + ) +``` + +- [ ] **Step 4: Run to verify it passes** + +Run: `hatch run test:test-target-quick "tests/test_unit/backends/ibis/test_upsert_render.py::TestMergeUpsert" -v` +Expected: PASS (2 tests). If duckdb's MERGE grammar rejects a clause ordering, reorder to its accepted form (the data-outcome assertions are the contract). + +- [ ] **Step 5: Add golden-SQL assertions for a warehouse dialect** (append): + +```python +class TestMergeGoldenSQL: + def test_snowflake_merge_shape(self): + # render-only: assert the emitted MERGE string shape for snowflake + from mountainash_data.backends.ibis.operations import _render_merge + con = ibis.duckdb.connect() + con.create_table("m", pl.DataFrame({"id": [1], "v": ["a"]})) + # compile against duckdb conn but assert structural tokens dialect-agnostically + sql = _render_merge( + con, "m", pl.DataFrame({"id": [1], "v": ["a"]}), + target_schema=con.table("m").schema(), conflict=["id"], update=["v"], + conflict_action="UPDATE", update_condition=None, database=None, schema=None, + ) + assert sql.startswith("MERGE INTO") + assert "WHEN MATCHED THEN UPDATE SET" in sql + assert "WHEN NOT MATCHED THEN INSERT" in sql +``` + +- [ ] **Step 6: Run, lint, types, commit** + +```bash +hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_render.py -v +hatch run ruff:check src tests/test_unit/backends/ibis/test_upsert_render.py +hatch run mypy:check +git add src/mountainash_data/backends/ibis/operations.py tests/test_unit/backends/ibis/test_upsert_render.py +git commit -m "feat(ibis): generic upsert MERGE branch (composite keys + conflict_action)" +``` + +--- + +### Task 8: `_render_on_duplicate_key` + MySQL preflight introspection + +**Files:** +- Modify: `src/mountainash_data/backends/ibis/operations.py` +- Test: `tests/test_integration/test_upsert_mysql_preflight.py` (create), `tests/test_unit/backends/ibis/test_upsert_render.py` (golden append) + +**Interfaces:** +- Consumes: `compiled_source`, `qualified_name`, `quote_identifier` (Tasks 1/6). +- Produces: `_render_on_duplicate_key(...) -> str`; `_mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None` (prove-safe-or-raise preflight, §6.2); wired into the `_generic_upsert` ON_DUPLICATE_KEY branch. + +- [ ] **Step 1: Write the failing live tests** + +Create `tests/test_integration/test_upsert_mysql_preflight.py`: + +```python +"""MySQL ON DUPLICATE KEY preflight: prove-safe-or-raise (spec §6.2).""" + +import polars as pl +import pytest + + +@pytest.mark.integration +def test_single_pk_proceeds(mysql_backend): + be = mysql_backend + con = be._connection._ibis_conn # raw ibis conn + con.raw_sql("DROP TABLE IF EXISTS odk_ok") + con.raw_sql("CREATE TABLE odk_ok (id INT PRIMARY KEY, v VARCHAR(16) NOT NULL)") + con.raw_sql("INSERT INTO odk_ok VALUES (1, 'a')") + be.upsert("odk_ok", pl.DataFrame({"id": [1, 2], "v": ["A", "b"]}), conflict_columns=["id"]) + rows = dict(con.table("odk_ok").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "A", 2: "b"} + con.raw_sql("DROP TABLE odk_ok") + + +@pytest.mark.integration +def test_multiple_unique_raises(mysql_backend): + be = mysql_backend + con = be._connection._ibis_conn + con.raw_sql("DROP TABLE IF EXISTS odk_multi") + con.raw_sql( + "CREATE TABLE odk_multi " + "(id INT PRIMARY KEY, email VARCHAR(64) NOT NULL UNIQUE, v VARCHAR(16) NOT NULL)" + ) + with pytest.raises(ValueError, match="unique"): + be.upsert("odk_multi", pl.DataFrame({"id": [1], "email": ["x"], "v": ["a"]}), conflict_columns=["id"]) + con.raw_sql("DROP TABLE odk_multi") + + +@pytest.mark.integration +def test_nullable_conflict_column_raises(mysql_backend): + be = mysql_backend + con = be._connection._ibis_conn + con.raw_sql("DROP TABLE IF EXISTS odk_null") + con.raw_sql("CREATE TABLE odk_null (k INT NULL UNIQUE, v VARCHAR(16) NOT NULL)") + with pytest.raises(ValueError, match="nullable|NOT NULL"): + be.upsert("odk_null", pl.DataFrame({"k": [1], "v": ["a"]}), conflict_columns=["k"]) + con.raw_sql("DROP TABLE odk_null") +``` + +(The exact `be._connection._ibis_conn` accessor: confirm the attribute path against `backend.py` — use whatever the existing code exposes for the raw connection. If `upsert` already runs through dispatch by this task, calling `be.upsert(...)` is enough and the raw-conn lines are only for setup DDL.) + +- [ ] **Step 2: Run to verify it fails** (services up) + +Run: `docker compose up -d --wait && hatch run test:test-target-quick tests/test_integration/test_upsert_mysql_preflight.py -v` +Expected: FAIL — `NotImplementedError: unimplemented style: ON_DUPLICATE_KEY` (or dispatch not yet wired → see Task 9 ordering note). + +- [ ] **Step 3: Implement preflight + renderer** + +```python +def _mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None: + """Prove the safe MySQL ON DUPLICATE KEY case or raise (spec §6.2).""" + db = database or ibis_conn.current_database + rows = ibis_conn.raw_sql( + "SELECT INDEX_NAME, COLUMN_NAME, SUB_PART, NON_UNIQUE " + "FROM information_schema.STATISTICS " + f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}'" + ).fetchall() + uniques: dict[str, list] = {} + for index_name, column_name, sub_part, non_unique in rows: + if int(non_unique) == 0: + uniques.setdefault(index_name, []).append((column_name, sub_part)) + if not uniques: + raise ValueError(f"table {name!r} has no unique/PK index for conflict_columns") + matching = [ + idx for idx, cols in uniques.items() + if [c for c, _ in cols] == list(conflict) and all(sp is None for _, sp in cols) + ] + if len(uniques) > 1: + raise ValueError( + f"table {name!r} has multiple unique indexes {list(uniques)}; MySQL " + f"ON DUPLICATE KEY detects on any of them — ambiguous for conflict_columns=" + f"{conflict}. Use the upsert_hook override." + ) + if not matching: + raise ValueError( + f"no non-prefix unique index exactly matches conflict_columns={conflict} " + f"(found {uniques}); refusing to guess. Use the upsert_hook override." + ) + # nullable check + cols_meta = ibis_conn.raw_sql( + "SELECT COLUMN_NAME, IS_NULLABLE FROM information_schema.COLUMNS " + f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}'" + ).fetchall() + nullable = {c for c, isn in cols_meta if isn == "YES"} + bad = [c for c in conflict if c in nullable] + if bad: + raise ValueError( + f"conflict columns {bad} are nullable; MySQL unique indexes are " + f"NULL-distinct, so duplicates would insert. Make them NOT NULL or " + f"use the upsert_hook override." + ) + + +def _render_on_duplicate_key( + ibis_conn, name, obj, *, target_schema, conflict, update, + conflict_action, update_condition, database, schema, +) -> str: + _mysql_validate_conflict_key(ibis_conn, name, conflict, database) + dialect = dialect_of(ibis_conn) + source_sql, cols = compiled_source(ibis_conn, obj, target_schema) + parts = [p for p in (database, schema, name) if p] + target = qualified_name(parts, dialect) + q = lambda c: quote_identifier(c, dialect) # noqa: E731 + col_list = ", ".join(q(c) for c in cols) + + if conflict_action == "NOTHING": + k0 = q(conflict[0]) + set_sql = f"{k0} = {k0}" # self-assign; see §6.2 (not a true no-op) + else: + set_sql = ", ".join(f"{q(c)} = VALUES({q(c)})" for c in update) + + return ( + f"INSERT INTO {target} ({col_list}) SELECT {col_list} FROM ({source_sql}) AS __src " + f"ON DUPLICATE KEY UPDATE {set_sql}" + ) +``` + +Wire the ON_DUPLICATE_KEY branch in `_generic_upsert` to `stmt = _render_on_duplicate_key(...)`. Also enforce step-5 precedence (§10): when `style is ON_DUPLICATE_KEY and update_condition is not None`, raise `ValueError("update_condition is not supported for the MySQL ON DUPLICATE KEY family")` — place this check in `_generic_upsert` BEFORE the branch dispatch. + +- [ ] **Step 4: Run to verify it passes** (services up) + +Run: `hatch run test:test-target-quick tests/test_integration/test_upsert_mysql_preflight.py -v` +Expected: 3 PASS (after Task 9 wires dispatch; if running before Task 9, call `_generic_upsert` directly as in Task 6's tests). + +- [ ] **Step 5: Add golden-SQL for on_duplicate_key** (append to `test_upsert_render.py`, render-only, no live MySQL): assert `_render_on_duplicate_key`-style output contains `ON DUPLICATE KEY UPDATE` and `VALUES(` — but since it calls the preflight, test the pure render by factoring the SQL-string builder out of the preflight, or mark this assertion as covered by the live preflight test. Keep the unit layer to the string shape only. + +- [ ] **Step 6: Lint, types, commit** + +```bash +hatch run ruff:check src tests/test_integration/test_upsert_mysql_preflight.py +hatch run mypy:check +git add src/mountainash_data/backends/ibis/operations.py tests/test_integration/test_upsert_mysql_preflight.py tests/test_unit/backends/ibis/test_upsert_render.py +git commit -m "feat(ibis): generic upsert ON DUPLICATE KEY + MySQL prove-safe preflight" +``` + +--- + +### Task 9: Cutover — flip `upsert` dispatch, retire `duckdb_family_upsert` + +**Files:** +- Modify: `src/mountainash_data/backends/ibis/backend.py` (dispatch + retype `update_condition`), `src/mountainash_data/backends/ibis/dialects/_registry.py` (remove 3 hook registrations), `src/mountainash_data/backends/ibis/operations.py` (delete `duckdb_family_upsert`) +- Test: existing upsert tests (must stay green via the generic path) + +**Interfaces:** +- Consumes: `_generic_upsert` (Tasks 6-8). +- Produces: `IbisBackend.upsert` dispatching hook-or-generic. + +- [ ] **Step 1: Find existing upsert tests and run them (baseline green via hook)** + +Run: `hatch run test:test-target-quick tests/ -k upsert -v` — note which pass today (via `duckdb_family_upsert`). + +- [ ] **Step 2: Flip dispatch in `backend.py`** + +Replace `upsert`'s body (currently raises when `upsert_hook is None`) with: + +```python + def upsert( + self, + name: str, + obj: t.Any, + *, + conflict_columns: list[str] | str, + update_columns: list[str] | str | None = None, + conflict_action: str = "UPDATE", + update_condition: t.Any = None, # ConditionPredicate | None + database: str | None = None, + schema: str | None = None, + ) -> IbisBackend: + conn = self._require_connected() + hook = self._spec.upsert_hook + if hook is not None: + hook( + conn._ibis_conn, name, obj, conflict_columns=conflict_columns, + update_columns=update_columns, conflict_action=conflict_action, + update_condition=update_condition, database=database, schema=schema, + ) + else: + _generic_upsert( + conn._ibis_conn, name, obj, style=self._spec.upsert_style, + conflict_columns=conflict_columns, update_columns=update_columns, + conflict_action=conflict_action, update_condition=update_condition, + database=database, schema=schema, + ) + return self +``` + +Add `_generic_upsert` to the operations import. (Keep `ConditionPredicate` type alias documented in `_render.py` / re-exported if the public API surfaces it.) + +- [ ] **Step 3: Remove the duckdb-family hook registrations** + +In `_registry.py`, delete the three `upsert_hook=duckdb_family_upsert,` lines (sqlite/duckdb/motherduck) — they already carry `upsert_style=ON_CONFLICT` from Task 3, so they now flow through the generic renderer. + +- [ ] **Step 4: Delete `duckdb_family_upsert`** + +Remove the `duckdb_family_upsert` function from `operations.py` and any now-unused imports it alone used (`uuid`, `contextlib`, `warnings` — keep any still used elsewhere; verify with ruff). + +- [ ] **Step 5: Run the existing upsert tests + the new render tests** + +Run: +```bash +hatch run test:test-target-quick tests/ -k upsert -v +docker compose up -d --wait && hatch run test:test-target-quick tests/test_integration -v ; docker compose down +``` +Expected: all upsert tests PASS via the generic path; live postgres/mysql round-trips PASS. If a previously-passing assertion encoded `duckdb_family_upsert`-specific behaviour (e.g. the staging-table temp name), update it to assert the data outcome instead — present any such test to the user before changing it (Test Integrity rule). + +- [ ] **Step 6: Lint, types, commit** + +```bash +hatch run ruff:check src +hatch run mypy:check +git add src/mountainash_data/backends/ibis/backend.py src/mountainash_data/backends/ibis/dialects/_registry.py src/mountainash_data/backends/ibis/operations.py tests/ +git commit -m "feat(ibis): cutover upsert to generic dispatch; retire duckdb_family_upsert" +``` + +--- + +### Task 10: Full-suite regression + matrix-completeness gate + +**Files:** none (verification only). + +- [ ] **Step 1: Golden-SQL matrix completeness** + +Confirm `test_upsert_style_registry.py::test_every_registry_dialect_has_an_explicit_decision` and the render suite iterate `DIALECTS` (no hardcoded count). Run: +`hatch run test:test-target-quick tests/test_unit/backends/ibis/ -v` — all PASS. + +- [ ] **Step 2: Full suite (no live) + live suite** + +```bash +hatch run test:test-target-quick tests/ -v +docker compose up -d --wait && MOUNTAINASH_REQUIRE_LIVE_DB=1 hatch run test:test-target-quick tests/test_integration -v ; docker compose down +``` +Expected: full unit/integration green; live job green with services up (and the `MOUNTAINASH_REQUIRE_LIVE_DB=1` run would FAIL if a service were down — proving the fail-closed gate). + +- [ ] **Step 3: Lint + types across the branch** + +```bash +hatch run ruff:check src +hatch run mypy:check +``` +Expected: both clean. + +- [ ] **Step 4: (If anything failed) fix and re-run** — do not proceed to PR until Steps 1-3 are green. + +--- + +## Out of Scope (tracked elsewhere) + +- `create_index` / `drop_index` / index-catalog queries — capability-divergent, stay hook-required. +- Consumer migration (mountainash-wearables → postgres) — in that repo after this ships. +- Live warehouse testing (snowflake/bigquery/mssql/…) — golden-SQL only here. +- An optional source-row `deduplicate=` flag — future extension (spec §11). + +## Self-Review + +**Spec coverage:** +- `upsert_style` registry field + assignment (spec §5.1, §7) → Task 3. ✓ +- `_render.py` primitives + `add_columns` consolidation (§5.2) → Task 1. ✓ +- `_generic_rename_table` + dispatch (§5.3, §4.3) → Task 4. ✓ +- Three upsert renderers (§5.4, §6) → Tasks 6 (ON CONFLICT), 7 (MERGE), 8 (ON DUPLICATE KEY). ✓ +- Compiled-subquery staging + column ordering + type casts (§5.5) → Task 6 (`compiled_source`). ✓ +- `update_condition` ibis-expression predicate + sentinel/alias-remap + grammar validator (§4.1, §6.1) → Task 5; integrated in Tasks 6/7. ✓ +- MySQL prove-safe-or-raise preflight (§6.2) → Task 8. ✓ +- Validation precedence (§10) → Task 6 (`_generic_upsert` ordering) + Task 8 (ON DUPLICATE KEY condition reject). ✓ +- Docker infra + skip/fail-closed fixtures + CI (§8) → Task 2. ✓ +- ibis pin >=12 + CLAUDE.md fix (§12) → Task 2. ✓ +- Cutover / delete `duckdb_family_upsert` (§13) → Task 9. ✓ +- Matrix completeness via registry iteration (§7, §8.4) → Task 3 + Task 10. ✓ + +**Placeholder scan:** the MERGE/ON_DUPLICATE branches are *intentional* `NotImplementedError` placeholders in Task 6 that Tasks 7/8 replace — each is a real, runnable line with a test that asserts the placeholder, then the next task removes it. No "TBD"/"add error handling"/uncoded steps remain. + +**Type consistency:** `_generic_upsert(ibis_conn, name, obj, *, style, conflict_columns, update_columns, conflict_action, update_condition, database, schema)` is identical across Tasks 6/8/9. `compiled_source(ibis_conn, obj, target_schema) -> (sql, cols)`, `compile_condition(ibis_conn, target_schema, predicate, *, incoming_alias, existing_alias)`, and the `_render_*` signatures match between definition and call sites. `UpsertStyle` members (`ON_CONFLICT`/`MERGE`/`ON_DUPLICATE_KEY`) are consistent across Tasks 3/6/7/8. + +**Known implementation-time confirmations (flagged inline, not placeholders):** exact sqlglot expression classes for rename (Task 4), the forbidden ibis op classes for the grammar validator (Task 5), and the raw-conn accessor on `IbisBackend` (Task 8) are each pinned by a one-line probe in their task; the behaviour contract is fixed by the tests in every case. From 91287e898dc9fdcbcd633c282c04ce73376ee359 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 22:41:39 +1000 Subject: [PATCH 06/23] docs: apply Codex plan review (3 CRITICAL + 4 HIGH) to dialect-ops plan C1 Task 8 tests call _generic_upsert directly (pass before Task 9 cutover) + Task 9 adds be.upsert dispatch round-trips. C2 ON CONFLICT renders INSERT..AS tgt + existing_alias=tgt. C3 EXCLUDED rendered as unquoted pseudo-relation (never quoted). H1 MySQL preflight: SEQ_IN_INDEX order, reject prefix/functional/ multi-unique/nullable. H2 compile_condition gains target_name + sentinel-collision check. H3 validator detects specific subquery ops not ops.Relation. H4 golden via pure build_*_sql(dialect=..) builders rendering correct sqlglot dialect; rename registry golden added. MEDIUM: column-existence + precedence; pure-builder structure; predicate-shape tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...6-29-generic-default-dialect-operations.md | 429 +++++++++++++----- 1 file changed, 328 insertions(+), 101 deletions(-) diff --git a/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md b/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md index 5b9d760..381500e 100644 --- a/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md +++ b/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md @@ -500,8 +500,23 @@ class TestGenericRenameTable: be.create_table("old", pl.DataFrame({"id": [1]})) assert be.rename_table("old", "new") is be assert "new" in be.list_tables() + + +class TestRenameGoldenPerDialect: + """Registry-iterating render assertion — every dialect renders a rename.""" + + @pytest.mark.parametrize("name", list(DIALECTS)) + def test_every_dialect_renders_rename(self, name): + from mountainash_data.backends.ibis.operations import build_rename_sql + d = _IBIS_TO_SQLGLOT.get(DIALECTS[name].ibis_backend_name, + DIALECTS[name].ibis_backend_name) + sql = build_rename_sql("old", "new", dialect=d) + # tsql renders sp_rename; everyone else an ALTER ... RENAME + assert ("sp_rename" in sql.lower()) or ("rename" in sql.lower()) ``` +Add the imports this test needs to the top of the file: `from mountainash_data.backends.ibis.dialects._registry import DIALECTS` and the shared `_IBIS_TO_SQLGLOT = {"mssql": "tsql"}` map (define once; reuse in the upsert golden test). + - [ ] **Step 2: Run to verify it fails** Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_rename_table_render.py -v` @@ -512,21 +527,26 @@ Expected: FAIL — `ImportError: cannot import name '_generic_rename_table'`. In `operations.py`, add (importing the helpers and `exp` at top if not already present: `from sqlglot import exp` is already imported for add_columns; add `from mountainash_data.backends.ibis._render import dialect_of, quote_identifier`): ```python -def _generic_rename_table(ibis_conn: t.Any, old_name: str, new_name: str) -> None: - """Rename a table via a sqlglot-rendered ALTER, portable across dialects. +def build_rename_sql(old_name: str, new_name: str, *, dialect: t.Any) -> str: + """Pure builder: render a portable rename for an explicit sqlglot dialect. sqlglot renders `ALTER TABLE … RENAME TO …` for most dialects, `EXEC - sp_rename …` for SQL Server, and `ALTER TABLE … RENAME …` for MySQL. + sp_rename …` for SQL Server (tsql), and `ALTER TABLE … RENAME …` for MySQL. + Taking `dialect` explicitly lets the registry-iterating golden test render + every dialect without a live connection. """ - _validate_simple_identifier(old_name, kind="old_name") - _validate_simple_identifier(new_name, kind="new_name") - dialect = dialect_of(ibis_conn) - stmt = exp.Alter( + return exp.Alter( this=exp.to_table(quote_identifier(old_name, dialect)), kind="TABLE", actions=[exp.AlterRename(this=exp.to_identifier(new_name, quoted=True))], ).sql(dialect=dialect) - ibis_conn.raw_sql(stmt) + + +def _generic_rename_table(ibis_conn: t.Any, old_name: str, new_name: str) -> None: + """Rename a table via the sqlglot generic default off the live connection.""" + _validate_simple_identifier(old_name, kind="old_name") + _validate_simple_identifier(new_name, kind="new_name") + ibis_conn.raw_sql(build_rename_sql(old_name, new_name, dialect=dialect_of(ibis_conn))) ``` > If `exp.Alter`/`exp.AlterRename` are not the exact class names in the installed sqlglot 30.x, use the verified-equivalent transpile fallback: `sqlglot.transpile(f'ALTER TABLE {quote_identifier(old_name, "")} RENAME TO {quote_identifier(new_name, "")}', read="duckdb", write=dialect)[0]`. Confirm via a one-line probe in the test env before settling. @@ -615,6 +635,7 @@ import ibis import pytest from mountainash_data.backends.ibis._render import ( + ConditionAliases, compile_condition, dialect_of, validate_predicate, @@ -622,51 +643,60 @@ from mountainash_data.backends.ibis._render import ( _SCHEMA = ibis.schema({"id": "int64", "updated_at": "timestamp", "v": "string"}) +# ON CONFLICT: incoming is the unquoted `excluded` pseudo-relation; existing is `tgt`. +_ONCONFLICT = ConditionAliases(incoming="excluded", existing="tgt", incoming_quoted=False) +# MERGE: both sides are normal quoted aliases. +_MERGE = ConditionAliases(incoming="src", existing="tgt") -def _render(con, predicate, *, incoming_alias, existing_alias): - ast = compile_condition( - con, _SCHEMA, predicate, - incoming_alias=incoming_alias, existing_alias=existing_alias, - ) + +def _render(con, predicate, aliases, *, target_name="t"): + ast = compile_condition(con, _SCHEMA, target_name, predicate, aliases=aliases) return ast.sql(dialect=dialect_of(con)) class TestCompileCondition: - def test_on_conflict_alias_mapping_duckdb(self): + def test_on_conflict_alias_mapping_unquoted_excluded(self): con = ibis.duckdb.connect() - sql = _render( - con, - lambda inc, exi: inc.updated_at > exi.updated_at, - incoming_alias="EXCLUDED", existing_alias="tgt", - ) - assert '"EXCLUDED"."updated_at"' in sql + sql = _render(con, lambda inc, exi: inc.updated_at > exi.updated_at, _ONCONFLICT) + # EXCLUDED is the unquoted pseudo-relation; existing is quoted "tgt" + assert "excluded." in sql.lower() and '"EXCLUDED"' not in sql assert '"tgt"."updated_at"' in sql def test_merge_alias_mapping_duckdb(self): con = ibis.duckdb.connect() - sql = _render( - con, - lambda inc, exi: inc.updated_at > exi.updated_at, - incoming_alias="src", existing_alias="tgt", - ) + sql = _render(con, lambda inc, exi: inc.updated_at > exi.updated_at, _MERGE) assert '"src"."updated_at"' in sql and '"tgt"."updated_at"' in sql def test_function_predicate_renders_per_dialect(self): con = ibis.duckdb.connect() - sql = _render( - con, - lambda inc, exi: inc.v.upper() != exi.v.upper(), - incoming_alias="src", existing_alias="tgt", - ) + sql = _render(con, lambda inc, exi: inc.v.upper() != exi.v.upper(), _MERGE) assert "UPPER(" in sql.upper() + def test_constant_predicate_renders(self): + con = ibis.duckdb.connect() + sql = _render(con, lambda inc, exi: inc.id > 0, _MERGE) + assert '"src"."id"' in sql + + def test_null_check_predicate_renders(self): + con = ibis.duckdb.connect() + sql = _render(con, lambda inc, exi: inc.v.notnull(), _MERGE) + assert "NULL" in sql.upper() + + def test_rejects_target_name_colliding_with_sentinel(self): + con = ibis.duckdb.connect() + with pytest.raises(ValueError, match="sentinel"): + _render(con, lambda inc, exi: inc.id > exi.id, _MERGE, + target_name="__ma_incoming__") + def test_rejects_aggregate_predicate(self): - with pytest.raises(ValueError, match="aggregat|window|subquer"): + with pytest.raises(ValueError, match="aggregat|window|scalar|subquer|row predicate"): validate_predicate( ibis.table(_SCHEMA, name="x").v.count() > 0 # aggregation ) ``` +> Add a window-function rejection test and a subquery/`EXISTS` rejection test once the exact ibis op classes are pinned by the Step-3 probe (the validator's `_SUBQUERY_OPS` tuple). Keep both as `pytest.raises(ValueError)`. + - [ ] **Step 2: Run to verify it fails** Run: `hatch run test:test-target-quick tests/test_unit/backends/ibis/test_upsert_condition_render.py -v` @@ -697,25 +727,49 @@ def validate_predicate(expr: ir.BooleanValue) -> None: f"{type(n).__name__} (aggregation/window). Use the upsert_hook " "override for conditions outside this grammar." ) - # subqueries / EXISTS surface as relational ops embedded in the predicate - for n in node.find((ops.Relation,)): # type: ignore[arg-type] + # subqueries / EXISTS — detect SPECIFIC subquery op types, NOT ops.Relation + # (ops.Relation also matches the two allowed sentinel tables and would + # reject every valid predicate — Codex finding). + for n in node.find(_SUBQUERY_OPS): # type: ignore[arg-type] raise ValueError( "update_condition may not contain subqueries/EXISTS/third-table " "references; use the upsert_hook override." ) +@dataclasses.dataclass(frozen=True) +class ConditionAliases: + """How each side's columns are referenced in the rendered clause. + + `quoted=False` is used for the ON CONFLICT `EXCLUDED` pseudo-relation, + which must NOT be a quoted identifier (Postgres exposes it as the special + unquoted `excluded`; quoting it risks referencing the wrong object). + """ + incoming: str # e.g. "EXCLUDED" (on conflict) or "src" (merge) + existing: str # e.g. "tgt" + incoming_quoted: bool = True + existing_quoted: bool = True + + def compile_condition( ibis_conn: t.Any, target_schema: t.Any, + target_name: str, predicate: t.Callable[[ir.Table, ir.Table], ir.BooleanValue], *, - incoming_alias: str, - existing_alias: str, + aliases: ConditionAliases, ) -> exp.Expression: """Render an (incoming, existing) -> bool predicate to a sqlglot ON sub-AST, - with incoming columns aliased to `incoming_alias` and existing to - `existing_alias`. See spec §6.1.""" + remapping incoming/existing columns to `aliases`. See spec §6.1. + + `target_name` is the real target table name — rejected if it collides with + a reserved sentinel (spec §6.1 step 0). + """ + if target_name in (INCOMING_SENTINEL, EXISTING_SENTINEL): + raise ValueError( + f"target table name {target_name!r} collides with a reserved " + f"sentinel; rename the table or use the upsert_hook override." + ) incoming = ibis.table(target_schema, name=INCOMING_SENTINEL) existing = ibis.table(target_schema, name=EXISTING_SENTINEL) pred = predicate(incoming, existing) @@ -725,12 +779,13 @@ def compile_condition( ast = ibis_conn.compiler.to_sqlglot(joined) ast = ast if isinstance(ast, exp.Expression) else ast[0] - alias_to_side = {} + # alias -> (target_alias, quoted) keyed by the underlying SENTINEL name + remap: dict[str, tuple[str, bool]] = {} for tbl in ast.find_all(exp.Table): if tbl.name == INCOMING_SENTINEL: - alias_to_side[tbl.alias_or_name] = incoming_alias + remap[tbl.alias_or_name] = (aliases.incoming, aliases.incoming_quoted) elif tbl.name == EXISTING_SENTINEL: - alias_to_side[tbl.alias_or_name] = existing_alias + remap[tbl.alias_or_name] = (aliases.existing, aliases.existing_quoted) join = next(ast.find_all(exp.Join), None) if join is None or join.args.get("on") is None: @@ -738,14 +793,29 @@ def compile_condition( on = join.args["on"].copy() def _remap(n: exp.Expression) -> exp.Expression: - if isinstance(n, exp.Column) and n.table in alias_to_side: - n.set("table", exp.to_identifier(alias_to_side[n.table], quoted=True)) + if isinstance(n, exp.Column) and n.table in remap: + alias, quoted = remap[n.table] + n.set("table", exp.to_identifier(alias, quoted=quoted)) return n return on.transform(_remap) ``` -> Pin the exact ibis op classes (`ops.Reduction`, `ops.WindowFunction`, `ops.Relation`) against the installed ibis 12 during this step — run a quick probe to confirm `expr.op().find((ops.Reduction,))` exists and behaves as used; adjust the forbidden-op tuple if the names differ. The behaviour contract (reject aggregate/window/subquery) is fixed by the tests. +Add `import dataclasses` and `import ibis.expr.operations as ops` to `_render.py`, plus the forbidden/subquery op tuples near the top: + +```python +_FORBIDDEN_OPS = (ops.Reduction, ops.WindowFunction) +# subquery/EXISTS op classes — pinned by probe (see note); NOT ops.Relation. +_SUBQUERY_OPS = tuple( + c for c in ( + getattr(ops, "ExistsSubquery", None), + getattr(ops, "InSubquery", None), + getattr(ops, "ScalarSubquery", None), + ) if c is not None +) +``` + +> **Pin by probe (required before writing the validator):** in the test env, confirm (a) `ops.Reduction` / `ops.WindowFunction` exist and `expr.op().find((ops.Reduction,))` flags an aggregate predicate; (b) the exact subquery op class names in ibis 12 (`ExistsSubquery`/`InSubquery`/`ScalarSubquery` or their current equivalents) and that a non-subquery predicate yields an empty `_SUBQUERY_OPS` match. The behaviour contract (accept scalar row predicates incl. the two sentinel tables; reject aggregate/window/subquery) is fixed by the tests in Step 1. - [ ] **Step 4: Run to verify it passes** @@ -908,10 +978,14 @@ def _generic_upsert( database: str | None, schema: str | None, ) -> None: + # Validation precedence per spec §10: style -> target existence -> identifiers + # -> conflict_action -> update_condition -> (odk preflight) -> updatable cols. if style is None: raise NotImplementedError( f"Dialect (connection {type(ibis_conn).__name__}) does not support upsert" ) + if not ibis_conn.table_exists(name, database=database): # target existence (§10.2) + raise ValueError(f"target table {name!r} does not exist") _validate_simple_identifier(name, kind="name") if database is not None: _validate_simple_identifier(database, kind="database") @@ -920,10 +994,24 @@ def _generic_upsert( target_schema = ibis_conn.table(name, database=database).schema() conflict = _normalize_columns(conflict_columns) + # column existence (§10 MEDIUM): fail loudly, not as a backend SQL error + missing = [c for c in conflict if c not in target_schema.names] + if missing: + raise ValueError(f"conflict_columns absent from target: {missing}") if update_columns is None: update = [c for c in target_schema.names if c not in conflict] else: update = _normalize_columns(update_columns) + missing_u = [c for c in update if c not in target_schema.names] + if missing_u: + raise ValueError(f"update_columns absent from target: {missing_u}") + + # update_condition validated unconditionally, BEFORE conflict_action handling (§10.5) + if update_condition is not None and style is UpsertStyle.ON_DUPLICATE_KEY: + raise ValueError( + "update_condition is not supported for the MySQL ON DUPLICATE KEY family" + ) + if conflict_action == "UPDATE" and not update: raise ValueError("no columns to update; provide update_columns or non-key columns") @@ -952,28 +1040,38 @@ def _render_on_conflict( target = qualified_name(parts, dialect) col_list = ", ".join(quote_identifier(c, dialect) for c in cols) conflict_list = ", ".join(quote_identifier(c, dialect) for c in conflict) + # Alias the target (INSERT INTO t AS tgt) so the existing row is referenced + # by a dedicated alias in the SET/WHERE — spec §6.1/§7. EXCLUDED is the + # UNQUOTED pseudo-relation (never a quoted identifier — Codex CRITICAL). + excl = "EXCLUDED" # unquoted keyword; Postgres exposes it as `excluded` if conflict_action == "NOTHING": action = f"ON CONFLICT ({conflict_list}) DO NOTHING" else: set_sql = ", ".join( - f"{quote_identifier(c, dialect)} = " - f'{quote_identifier("EXCLUDED", dialect)}.{quote_identifier(c, dialect)}' + f"{quote_identifier(c, dialect)} = {excl}.{quote_identifier(c, dialect)}" for c in update ) where = "" if update_condition is not None: + aliases = ConditionAliases( + incoming=excl, existing="tgt", incoming_quoted=False + ) cond = compile_condition( - ibis_conn, target_schema, update_condition, - incoming_alias="EXCLUDED", existing_alias=name, + ibis_conn, target_schema, name, update_condition, aliases=aliases, ).sql(dialect=dialect) where = f" WHERE {cond}" action = f"ON CONFLICT ({conflict_list}) DO UPDATE SET {set_sql}{where}" - return f"INSERT INTO {target} ({col_list}) SELECT {col_list} FROM ({source_sql}) AS __src {action}" + return ( + f"INSERT INTO {target} AS tgt ({col_list}) " + f"SELECT {col_list} FROM ({source_sql}) AS __src {action}" + ) ``` -> Note: when `update_condition` is supplied, the existing row is referenced by the bare table `name` (the default ON CONFLICT convention); if a future dialect needs `INSERT INTO t AS tgt` aliasing (spec §7), thread an alias through here. For the live-tested dialects (duckdb/sqlite/postgres) the bare-name form is valid. +Add `ConditionAliases` to the `_render` import line at the top of `operations.py`. + +> **§7 per-dialect alias gate:** `INSERT INTO t AS tgt … ON CONFLICT` is valid on the live `on_conflict` dialects (duckdb/sqlite/postgres). If a future `on_conflict` dialect rejects target aliasing *and* an `update_condition` is supplied, raise `ValueError` pointing at the `upsert_hook` (spec §7). The capability flag is set from live/golden verification; it does not affect unconditional upserts (which never reference the existing row). - [ ] **Step 5: Run to verify it passes** @@ -1079,26 +1177,65 @@ def _render_merge( Run: `hatch run test:test-target-quick "tests/test_unit/backends/ibis/test_upsert_render.py::TestMergeUpsert" -v` Expected: PASS (2 tests). If duckdb's MERGE grammar rejects a clause ordering, reorder to its accepted form (the data-outcome assertions are the contract). -- [ ] **Step 5: Add golden-SQL assertions for a warehouse dialect** (append): +- [ ] **Step 5: Per-dialect golden SQL via a pure builder (correct dialect, no live warehouse)** + +The conn-bound `_render_merge` cannot render Snowflake without a Snowflake connection (Codex: rendering "Snowflake golden SQL" through a DuckDB conn is not a Snowflake test). Split a **pure builder** that takes an explicit sqlglot `dialect` + an already-rendered `source_sql`, and make `_render_merge` (and the other two renderers) thin wrappers that compute `dialect`/`source_sql` from the live conn and delegate: ```python -class TestMergeGoldenSQL: - def test_snowflake_merge_shape(self): - # render-only: assert the emitted MERGE string shape for snowflake - from mountainash_data.backends.ibis.operations import _render_merge - con = ibis.duckdb.connect() - con.create_table("m", pl.DataFrame({"id": [1], "v": ["a"]})) - # compile against duckdb conn but assert structural tokens dialect-agnostically - sql = _render_merge( - con, "m", pl.DataFrame({"id": [1], "v": ["a"]}), - target_schema=con.table("m").schema(), conflict=["id"], update=["v"], - conflict_action="UPDATE", update_condition=None, database=None, schema=None, - ) - assert sql.startswith("MERGE INTO") - assert "WHEN MATCHED THEN UPDATE SET" in sql - assert "WHEN NOT MATCHED THEN INSERT" in sql +def build_merge_sql( + *, dialect, target, cols, conflict, update, conflict_action, + source_sql, condition_sql=None, +) -> str: + q = lambda c: quote_identifier(c, dialect) # noqa: E731 + on = " AND ".join(f"tgt.{q(c)} = src.{q(c)}" for c in conflict) + not_matched = ( + f"WHEN NOT MATCHED THEN INSERT ({', '.join(q(c) for c in cols)}) " + f"VALUES ({', '.join(f'src.{q(c)}' for c in cols)})" + ) + clauses = [] + if conflict_action == "UPDATE": + set_sql = ", ".join(f"{q(c)} = src.{q(c)}" for c in update) + cond = f" AND {condition_sql}" if condition_sql else "" + clauses.append(f"WHEN MATCHED{cond} THEN UPDATE SET {set_sql}") + clauses.append(not_matched) + return f"MERGE INTO {target} AS tgt USING ({source_sql}) AS src ON {on} " + " ".join(clauses) ``` +`_render_merge(ibis_conn, ...)` computes `dialect = dialect_of(ibis_conn)`, `source_sql, cols = compiled_source(ibis_conn, obj, target_schema)`, renders the condition AST with `dialect` if present, builds `target = qualified_name(...)`, and returns `build_merge_sql(...)`. (Apply the same pure-builder split to `build_on_conflict_sql` and `build_on_duplicate_key_sql` in Tasks 6/8 — each takes `dialect` + `source_sql`, so golden tests render any dialect.) + +Then a registry-iterating golden test renders each MERGE-family dialect with its **own** sqlglot dialect (mapping ibis backend name → sqlglot dialect; identity except `mssql`→`tsql`): + +```python +import pytest +from mountainash_data.backends.ibis.dialects._registry import DIALECTS, UpsertStyle +from mountainash_data.backends.ibis.operations import build_merge_sql + +_IBIS_TO_SQLGLOT = {"mssql": "tsql"} # extend if any other name differs + + +def _sqlglot_dialect(spec): + return _IBIS_TO_SQLGLOT.get(spec.ibis_backend_name, spec.ibis_backend_name) + + +@pytest.mark.parametrize( + "name", [n for n, s in DIALECTS.items() if s.upsert_style is UpsertStyle.MERGE] +) +def test_merge_golden_per_dialect(name): + d = _sqlglot_dialect(DIALECTS[name]) + sql = build_merge_sql( + dialect=d, target=f'"m"' if d not in {"tsql", "mysql"} else "m", + cols=["id", "v"], conflict=["id"], update=["v"], + conflict_action="UPDATE", source_sql="SELECT 1 AS id, 'a' AS v", + ) + assert sql.startswith("MERGE INTO") + assert "WHEN MATCHED THEN UPDATE SET" in sql + assert "WHEN NOT MATCHED THEN INSERT" in sql + # identifiers quoted in the target dialect's style + assert ("`" in sql) == (d in {"mysql"}) +``` + +This renders each warehouse dialect with the correct sqlglot dialect (real per-dialect emission), not DuckDB-as-Snowflake. Pin the `_IBIS_TO_SQLGLOT` map against the installed sqlglot during implementation (most ibis names equal their sqlglot dialect; `mssql`→`tsql` is the known exception). + - [ ] **Step 6: Run, lint, types, commit** ```bash @@ -1126,20 +1263,41 @@ git commit -m "feat(ibis): generic upsert MERGE branch (composite keys + conflic Create `tests/test_integration/test_upsert_mysql_preflight.py`: ```python -"""MySQL ON DUPLICATE KEY preflight: prove-safe-or-raise (spec §6.2).""" +"""MySQL ON DUPLICATE KEY preflight: prove-safe-or-raise (spec §6.2). + +Calls `_generic_upsert(...)` DIRECTLY against the raw mariadb connection — the +`be.upsert()` dispatch is not flipped until Task 9, so testing the generic +function directly is what keeps this task self-contained (Codex finding). +""" import polars as pl import pytest +from mountainash_data.backends.ibis.dialects._registry import UpsertStyle +from mountainash_data.backends.ibis.operations import _generic_upsert + + +def _raw(be): + # The fixture yields a connected IbisBackend; reach its raw ibis conn. + # Confirm the exact accessor against backend.py during implementation. + return be._require_connected()._ibis_conn + + +def _odk(con, name, df, conflict): + _generic_upsert( + con, name, df, style=UpsertStyle.ON_DUPLICATE_KEY, + conflict_columns=conflict, update_columns=None, conflict_action="UPDATE", + update_condition=None, database=None, schema=None, + ) + @pytest.mark.integration def test_single_pk_proceeds(mysql_backend): - be = mysql_backend - con = be._connection._ibis_conn # raw ibis conn + con = _raw(mysql_backend) con.raw_sql("DROP TABLE IF EXISTS odk_ok") con.raw_sql("CREATE TABLE odk_ok (id INT PRIMARY KEY, v VARCHAR(16) NOT NULL)") con.raw_sql("INSERT INTO odk_ok VALUES (1, 'a')") - be.upsert("odk_ok", pl.DataFrame({"id": [1, 2], "v": ["A", "b"]}), conflict_columns=["id"]) + _odk(con, "odk_ok", pl.DataFrame({"id": [1, 2], "v": ["A", "b"]}), ["id"]) rows = dict(con.table("odk_ok").order_by("id").execute()[["id", "v"]].itertuples(index=False)) assert rows == {1: "A", 2: "b"} con.raw_sql("DROP TABLE odk_ok") @@ -1147,67 +1305,90 @@ def test_single_pk_proceeds(mysql_backend): @pytest.mark.integration def test_multiple_unique_raises(mysql_backend): - be = mysql_backend - con = be._connection._ibis_conn + con = _raw(mysql_backend) con.raw_sql("DROP TABLE IF EXISTS odk_multi") con.raw_sql( "CREATE TABLE odk_multi " "(id INT PRIMARY KEY, email VARCHAR(64) NOT NULL UNIQUE, v VARCHAR(16) NOT NULL)" ) with pytest.raises(ValueError, match="unique"): - be.upsert("odk_multi", pl.DataFrame({"id": [1], "email": ["x"], "v": ["a"]}), conflict_columns=["id"]) + _odk(con, "odk_multi", pl.DataFrame({"id": [1], "email": ["x"], "v": ["a"]}), ["id"]) con.raw_sql("DROP TABLE odk_multi") +@pytest.mark.integration +def test_prefix_index_raises(mysql_backend): + con = _raw(mysql_backend) + con.raw_sql("DROP TABLE IF EXISTS odk_prefix") + con.raw_sql("CREATE TABLE odk_prefix (email VARCHAR(64) NOT NULL, v VARCHAR(16) NOT NULL, UNIQUE (email(10)))") + with pytest.raises(ValueError, match="prefix|SUB_PART"): + _odk(con, "odk_prefix", pl.DataFrame({"email": ["x"], "v": ["a"]}), ["email"]) + con.raw_sql("DROP TABLE odk_prefix") + + @pytest.mark.integration def test_nullable_conflict_column_raises(mysql_backend): - be = mysql_backend - con = be._connection._ibis_conn + con = _raw(mysql_backend) con.raw_sql("DROP TABLE IF EXISTS odk_null") con.raw_sql("CREATE TABLE odk_null (k INT NULL UNIQUE, v VARCHAR(16) NOT NULL)") with pytest.raises(ValueError, match="nullable|NOT NULL"): - be.upsert("odk_null", pl.DataFrame({"k": [1], "v": ["a"]}), conflict_columns=["k"]) + _odk(con, "odk_null", pl.DataFrame({"k": [1], "v": ["a"]}), ["k"]) con.raw_sql("DROP TABLE odk_null") ``` -(The exact `be._connection._ibis_conn` accessor: confirm the attribute path against `backend.py` — use whatever the existing code exposes for the raw connection. If `upsert` already runs through dispatch by this task, calling `be.upsert(...)` is enough and the raw-conn lines are only for setup DDL.) - - [ ] **Step 2: Run to verify it fails** (services up) Run: `docker compose up -d --wait && hatch run test:test-target-quick tests/test_integration/test_upsert_mysql_preflight.py -v` -Expected: FAIL — `NotImplementedError: unimplemented style: ON_DUPLICATE_KEY` (or dispatch not yet wired → see Task 9 ordering note). +Expected: FAIL — `NotImplementedError: unimplemented style: ON_DUPLICATE_KEY`. - [ ] **Step 3: Implement preflight + renderer** ```python def _mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None: - """Prove the safe MySQL ON DUPLICATE KEY case or raise (spec §6.2).""" - db = database or ibis_conn.current_database + """Prove the safe MySQL/MariaDB ON DUPLICATE KEY case or raise (spec §6.2). + + Fails closed on: no unique index, >1 unique index, prefix index (SUB_PART), + functional/expression index, a unique index whose ORDERED columns don't + exactly equal conflict_columns, or any nullable conflict column. + """ + db = database or _current_schema(ibis_conn) # see note: pin the resolver + # ORDER BY SEQ_IN_INDEX so composite-key column order is correct. + # EXPRESSION is non-NULL for functional/expression index parts (MySQL 8 / + # MariaDB); SUB_PART is non-NULL for prefix index parts. rows = ibis_conn.raw_sql( - "SELECT INDEX_NAME, COLUMN_NAME, SUB_PART, NON_UNIQUE " + "SELECT INDEX_NAME, SEQ_IN_INDEX, COLUMN_NAME, SUB_PART, EXPRESSION, NON_UNIQUE " "FROM information_schema.STATISTICS " - f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}'" + f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}' " + "ORDER BY INDEX_NAME, SEQ_IN_INDEX" ).fetchall() uniques: dict[str, list] = {} - for index_name, column_name, sub_part, non_unique in rows: + for index_name, _seq, column_name, sub_part, expression, non_unique in rows: if int(non_unique) == 0: - uniques.setdefault(index_name, []).append((column_name, sub_part)) + uniques.setdefault(index_name, []).append((column_name, sub_part, expression)) if not uniques: raise ValueError(f"table {name!r} has no unique/PK index for conflict_columns") - matching = [ - idx for idx, cols in uniques.items() - if [c for c, _ in cols] == list(conflict) and all(sp is None for _, sp in cols) - ] if len(uniques) > 1: raise ValueError( f"table {name!r} has multiple unique indexes {list(uniques)}; MySQL " - f"ON DUPLICATE KEY detects on any of them — ambiguous for conflict_columns=" - f"{conflict}. Use the upsert_hook override." + f"ON DUPLICATE KEY detects on any of them — ambiguous for " + f"conflict_columns={conflict}. Use the upsert_hook override." ) - if not matching: + (idx_name, parts), = uniques.items() + if any(expr is not None for _, _, expr in parts): raise ValueError( - f"no non-prefix unique index exactly matches conflict_columns={conflict} " - f"(found {uniques}); refusing to guess. Use the upsert_hook override." + f"unique index {idx_name!r} is a functional/expression index; cannot " + f"prove it matches conflict_columns={conflict}. Use the upsert_hook override." + ) + if any(sub is not None for _, sub, _ in parts): + raise ValueError( + f"unique index {idx_name!r} has a prefix (SUB_PART); it detects on a " + f"truncated value, not the full column. Use the upsert_hook override." + ) + if [c for c, _, _ in parts] != list(conflict): + raise ValueError( + f"unique index {idx_name!r} columns {[c for c, _, _ in parts]} do not " + f"exactly match conflict_columns={list(conflict)}; refusing to guess. " + f"Use the upsert_hook override." ) # nullable check cols_meta = ibis_conn.raw_sql( @@ -1219,9 +1400,14 @@ def _mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None: if bad: raise ValueError( f"conflict columns {bad} are nullable; MySQL unique indexes are " - f"NULL-distinct, so duplicates would insert. Make them NOT NULL or " - f"use the upsert_hook override." + f"NULL-distinct, so duplicates would insert instead of update. Make " + f"them NOT NULL or use the upsert_hook override." ) +``` + +> **Pin `_current_schema(ibis_conn)`** during implementation: ibis's current-database accessor differs by version (a `current_database`/`current_catalog` property vs a method). Probe the mysql backend in the test env and use the correct one (it must return the schema MariaDB resolves unqualified table names against). Do **not** assume `ibis_conn.current_database` is a property. + +> **MariaDB `VALUES(col)` note:** the renderer's UPDATE uses `VALUES(col)`, valid on the MariaDB 12.x target. MySQL 8.0.20+ deprecates `VALUES()` in favour of a row alias; if MySQL-8 support is later required, switch to the alias form behind the dialect — out of scope for the MariaDB-tested target here. def _render_on_duplicate_key( @@ -1248,12 +1434,12 @@ def _render_on_duplicate_key( ) ``` -Wire the ON_DUPLICATE_KEY branch in `_generic_upsert` to `stmt = _render_on_duplicate_key(...)`. Also enforce step-5 precedence (§10): when `style is ON_DUPLICATE_KEY and update_condition is not None`, raise `ValueError("update_condition is not supported for the MySQL ON DUPLICATE KEY family")` — place this check in `_generic_upsert` BEFORE the branch dispatch. +Wire the ON_DUPLICATE_KEY branch in `_generic_upsert` to `stmt = _render_on_duplicate_key(...)` (replacing the `NotImplementedError` placeholder). The `update_condition`-on-ODK → `ValueError` precedence check already lives in `_generic_upsert` from Task 6 (§10.5), so no extra check is needed here. - [ ] **Step 4: Run to verify it passes** (services up) Run: `hatch run test:test-target-quick tests/test_integration/test_upsert_mysql_preflight.py -v` -Expected: 3 PASS (after Task 9 wires dispatch; if running before Task 9, call `_generic_upsert` directly as in Task 6's tests). +Expected: 4 PASS (single-PK proceeds; multi-unique, prefix, nullable each raise `ValueError`). These call `_generic_upsert` directly, so they pass at this task's position regardless of the Task 9 dispatch flip. - [ ] **Step 5: Add golden-SQL for on_duplicate_key** (append to `test_upsert_render.py`, render-only, no live MySQL): assert `_render_on_duplicate_key`-style output contains `ON DUPLICATE KEY UPDATE` and `VALUES(` — but since it calls the preflight, test the pure render by factoring the SQL-string builder out of the preflight, or mark this assertion as covered by the live preflight test. Keep the unit layer to the string shape only. @@ -1327,7 +1513,36 @@ In `_registry.py`, delete the three `upsert_hook=duckdb_family_upsert,` lines (s Remove the `duckdb_family_upsert` function from `operations.py` and any now-unused imports it alone used (`uuid`, `contextlib`, `warnings` — keep any still used elsewhere; verify with ruff). -- [ ] **Step 5: Run the existing upsert tests + the new render tests** +- [ ] **Step 5: Add `be.upsert()` live round-trips (dispatch path, post-cutover)** + +Append to `tests/test_integration/test_write_ops_live.py` — these exercise the full dispatch (Task 8's preflight tests call `_generic_upsert` directly, so the public `be.upsert()` path needs its own coverage): + +```python +@pytest.mark.integration +def test_upsert_via_dispatch_postgres(postgres_backend): + be = postgres_backend + be.create_table("up_pg", pl.DataFrame({"id": [1, 2], "v": ["a", "b"]}), overwrite=True) + be._require_connected()._ibis_conn.raw_sql('ALTER TABLE up_pg ADD PRIMARY KEY (id)') + be.upsert("up_pg", pl.DataFrame({"id": [2, 3], "v": ["B", "c"]}), conflict_columns=["id"]) + rows = dict(be.table("up_pg").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "a", 2: "B", 3: "c"} + be.drop_table("up_pg", force=True) + + +@pytest.mark.integration +def test_upsert_via_dispatch_mysql(mysql_backend): + be = mysql_backend + con = be._require_connected()._ibis_conn + con.raw_sql("DROP TABLE IF EXISTS up_my") + con.raw_sql("CREATE TABLE up_my (id INT PRIMARY KEY, v VARCHAR(16) NOT NULL)") + con.raw_sql("INSERT INTO up_my VALUES (1, 'a')") + be.upsert("up_my", pl.DataFrame({"id": [1, 2], "v": ["A", "b"]}), conflict_columns=["id"]) + rows = dict(con.table("up_my").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "A", 2: "b"} + con.raw_sql("DROP TABLE up_my") +``` + +- [ ] **Step 6: Run the existing + new upsert tests** Run: ```bash @@ -1336,7 +1551,7 @@ docker compose up -d --wait && hatch run test:test-target-quick tests/test_integ ``` Expected: all upsert tests PASS via the generic path; live postgres/mysql round-trips PASS. If a previously-passing assertion encoded `duckdb_family_upsert`-specific behaviour (e.g. the staging-table temp name), update it to assert the data outcome instead — present any such test to the user before changing it (Test Integrity rule). -- [ ] **Step 6: Lint, types, commit** +- [ ] **Step 7: Lint, types, commit** ```bash hatch run ruff:check src @@ -1401,6 +1616,18 @@ Expected: both clean. **Placeholder scan:** the MERGE/ON_DUPLICATE branches are *intentional* `NotImplementedError` placeholders in Task 6 that Tasks 7/8 replace — each is a real, runnable line with a test that asserts the placeholder, then the next task removes it. No "TBD"/"add error handling"/uncoded steps remain. -**Type consistency:** `_generic_upsert(ibis_conn, name, obj, *, style, conflict_columns, update_columns, conflict_action, update_condition, database, schema)` is identical across Tasks 6/8/9. `compiled_source(ibis_conn, obj, target_schema) -> (sql, cols)`, `compile_condition(ibis_conn, target_schema, predicate, *, incoming_alias, existing_alias)`, and the `_render_*` signatures match between definition and call sites. `UpsertStyle` members (`ON_CONFLICT`/`MERGE`/`ON_DUPLICATE_KEY`) are consistent across Tasks 3/6/7/8. +**Type consistency:** `_generic_upsert(ibis_conn, name, obj, *, style, conflict_columns, update_columns, conflict_action, update_condition, database, schema)` is identical across Tasks 6/8/9. `compiled_source(ibis_conn, obj, target_schema) -> (sql, cols)`; `compile_condition(ibis_conn, target_schema, target_name, predicate, *, aliases: ConditionAliases) -> exp.Expression` (Task 5) matches its call sites in Tasks 6/7; the pure builders `build_rename_sql(old, new, *, dialect)`, `build_merge_sql(*, dialect, target, cols, conflict, update, conflict_action, source_sql, condition_sql=None)` (and `build_on_conflict_sql` / `build_on_duplicate_key_sql` per the Task 6/8 directive) take an explicit `dialect`, so golden tests render any dialect. `UpsertStyle` members are consistent across Tasks 3/6/7/8. + +**Known implementation-time confirmations (flagged inline, not placeholders):** exact sqlglot expression classes for rename (Task 4), the forbidden/subquery ibis op classes for the grammar validator (Task 5), the ibis→sqlglot dialect map (`mssql`→`tsql`, Tasks 4/7), the `_current_schema` resolver (Task 8), and the raw-conn accessor on `IbisBackend` (Tasks 8/9) are each pinned by a one-line probe in their task; the behaviour contract is fixed by the tests in every case. + +## Codex Plan Review — Disposition (applied) -**Known implementation-time confirmations (flagged inline, not placeholders):** exact sqlglot expression classes for rename (Task 4), the forbidden ibis op classes for the grammar validator (Task 5), and the raw-conn accessor on `IbisBackend` (Task 8) are each pinned by a one-line probe in their task; the behaviour contract is fixed by the tests in every case. +Two-pass-reviewed spec; this plan then went through a Codex pass that found 3 CRITICAL + 4 HIGH execution-breakers, all resolved in-plan before execution: +- **C1 task ordering** — Task 8's MySQL tests now call `_generic_upsert(...)` directly (not `be.upsert()`), so they pass before the Task 9 cutover; Task 9 adds the `be.upsert()` dispatch-path round-trips. +- **C2 ON CONFLICT aliasing** — renders `INSERT INTO t AS tgt`, `existing_alias="tgt"`; §7 per-dialect alias gate noted. +- **C3 EXCLUDED quoting** — `excluded` rendered as an UNQUOTED pseudo-relation (via `ConditionAliases(incoming_quoted=False)`), never `"EXCLUDED"`. +- **H1 MySQL preflight** — orders by `SEQ_IN_INDEX`; fails closed on multi-unique, prefix (`SUB_PART`), functional/expression (`EXPRESSION`), column-set mismatch, and nullable conflict columns; live prefix test added. +- **H2 `compile_condition`** — gains `target_name` + sentinel-collision check + test. +- **H3 validator** — detects specific subquery ops (`_SUBQUERY_OPS`), not `ops.Relation` (which would match the sentinels); probe-pinned. +- **H4 golden rendering** — replaced DuckDB-as-Snowflake with registry-iterating golden tests over pure `build_*_sql(*, dialect, …)` builders rendering each dialect's correct sqlglot dialect; added the `rename_table` registry-iterating golden. +- **MEDIUM** — column-existence validation + §10 precedence ordering in `_generic_upsert`; pure-builder structure (over string-concat-on-a-conn); added predicate-shape tests. **LOW** — `VALUES()` MariaDB/MySQL-8 note; `_current_schema` resolver pinned. From e502004f8e4157247c4d3f67c2721efffd2fd4f6 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 22:52:24 +1000 Subject: [PATCH 07/23] docs: apply Codex plan verification pass (2 CRITICAL + 2 HIGH + MEDIUM) CRITICAL: drop MySQL-8-only EXPRESSION from MariaDB preflight (detect functional indexes via NULL COLUMN_NAME); add SQLite-required WHERE true to INSERT..SELECT.. ON CONFLICT. HIGH: fix Task 7 MERGE stale compile_condition signature (target_name + ConditionAliases); expand _IBIS_TO_SQLGLOT (motherduck->duckdb, singlestoredb-> singlestore) + probe directive. MEDIUM: unconditional update_condition grammar validation via validate_condition; explicit build_on_conflict_sql/ build_on_duplicate_key_sql pure builders. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...6-29-generic-default-dialect-operations.md | 95 ++++++++++++++----- 1 file changed, 73 insertions(+), 22 deletions(-) diff --git a/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md b/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md index 381500e..b79ab66 100644 --- a/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md +++ b/docs/superpowers/plans/2026-06-29-generic-default-dialect-operations.md @@ -515,7 +515,19 @@ class TestRenameGoldenPerDialect: assert ("sp_rename" in sql.lower()) or ("rename" in sql.lower()) ``` -Add the imports this test needs to the top of the file: `from mountainash_data.backends.ibis.dialects._registry import DIALECTS` and the shared `_IBIS_TO_SQLGLOT = {"mssql": "tsql"}` map (define once; reuse in the upsert golden test). +Add the imports this test needs to the top of the file: `from mountainash_data.backends.ibis.dialects._registry import DIALECTS` and the shared ibis→sqlglot dialect map (define once; reuse in the upsert golden test): + +```python +# ibis backend name -> sqlglot dialect name (identity unless noted). +# Pinned by probe (see below): sqlglot 30.x has no "motherduck"/"singlestoredb". +_IBIS_TO_SQLGLOT = { + "mssql": "tsql", + "motherduck": "duckdb", + "singlestoredb": "singlestore", +} +``` + +> **Pin the full map by probe (required before the golden tests run).** For every `DIALECTS` entry, confirm `sqlglot.Dialect.get_or_raise()` succeeds — a name sqlglot 30.x doesn't know (e.g. it may not register `risingwave`/`exasol`/`databricks` under those exact strings) makes the golden test error, not assert. Where sqlglot lacks a dialect, map to its closest wire-compatible base (e.g. risingwave→`postgres`) and note it in the test; this is rendering-shape verification, so the base dialect's quoting/keywords are what we assert. (Codex HIGH-2.) - [ ] **Step 2: Run to verify it fails** @@ -799,8 +811,22 @@ def compile_condition( return n return on.transform(_remap) + + +def validate_condition(target_schema: t.Any, target_name: str, predicate) -> None: + """Grammar + sentinel-collision validation only (no rendering) — for the + unconditional §10.5 check in `_generic_upsert`.""" + if target_name in (INCOMING_SENTINEL, EXISTING_SENTINEL): + raise ValueError( + f"target table name {target_name!r} collides with a reserved sentinel." + ) + incoming = ibis.table(target_schema, name=INCOMING_SENTINEL) + existing = ibis.table(target_schema, name=EXISTING_SENTINEL) + validate_predicate(predicate(incoming, existing)) ``` +`compile_condition` should call `validate_condition` at its top rather than duplicating the collision/grammar checks (keep the validation in one place). + Add `import dataclasses` and `import ibis.expr.operations as ops` to `_render.py`, plus the forbidden/subquery op tuples near the top: ```python @@ -845,7 +871,7 @@ git commit -m "feat(ibis): conditional-predicate compiler (sentinel join->AST->r - Produces: - `compiled_source(ibis_conn, obj, target_schema) -> tuple[str, list[str]]` — `(subquery_sql, source_columns)`; columns cast to the target type and projected in target order. - `_generic_upsert(ibis_conn, name, obj, *, style, conflict_columns, update_columns, conflict_action, update_condition, database, schema) -> None`. - - `_render_on_conflict(...) -> str`. + - `build_on_conflict_sql(*, dialect, target, cols, conflict, update, conflict_action, source_sql, condition_sql=None) -> str` — the pure, dialect-parameterized builder (mirrors `build_merge_sql`, Task 7). `_render_on_conflict(ibis_conn, ...)` is the thin wrapper that derives `dialect`/`source_sql`/`condition_sql` from the live conn and delegates — so the registry-iterating upsert golden test (Task 7/10) can render the ON CONFLICT family per dialect without a live connection. This task wires `_generic_upsert` and the ON_CONFLICT branch only; MERGE/ON_DUPLICATE raise `NotImplementedError("unimplemented style")` as placeholders until Tasks 7/8. Dispatch is NOT flipped yet (Task 9), so this is exercised directly against a raw connection. @@ -962,7 +988,7 @@ def compiled_source( - [ ] **Step 4: Implement `_generic_upsert` + `_render_on_conflict` in `operations.py`** -Add imports at top: `from mountainash_data.backends.ibis._render import (compile_condition, compiled_source, qualified_name, quote_identifier, dialect_of)` and `from mountainash_data.backends.ibis.dialects._registry import UpsertStyle`. Then: +Add imports at top: `from mountainash_data.backends.ibis._render import (ConditionAliases, compile_condition, compiled_source, qualified_name, quote_identifier, dialect_of, validate_condition)` and `from mountainash_data.backends.ibis.dialects._registry import UpsertStyle`. Keep `import warnings` (used by the NOTHING-ignores-condition warning; it predates this work). Then: ```python def _generic_upsert( @@ -1006,11 +1032,18 @@ def _generic_upsert( if missing_u: raise ValueError(f"update_columns absent from target: {missing_u}") - # update_condition validated unconditionally, BEFORE conflict_action handling (§10.5) - if update_condition is not None and style is UpsertStyle.ON_DUPLICATE_KEY: - raise ValueError( - "update_condition is not supported for the MySQL ON DUPLICATE KEY family" - ) + # update_condition validated UNCONDITIONALLY, before any action-specific path + # (§10.5) — a malformed predicate must error even under NOTHING. + if update_condition is not None: + if style is UpsertStyle.ON_DUPLICATE_KEY: + raise ValueError( + "update_condition is not supported for the MySQL ON DUPLICATE KEY family" + ) + # validate grammar + sentinel collision now (raises on aggregate/window/ + # subquery or a target name colliding with a sentinel) + validate_condition(target_schema, name, update_condition) + if conflict_action == "NOTHING": + warnings.warn("update_condition is ignored when conflict_action='NOTHING'") if conflict_action == "UPDATE" and not update: raise ValueError("no columns to update; provide update_columns or non-key columns") @@ -1063,9 +1096,12 @@ def _render_on_conflict( where = f" WHERE {cond}" action = f"ON CONFLICT ({conflict_list}) DO UPDATE SET {set_sql}{where}" + # `WHERE true` is REQUIRED by SQLite to disambiguate INSERT…SELECT…ON CONFLICT + # (its parser errors near DO without it); harmless on duckdb/postgres. This + # mirrors the original duckdb_family_upsert template. (Codex CRITICAL-2.) return ( f"INSERT INTO {target} AS tgt ({col_list}) " - f"SELECT {col_list} FROM ({source_sql}) AS __src {action}" + f"SELECT {col_list} FROM ({source_sql}) AS __src WHERE true {action}" ) ``` @@ -1159,9 +1195,10 @@ def _render_merge( set_sql = ", ".join(f"{q(c)} = src.{q(c)}" for c in update) cond = "" if update_condition is not None: + # NEW compile_condition signature (Task 5): target_name + aliases. cond = " AND " + compile_condition( - ibis_conn, target_schema, update_condition, - incoming_alias="src", existing_alias="tgt", + ibis_conn, target_schema, name, update_condition, + aliases=ConditionAliases(incoming="src", existing="tgt"), ).sql(dialect=dialect) clauses.append(f"WHEN MATCHED{cond} THEN UPDATE SET {set_sql}") clauses.append(not_matched) @@ -1172,6 +1209,8 @@ def _render_merge( ) ``` +Add `ConditionAliases` to the `_render` import in `operations.py` (shared with Task 6). + - [ ] **Step 4: Run to verify it passes** Run: `hatch run test:test-target-quick "tests/test_unit/backends/ibis/test_upsert_render.py::TestMergeUpsert" -v` @@ -1210,7 +1249,9 @@ import pytest from mountainash_data.backends.ibis.dialects._registry import DIALECTS, UpsertStyle from mountainash_data.backends.ibis.operations import build_merge_sql -_IBIS_TO_SQLGLOT = {"mssql": "tsql"} # extend if any other name differs +# Reuse the shared, probe-pinned map (mssql->tsql, motherduck->duckdb, +# singlestoredb->singlestore, + any sqlglot-unknown dialect mapped to its base). +from tests.test_unit.backends.ibis.test_rename_table_render import _IBIS_TO_SQLGLOT def _sqlglot_dialect(spec): @@ -1256,7 +1297,7 @@ git commit -m "feat(ibis): generic upsert MERGE branch (composite keys + conflic **Interfaces:** - Consumes: `compiled_source`, `qualified_name`, `quote_identifier` (Tasks 1/6). -- Produces: `_render_on_duplicate_key(...) -> str`; `_mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None` (prove-safe-or-raise preflight, §6.2); wired into the `_generic_upsert` ON_DUPLICATE_KEY branch. +- Produces: `build_on_duplicate_key_sql(*, dialect, target, cols, conflict, update, conflict_action, source_sql) -> str` (pure builder, mirrors `build_merge_sql`); `_render_on_duplicate_key(ibis_conn, ...)` thin wrapper that runs `_mysql_validate_conflict_key` then delegates; `_mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None` (prove-safe-or-raise preflight, §6.2); wired into the `_generic_upsert` ON_DUPLICATE_KEY branch. - [ ] **Step 1: Write the failing live tests** @@ -1353,18 +1394,21 @@ def _mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None: """ db = database or _current_schema(ibis_conn) # see note: pin the resolver # ORDER BY SEQ_IN_INDEX so composite-key column order is correct. - # EXPRESSION is non-NULL for functional/expression index parts (MySQL 8 / - # MariaDB); SUB_PART is non-NULL for prefix index parts. + # NOTE: do NOT select EXPRESSION — it exists only in MySQL 8's STATISTICS, + # not MariaDB's (the test image is mariadb:12.1.2), so selecting it errors + # the query (Codex CRITICAL-1). Functional/expression index PARTS have a + # NULL COLUMN_NAME on BOTH MariaDB and MySQL 8 — detect them that way. + # SUB_PART is non-NULL for prefix index parts (present on both engines). rows = ibis_conn.raw_sql( - "SELECT INDEX_NAME, SEQ_IN_INDEX, COLUMN_NAME, SUB_PART, EXPRESSION, NON_UNIQUE " + "SELECT INDEX_NAME, SEQ_IN_INDEX, COLUMN_NAME, SUB_PART, NON_UNIQUE " "FROM information_schema.STATISTICS " f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}' " "ORDER BY INDEX_NAME, SEQ_IN_INDEX" ).fetchall() uniques: dict[str, list] = {} - for index_name, _seq, column_name, sub_part, expression, non_unique in rows: + for index_name, _seq, column_name, sub_part, non_unique in rows: if int(non_unique) == 0: - uniques.setdefault(index_name, []).append((column_name, sub_part, expression)) + uniques.setdefault(index_name, []).append((column_name, sub_part)) if not uniques: raise ValueError(f"table {name!r} has no unique/PK index for conflict_columns") if len(uniques) > 1: @@ -1374,19 +1418,19 @@ def _mysql_validate_conflict_key(ibis_conn, name, conflict, database) -> None: f"conflict_columns={conflict}. Use the upsert_hook override." ) (idx_name, parts), = uniques.items() - if any(expr is not None for _, _, expr in parts): + if any(col is None for col, _ in parts): # NULL COLUMN_NAME = functional part raise ValueError( f"unique index {idx_name!r} is a functional/expression index; cannot " f"prove it matches conflict_columns={conflict}. Use the upsert_hook override." ) - if any(sub is not None for _, sub, _ in parts): + if any(sub is not None for _, sub in parts): raise ValueError( f"unique index {idx_name!r} has a prefix (SUB_PART); it detects on a " f"truncated value, not the full column. Use the upsert_hook override." ) - if [c for c, _, _ in parts] != list(conflict): + if [c for c, _ in parts] != list(conflict): raise ValueError( - f"unique index {idx_name!r} columns {[c for c, _, _ in parts]} do not " + f"unique index {idx_name!r} columns {[c for c, _ in parts]} do not " f"exactly match conflict_columns={list(conflict)}; refusing to guess. " f"Use the upsert_hook override." ) @@ -1631,3 +1675,10 @@ Two-pass-reviewed spec; this plan then went through a Codex pass that found 3 CR - **H3 validator** — detects specific subquery ops (`_SUBQUERY_OPS`), not `ops.Relation` (which would match the sentinels); probe-pinned. - **H4 golden rendering** — replaced DuckDB-as-Snowflake with registry-iterating golden tests over pure `build_*_sql(*, dialect, …)` builders rendering each dialect's correct sqlglot dialect; added the `rename_table` registry-iterating golden. - **MEDIUM** — column-existence validation + §10 precedence ordering in `_generic_upsert`; pure-builder structure (over string-concat-on-a-conn); added predicate-shape tests. **LOW** — `VALUES()` MariaDB/MySQL-8 note; `_current_schema` resolver pinned. + +A second Codex pass verified the above all RESOLVED and caught issues the revisions introduced — all fixed: +- **CRITICAL (MariaDB `EXPRESSION`)** — that column is MySQL-8-only; the preflight query would error on `mariadb:12.1.2`. Dropped it; functional indexes detected via `COLUMN_NAME IS NULL` (portable across MariaDB + MySQL 8). +- **CRITICAL (SQLite UPSERT)** — `INSERT … SELECT … ON CONFLICT` needs a discriminating `WHERE true`; added (mirrors the original `duckdb_family_upsert` template). +- **HIGH (stale call)** — Task 7's MERGE renderer still used the old `compile_condition` positional signature; updated to `target_name` + `ConditionAliases(incoming="src", existing="tgt")`. +- **HIGH (dialect map)** — expanded `_IBIS_TO_SQLGLOT` (`+motherduck→duckdb`, `+singlestoredb→singlestore`) with a probe directive to validate every name against sqlglot 30.x and map sqlglot-unknown dialects to their wire-compatible base. +- **MEDIUM** — `update_condition` grammar now validated unconditionally via `validate_condition` (errors even under NOTHING, which warns-and-ignores); `build_on_conflict_sql` / `build_on_duplicate_key_sql` made explicit pure-builder outputs. From 75a61a2acb856db948fd021c76278fff56cd31d1 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 23:34:18 +1000 Subject: [PATCH 08/23] feat(ibis): extract shared _render.py primitives; migrate add_columns Co-Authored-By: Claude Sonnet 4.6 --- src/mountainash_data/backends/ibis/_render.py | 31 +++++++++++++++++++ .../backends/ibis/operations.py | 9 ++---- .../backends/ibis/test_render_primitives.py | 28 +++++++++++++++++ 3 files changed, 62 insertions(+), 6 deletions(-) create mode 100644 src/mountainash_data/backends/ibis/_render.py create mode 100644 tests/test_unit/backends/ibis/test_render_primitives.py diff --git a/src/mountainash_data/backends/ibis/_render.py b/src/mountainash_data/backends/ibis/_render.py new file mode 100644 index 0000000..f400a53 --- /dev/null +++ b/src/mountainash_data/backends/ibis/_render.py @@ -0,0 +1,31 @@ +"""Shared sqlglot rendering primitives for dialect-agnostic write ops. + +Everything renders off a *live* ibis connection's own compiler, so identifier +quoting and type rendering match what ibis emits for create_table. +""" + +from __future__ import annotations + +import typing as t + +from sqlglot import exp + + +def dialect_of(ibis_conn: t.Any) -> t.Any: + """The live connection's sqlglot dialect (NOT ibis's backend name).""" + return ibis_conn.compiler.dialect + + +def quote_identifier(name: str, dialect: t.Any) -> str: + """Quote a single identifier for `dialect` via sqlglot.""" + return exp.to_identifier(name, quoted=True).sql(dialect=dialect) + + +def qualified_name(parts: list[str], dialect: t.Any) -> str: + """Quote each part and join with '.' (e.g. database.table).""" + return ".".join(quote_identifier(p, dialect) for p in parts) + + +def render_type(type_mapper: t.Any, dtype: t.Any) -> str: + """Render an ibis dtype to SQL via the connection's type-mapper.""" + return type_mapper.to_string(dtype) diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index 380b58e..f628a3e 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -14,12 +14,12 @@ import ibis import mountainash as ma -from sqlglot import exp from mountainash_data.core.constants import ( CONST_CONFLICT_ACTION, CONST_INDEX_TYPE, ) +from mountainash_data.backends.ibis._render import quote_identifier # =========================================================================== @@ -165,11 +165,8 @@ def _generic_add_columns( type_mapper = ibis_conn.compiler.type_mapper dialect = ibis_conn.compiler.dialect - def _quote(identifier: str) -> str: - return exp.to_identifier(identifier, quoted=True).sql(dialect=dialect) - table_parts = [database, table_name] if database else [table_name] - qualified = ".".join(_quote(part) for part in table_parts) + qualified = ".".join(quote_identifier(part, dialect) for part in table_parts) for col_name, dtype in candidate.items(): if col_name in existing: @@ -178,7 +175,7 @@ def _quote(identifier: str) -> str: dtype = ibis.dtype("string") type_sql = type_mapper.to_string(dtype) ibis_conn.raw_sql( - f"ALTER TABLE {qualified} ADD COLUMN {_quote(col_name)} {type_sql}" + f"ALTER TABLE {qualified} ADD COLUMN {quote_identifier(col_name, dialect)} {type_sql}" ) diff --git a/tests/test_unit/backends/ibis/test_render_primitives.py b/tests/test_unit/backends/ibis/test_render_primitives.py new file mode 100644 index 0000000..dcd3b3c --- /dev/null +++ b/tests/test_unit/backends/ibis/test_render_primitives.py @@ -0,0 +1,28 @@ +"""Unit tests for the shared sqlglot rendering primitives.""" + +import ibis + +from mountainash_data.backends.ibis._render import ( + dialect_of, + qualified_name, + quote_identifier, + render_type, +) + + +class TestRenderPrimitives: + def test_quote_identifier_duckdb(self): + d = dialect_of(ibis.duckdb.connect()) + assert quote_identifier("new col", d) == '"new col"' + + def test_quote_identifier_mysql_backticks(self): + # mysql connect needs a server; render via a sqlglot dialect string instead + assert quote_identifier("c", "mysql") == "`c`" + + def test_qualified_name_two_parts(self): + assert qualified_name(["db", "t"], "duckdb") == '"db"."t"' + + def test_render_type_matches_create_table_mapper(self): + con = ibis.duckdb.connect() + tm = con.compiler.type_mapper + assert render_type(tm, ibis.dtype("int64")) == tm.to_string(ibis.dtype("int64")) From 84b01b2d1716b7806cbd5e9aea4a2a6f4342d37d Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 23:48:56 +1000 Subject: [PATCH 09/23] test(infra): docker postgres+mariadb services, live fixtures, ibis>=12 pin Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/python-run-pytest.yml | 46 +++++++++++++++++++++ CLAUDE.md | 2 +- compose.yaml | 26 ++++++++++++ hatch.toml | 13 ++++++ pyproject.toml | 32 +++++++-------- tests/conftest.py | 2 + tests/fixtures/database_fixtures.py | 49 +++++++++++++++++++++++ tests/test_integration/test_live_smoke.py | 13 ++++++ 8 files changed, 166 insertions(+), 17 deletions(-) create mode 100644 compose.yaml create mode 100644 tests/test_integration/test_live_smoke.py diff --git a/.github/workflows/python-run-pytest.yml b/.github/workflows/python-run-pytest.yml index 6163c48..05c8ead 100644 --- a/.github/workflows/python-run-pytest.yml +++ b/.github/workflows/python-run-pytest.yml @@ -27,6 +27,47 @@ jobs: os: [ubuntu-24.04] python-version: ["3.12"] + env: + MOUNTAINASH_REQUIRE_LIVE_DB: "1" + IBIS_TEST_POSTGRES_HOST: localhost + IBIS_TEST_POSTGRES_PORT: "5432" + IBIS_TEST_POSTGRES_USER: postgres + IBIS_TEST_POSTGRES_PASSWORD: postgres + IBIS_TEST_POSTGRES_DATABASE: ibis_testing + IBIS_TEST_MYSQL_HOST: localhost + IBIS_TEST_MYSQL_PORT: "3306" + IBIS_TEST_MYSQL_USER: ibis + IBIS_TEST_MYSQL_PASSWORD: ibis + IBIS_TEST_MYSQL_DATABASE: ibis_testing + + services: + postgres: + image: postgres:18-alpine + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: ibis_testing + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres" + --health-interval 1s + --health-retries 20 + + mysql: + image: mariadb:12.1.2 + env: + MYSQL_ALLOW_EMPTY_PASSWORD: "true" + MYSQL_DATABASE: ibis_testing + MYSQL_USER: ibis + MYSQL_PASSWORD: ibis + ports: + - 3306:3306 + options: >- + --health-cmd "mariadb-admin ping -h localhost" + --health-interval 1s + --health-retries 20 + steps: - name: Set fallback branch run: | @@ -48,6 +89,11 @@ jobs: python-version: ${{ matrix.python-version }} check-latest: true + - name: Install system dependencies for live-db drivers + run: | + sudo apt-get update -q + sudo apt-get install -y libmariadb-dev + - name: Display SQLite version run: | python -c "import sqlite3; print(f'SQLite version: {sqlite3.sqlite_version}')" diff --git a/CLAUDE.md b/CLAUDE.md index f892e3b..47e343a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -109,7 +109,7 @@ src/mountainash_data/ ## Dependencies ### Core Dependencies -- **ibis-framework[polars,pandas,sqlite,duckdb]** == 10.4.0 - Core data processing framework +- **ibis-framework[polars,pandas,sqlite,duckdb]** >= 12.0.0 - Core data processing framework - **numpy** >=1.23.2,<3 - Numerical computing - **pandas** >=2.2.0 - Data manipulation and analysis - **polars** ==1.16.0 - Fast DataFrame library diff --git a/compose.yaml b/compose.yaml new file mode 100644 index 0000000..4772c00 --- /dev/null +++ b/compose.yaml @@ -0,0 +1,26 @@ +services: + postgres: + image: postgres:18-alpine + environment: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: ibis_testing + healthcheck: + test: ["CMD", "pg_isready", "-U", "postgres"] + interval: 1s + retries: 20 + ports: + - "5432:5432" + mysql: + image: mariadb:12.1.2 + environment: + MYSQL_ALLOW_EMPTY_PASSWORD: "true" + MYSQL_DATABASE: ibis_testing + MYSQL_USER: ibis + MYSQL_PASSWORD: ibis + healthcheck: + test: ["CMD", "mariadb-admin", "ping", "-h", "localhost"] + interval: 1s + retries: 20 + ports: + - "3306:3306" diff --git a/hatch.toml b/hatch.toml index 90338eb..dc10e56 100644 --- a/hatch.toml +++ b/hatch.toml @@ -82,6 +82,11 @@ dependencies = [ "pytest-check==2.5.3", "pytest-cov==6.1.1", + # Live-db extras (system libmariadb-dev installed in CI before this env) + "psycopg-binary>=3.1.0", + "ibis-framework[postgres]>=12.0.0", + "ibis-framework[mysql]>=12.0.0", + "mountainash_settings @ {root:uri}/temp/mountainash-settings", "mountainash @ {root:uri}/temp/mountainash", "mountainash_transport @ {root:uri}/temp/mountainash-transport", @@ -120,6 +125,13 @@ dependencies = [ "psutil>=7.0.0", + # Live-db test extras: postgres driver for integration tests. + # ibis 12 uses psycopg (psycopg3) for postgres — needs psycopg-binary for + # a pure-Python/bundled-libpq build that works without system libpq-dev. + # mysql/mariadb (ibis-framework[mysql]) requires mysqlclient which needs + # libmariadb-dev system headers — installed via apt in CI, skipped locally. + "psycopg-binary>=3.1.0", + "ibis-framework[postgres]>=12.0.0", "mountainash_settings @ {root:uri}/../mountainash-settings", "mountainash @ {root:uri}/../mountainash", @@ -181,6 +193,7 @@ test-perf-target = "pytest --benchmark-only {args}" test-unit = "pytest -m unit" test-integration = "pytest -m integration" test-performance = "pytest -m performance" +test-live = "docker compose up -d --wait && pytest -m integration {args}" # =========================================== # CI/REPORTING - For automated environments diff --git a/pyproject.toml b/pyproject.toml index 534e860..abee702 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] dependencies = [ - "ibis-framework[polars,sqlite,duckdb]>=11.0.0", + "ibis-framework[polars,sqlite,duckdb]>=12.0.0", # "hatchling", @@ -54,26 +54,26 @@ all = [ "pyodbc==5.2.0", "snowflake-connector-python==3.12.3", "setuptools", - "ibis-framework[mssql,snowflake,postgres,pyspark,trino]>=11.0.0", + "ibis-framework[mssql,snowflake,postgres,pyspark,trino]>=12.0.0", # "ibis-framework[mssql,snowflake,postgres,bigquery,pyspark,trino]>=10.8.0", ] -mssql = ["pyodbc==5.2.0", "ibis-framework[mssql]>=11.0.0"] +mssql = ["pyodbc==5.2.0", "ibis-framework[mssql]>=12.0.0"] snowflake = [ "snowflake-connector-python==3.12.3", - "ibis-framework[snowflake]>=11.0.0", + "ibis-framework[snowflake]>=12.0.0", ] -postgres = ["psycopg2-binary==2.9.9", "ibis-framework[postgres]>=11.0.0"] -bigquery = ["ibis-framework[bigquery]>=11.0.0"] -pyspark = ["setuptools", "ibis-framework[pyspark]>=11.0.0"] -clickhouse = ["ibis-framework[clickhouse]>=11.0.0"] -databricks = ["databricks-sql-connector>=4", "ibis-framework[databricks]>=11.0.0"] -singlestoredb = ["singlestoredb>=1.0", "ibis-framework[singlestoredb]>=11.0.0"] -exasol = ["pyexasol>=0.25.2", "ibis-framework[exasol]>=11.0.0"] -impala = ["impyla>=0.17", "ibis-framework[impala]>=11.0.0"] -materialize = ["psycopg>=3.2.0", "ibis-framework[materialize]>=11.0.0"] -risingwave = ["psycopg2>=2.8.4", "ibis-framework[risingwave]>=11.0.0"] -druid = ["pydruid>=0.6.7", "ibis-framework[druid]>=11.0.0"] -trino = ["ibis-framework[trino]>=11.0.0"] +postgres = ["psycopg2-binary==2.9.9", "ibis-framework[postgres]>=12.0.0"] +bigquery = ["ibis-framework[bigquery]>=12.0.0"] +pyspark = ["setuptools", "ibis-framework[pyspark]>=12.0.0"] +clickhouse = ["ibis-framework[clickhouse]>=12.0.0"] +databricks = ["databricks-sql-connector>=4", "ibis-framework[databricks]>=12.0.0"] +singlestoredb = ["singlestoredb>=1.0", "ibis-framework[singlestoredb]>=12.0.0"] +exasol = ["pyexasol>=0.25.2", "ibis-framework[exasol]>=12.0.0"] +impala = ["impyla>=0.17", "ibis-framework[impala]>=12.0.0"] +materialize = ["psycopg>=3.2.0", "ibis-framework[materialize]>=12.0.0"] +risingwave = ["psycopg2>=2.8.4", "ibis-framework[risingwave]>=12.0.0"] +druid = ["pydruid>=0.6.7", "ibis-framework[druid]>=12.0.0"] +trino = ["ibis-framework[trino]>=12.0.0"] [project.urls] diff --git a/tests/conftest.py b/tests/conftest.py index 3d098b3..ef1ea6f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,6 +9,8 @@ # Import all fixtures from consolidated fixture modules from fixtures.database_fixtures import ( + mysql_backend, + postgres_backend, temp_sqlite_db, temp_duckdb_db, ibis_sqlite_backend, diff --git a/tests/fixtures/database_fixtures.py b/tests/fixtures/database_fixtures.py index 13d4326..b3cd629 100644 --- a/tests/fixtures/database_fixtures.py +++ b/tests/fixtures/database_fixtures.py @@ -1,5 +1,6 @@ """Database-related fixtures for testing.""" +import os import pytest import tempfile import sqlite3 @@ -7,6 +8,54 @@ from typing import Generator import ibis +from mountainash_data import IbisBackend + +_PG = dict( + host=os.environ.get("IBIS_TEST_POSTGRES_HOST", os.environ.get("PGHOST", "localhost")), + port=int(os.environ.get("IBIS_TEST_POSTGRES_PORT", os.environ.get("PGPORT", "5432"))), + user=os.environ.get("IBIS_TEST_POSTGRES_USER", os.environ.get("PGUSER", "postgres")), + password=os.environ.get("IBIS_TEST_POSTGRES_PASSWORD", os.environ.get("PGPASSWORD", "postgres")), + database=os.environ.get("IBIS_TEST_POSTGRES_DATABASE", os.environ.get("PGDATABASE", "ibis_testing")), +) +_MY = dict( + host=os.environ.get("IBIS_TEST_MYSQL_HOST", "localhost"), + port=int(os.environ.get("IBIS_TEST_MYSQL_PORT", "3306")), + user=os.environ.get("IBIS_TEST_MYSQL_USER", "ibis"), + password=os.environ.get("IBIS_TEST_MYSQL_PASSWORD", "ibis"), + database=os.environ.get("IBIS_TEST_MYSQL_DATABASE", "ibis_testing"), +) + + +def _live_or_skip(dialect: str, params: dict): + require = os.environ.get("MOUNTAINASH_REQUIRE_LIVE_DB") == "1" + try: + be = IbisBackend(dialect=dialect, **params) + be.connect() + return be + except Exception as exc: # noqa: BLE001 - service availability gate + msg = f"{dialect} service unreachable: {exc}" + if require: + pytest.fail(msg) + pytest.skip(msg) + + +@pytest.fixture +def postgres_backend(): + be = _live_or_skip("postgres", _PG) + try: + yield be + finally: + be.close() + + +@pytest.fixture +def mysql_backend(): + be = _live_or_skip("mysql", _MY) + try: + yield be + finally: + be.close() + @pytest.fixture(scope="session") def temp_sqlite_db() -> Generator[Path, None, None]: diff --git a/tests/test_integration/test_live_smoke.py b/tests/test_integration/test_live_smoke.py new file mode 100644 index 0000000..ef2ffe1 --- /dev/null +++ b/tests/test_integration/test_live_smoke.py @@ -0,0 +1,13 @@ +"""Smoke test that the live-db fixtures connect or skip correctly.""" + +import pytest + + +@pytest.mark.integration +def test_postgres_smoke(postgres_backend): + assert isinstance(postgres_backend.list_tables(), list) + + +@pytest.mark.integration +def test_mysql_smoke(mysql_backend): + assert isinstance(mysql_backend.list_tables(), list) From c3f458b95dfb3581c1381b96594620651acf63fe Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Mon, 29 Jun 2026 23:57:30 +1000 Subject: [PATCH 10/23] fix(deps): postgres/all extras use psycopg3 (ibis 12 dropped psycopg2) Co-Authored-By: Claude Opus 4.8 (1M context) --- pyproject.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index abee702..e835438 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -50,7 +50,7 @@ classifiers = [ [project.optional-dependencies] all = [ - "psycopg2-binary==2.9.9", + "psycopg-binary>=3.1.0", "pyodbc==5.2.0", "snowflake-connector-python==3.12.3", "setuptools", @@ -62,7 +62,7 @@ snowflake = [ "snowflake-connector-python==3.12.3", "ibis-framework[snowflake]>=12.0.0", ] -postgres = ["psycopg2-binary==2.9.9", "ibis-framework[postgres]>=12.0.0"] +postgres = ["psycopg-binary>=3.1.0", "ibis-framework[postgres]>=12.0.0"] bigquery = ["ibis-framework[bigquery]>=12.0.0"] pyspark = ["setuptools", "ibis-framework[pyspark]>=12.0.0"] clickhouse = ["ibis-framework[clickhouse]>=12.0.0"] From 6c86931dacc7290bc79af90757fc39e9c156dcd0 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:01:23 +1000 Subject: [PATCH 11/23] feat(ibis): add UpsertStyle enum + upsert_style field; assign per matrix --- .../backends/ibis/dialects/_registry.py | 23 +++++++++ .../ibis/test_upsert_style_registry.py | 51 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 tests/test_unit/backends/ibis/test_upsert_style_registry.py diff --git a/src/mountainash_data/backends/ibis/dialects/_registry.py b/src/mountainash_data/backends/ibis/dialects/_registry.py index 56738d4..168708a 100644 --- a/src/mountainash_data/backends/ibis/dialects/_registry.py +++ b/src/mountainash_data/backends/ibis/dialects/_registry.py @@ -17,10 +17,17 @@ from __future__ import annotations +import enum from dataclasses import dataclass, field import typing as t +class UpsertStyle(str, enum.Enum): + ON_CONFLICT = "on_conflict" + MERGE = "merge" + ON_DUPLICATE_KEY = "on_duplicate_key" + + # Capability hook signatures GetIndexExistsSql = t.Callable[[str, str, t.Optional[str]], str] # (index_name, table_name, database) -> SQL GetListIndexesSql = t.Callable[[str, t.Optional[str]], str] # (table_name, database) -> SQL @@ -43,6 +50,7 @@ class DialectSpec: get_index_exists_sql: t.Optional[GetIndexExistsSql] = None get_list_indexes_sql: t.Optional[GetListIndexesSql] = None upsert_hook: t.Optional[UpsertHook] = None + upsert_style: t.Optional[UpsertStyle] = None create_index_hook: t.Optional[CreateIndexHook] = None drop_index_hook: t.Optional[DropIndexHook] = None rename_table_hook: t.Optional[RenameTableHook] = None @@ -655,6 +663,7 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: get_index_exists_sql=sqlite_get_index_exists_sql, get_list_indexes_sql=sqlite_get_list_indexes_sql, upsert_hook=duckdb_family_upsert, + upsert_style=UpsertStyle.ON_CONFLICT, create_index_hook=duckdb_family_create_index, drop_index_hook=duckdb_family_drop_index, ), @@ -666,6 +675,7 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: get_index_exists_sql=duckdb_get_index_exists_sql, get_list_indexes_sql=duckdb_get_list_indexes_sql, upsert_hook=duckdb_family_upsert, + upsert_style=UpsertStyle.ON_CONFLICT, create_index_hook=duckdb_family_create_index, drop_index_hook=duckdb_family_drop_index, ), @@ -677,6 +687,7 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: get_index_exists_sql=motherduck_get_index_exists_sql, get_list_indexes_sql=motherduck_get_list_indexes_sql, upsert_hook=duckdb_family_upsert, + upsert_style=UpsertStyle.ON_CONFLICT, create_index_hook=duckdb_family_create_index, drop_index_hook=duckdb_family_drop_index, ), @@ -685,48 +696,56 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: connection_mode=_CONNECTION_STRING, connection_string_scheme="postgres://", connection_builder=_build_postgres_connection, + upsert_style=UpsertStyle.ON_CONFLICT, ), "mysql": DialectSpec( ibis_backend_name="mysql", connection_mode=_CONNECTION_STRING, connection_string_scheme="mysql://", connection_builder=_build_mysql_connection, + upsert_style=UpsertStyle.ON_DUPLICATE_KEY, ), "mssql": DialectSpec( ibis_backend_name="mssql", connection_mode=_CONNECTION_STRING, connection_string_scheme="mssql://", connection_builder=_build_mssql_connection, + upsert_style=UpsertStyle.MERGE, ), "oracle": DialectSpec( ibis_backend_name="oracle", connection_mode=_CONNECTION_STRING, connection_string_scheme="oracle://", connection_builder=_build_oracle_connection, + upsert_style=UpsertStyle.MERGE, ), "snowflake": DialectSpec( ibis_backend_name="snowflake", connection_mode=_HYBRID, # confirmed: snowflake defaults to HYBRID connection_string_scheme="snowflake://", connection_builder=_build_snowflake_connection, + upsert_style=UpsertStyle.MERGE, ), "bigquery": DialectSpec( ibis_backend_name="bigquery", connection_mode=_KWARGS, # confirmed: bigquery defaults to KWARGS connection_string_scheme="bigquery://", connection_builder=_build_bigquery_connection, + upsert_style=UpsertStyle.MERGE, ), "redshift": DialectSpec( ibis_backend_name="postgres", # Redshift uses postgres protocol connection_mode=_CONNECTION_STRING, connection_string_scheme="postgres://", # confirmed: redshift uses postgres:// connection_builder=_build_redshift_connection, + upsert_style=UpsertStyle.MERGE, ), "trino": DialectSpec( ibis_backend_name="trino", connection_mode=_HYBRID, # confirmed: trino defaults to HYBRID connection_string_scheme="trino://", connection_builder=_build_trino_connection, + upsert_style=UpsertStyle.MERGE, ), "clickhouse": DialectSpec( ibis_backend_name="clickhouse", @@ -739,18 +758,21 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: connection_mode=_KWARGS, connection_string_scheme="", connection_builder=_build_databricks_connection, + upsert_style=UpsertStyle.MERGE, ), "singlestoredb": DialectSpec( ibis_backend_name="singlestoredb", connection_mode=_KWARGS, connection_string_scheme="singlestoredb://", connection_builder=_build_singlestoredb_connection, + upsert_style=UpsertStyle.ON_DUPLICATE_KEY, ), "exasol": DialectSpec( ibis_backend_name="exasol", connection_mode=_KWARGS, connection_string_scheme="exasol://", connection_builder=_build_exasol_connection, + upsert_style=UpsertStyle.MERGE, ), "impala": DialectSpec( ibis_backend_name="impala", @@ -769,6 +791,7 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: connection_mode=_KWARGS, connection_string_scheme="risingwave://", connection_builder=_build_risingwave_connection, + upsert_style=UpsertStyle.ON_CONFLICT, ), "druid": DialectSpec( ibis_backend_name="druid", diff --git a/tests/test_unit/backends/ibis/test_upsert_style_registry.py b/tests/test_unit/backends/ibis/test_upsert_style_registry.py new file mode 100644 index 0000000..7ee9889 --- /dev/null +++ b/tests/test_unit/backends/ibis/test_upsert_style_registry.py @@ -0,0 +1,51 @@ +"""The upsert_style assignment must match the spec's §7 coverage matrix.""" + +from mountainash_data.backends.ibis.dialects._registry import ( + DIALECTS, + DialectSpec, + UpsertStyle, +) + +# Spec §7 coverage matrix — the single source of truth for this assertion. +EXPECTED_STYLE = { + "sqlite": UpsertStyle.ON_CONFLICT, + "duckdb": UpsertStyle.ON_CONFLICT, + "motherduck": UpsertStyle.ON_CONFLICT, + "postgres": UpsertStyle.ON_CONFLICT, + "risingwave": UpsertStyle.ON_CONFLICT, + "mysql": UpsertStyle.ON_DUPLICATE_KEY, + "singlestoredb": UpsertStyle.ON_DUPLICATE_KEY, + "snowflake": UpsertStyle.MERGE, + "bigquery": UpsertStyle.MERGE, + "mssql": UpsertStyle.MERGE, + "oracle": UpsertStyle.MERGE, + "databricks": UpsertStyle.MERGE, + "exasol": UpsertStyle.MERGE, + "trino": UpsertStyle.MERGE, + "redshift": UpsertStyle.MERGE, + "clickhouse": None, + "impala": None, + "materialize": None, + "druid": None, + "pyspark": None, +} + + +class TestUpsertStyleField: + def test_field_defaults_none(self): + spec = DialectSpec( + ibis_backend_name="duckdb", + connection_mode="connection_string", + connection_string_scheme="duckdb://", + ) + assert spec.upsert_style is None + + def test_every_registry_dialect_has_an_explicit_decision(self): + # Iterates the live registry — a new dialect with no matrix entry fails. + assert set(DIALECTS) == set(EXPECTED_STYLE), ( + "registry dialects and the §7 matrix have diverged" + ) + + def test_assigned_styles_match_matrix(self): + for name, expected in EXPECTED_STYLE.items(): + assert DIALECTS[name].upsert_style == expected, name From b600dafc4dc7ed9242ee808fd08f362c3b61653e Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:09:15 +1000 Subject: [PATCH 12/23] feat(ibis): generic sqlglot rename_table (works on every dialect) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add build_rename_sql (pure SQL builder via sqlglot exp.Alter/AlterRename) and _generic_rename_table (validates identifiers, calls raw_sql) in operations.py - Wire backend.py rename_table dispatch: hook-or-generic fallback (no more NotImplementedError) - 20-dialect golden test confirms all render correctly (tsql→sp_rename, mysql→RENAME without TO, all others→ALTER TABLE … RENAME TO) - Live postgres rename PASS; mysql SKIP (libmariadb-dev not installed locally) Co-Authored-By: Claude Sonnet 4.6 --- src/mountainash_data/backends/ibis/backend.py | 12 ++-- .../backends/ibis/operations.py | 26 +++++++- tests/test_integration/test_write_ops_live.py | 22 +++++++ .../backends/ibis/test_rename_table_render.py | 63 +++++++++++++++++++ 4 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 tests/test_integration/test_write_ops_live.py create mode 100644 tests/test_unit/backends/ibis/test_rename_table_render.py diff --git a/src/mountainash_data/backends/ibis/backend.py b/src/mountainash_data/backends/ibis/backend.py index 19aa842..0987cb4 100644 --- a/src/mountainash_data/backends/ibis/backend.py +++ b/src/mountainash_data/backends/ibis/backend.py @@ -11,7 +11,7 @@ import typing as t from mountainash_data.backends.ibis.dialects._registry import DIALECTS, DialectSpec -from mountainash_data.backends.ibis.operations import _generic_add_columns +from mountainash_data.backends.ibis.operations import _generic_add_columns, _generic_rename_table from mountainash_data.core.inspection import ( CatalogInfo, NamespaceInfo, @@ -461,12 +461,12 @@ def truncate( return self def rename_table(self, old_name: str, new_name: str) -> IbisBackend: - if self._spec.rename_table_hook is None: - raise NotImplementedError( - f"Dialect {self.dialect!r} does not support rename_table" - ) conn = self._require_connected() - self._spec.rename_table_hook(conn._ibis_conn, old_name, new_name) + hook = self._spec.rename_table_hook + if hook is not None: + hook(conn._ibis_conn, old_name, new_name) + else: + _generic_rename_table(conn._ibis_conn, old_name, new_name) return self # --- Terminal operations (return data) --- diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index f628a3e..0c4b98b 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -19,7 +19,8 @@ CONST_CONFLICT_ACTION, CONST_INDEX_TYPE, ) -from mountainash_data.backends.ibis._render import quote_identifier +from sqlglot import exp +from mountainash_data.backends.ibis._render import dialect_of, quote_identifier # =========================================================================== @@ -135,6 +136,29 @@ def _validate_simple_identifier(value: str, *, kind: str) -> None: ) +def build_rename_sql(old_name: str, new_name: str, *, dialect: t.Any) -> str: + """Pure builder: render a portable rename for an explicit sqlglot dialect. + + sqlglot renders ALTER TABLE … RENAME TO … for most dialects, EXEC sp_rename + for SQL Server (tsql), and ALTER TABLE … RENAME … for MySQL/SingleStore. + Taking `dialect` explicitly lets the registry golden test render every + dialect without a live connection. Identifiers are built directly via + to_identifier(quoted=True) — never pre-quoted-then-reparsed (that double-quotes). + """ + return exp.Alter( + this=exp.Table(this=exp.to_identifier(old_name, quoted=True)), + kind="TABLE", + actions=[exp.AlterRename(this=exp.to_identifier(new_name, quoted=True))], + ).sql(dialect=dialect) + + +def _generic_rename_table(ibis_conn: t.Any, old_name: str, new_name: str) -> None: + """Rename a table via the sqlglot generic default off the live connection.""" + _validate_simple_identifier(old_name, kind="old_name") + _validate_simple_identifier(new_name, kind="new_name") + ibis_conn.raw_sql(build_rename_sql(old_name, new_name, dialect=dialect_of(ibis_conn))) + + def _generic_add_columns( ibis_conn: t.Any, table_name: str, diff --git a/tests/test_integration/test_write_ops_live.py b/tests/test_integration/test_write_ops_live.py new file mode 100644 index 0000000..1e60bea --- /dev/null +++ b/tests/test_integration/test_write_ops_live.py @@ -0,0 +1,22 @@ +"""Live round-trip tests for generic write ops (postgres + mysql).""" + +import polars as pl +import pytest + + +@pytest.mark.integration +def test_rename_table_live_postgres(postgres_backend): + be = postgres_backend + be.create_table("ren_old", pl.DataFrame({"id": [1]}), overwrite=True) + be.rename_table("ren_old", "ren_new") + assert "ren_new" in be.list_tables() + be.drop_table("ren_new", force=True) + + +@pytest.mark.integration +def test_rename_table_live_mysql(mysql_backend): + be = mysql_backend + be.create_table("ren_old", pl.DataFrame({"id": [1]}), overwrite=True) + be.rename_table("ren_old", "ren_new") + assert "ren_new" in be.list_tables() + be.drop_table("ren_new", force=True) diff --git a/tests/test_unit/backends/ibis/test_rename_table_render.py b/tests/test_unit/backends/ibis/test_rename_table_render.py new file mode 100644 index 0000000..586bdba --- /dev/null +++ b/tests/test_unit/backends/ibis/test_rename_table_render.py @@ -0,0 +1,63 @@ +"""rename_table works via the sqlglot generic default on every dialect.""" + +import ibis +import polars as pl +import pytest + +from mountainash_data import IbisBackend +from mountainash_data.backends.ibis.operations import _generic_rename_table, build_rename_sql +from mountainash_data.backends.ibis.dialects._registry import DIALECTS + +# ibis backend name -> sqlglot dialect name (identity unless listed). +# Pinned against sqlglot 30.12.0 by probe: impala/pyspark are unknown to +# sqlglot, mapped to their wire-compatible Hive/Spark base; mssql/singlestoredb +# differ by name. (motherduck/redshift already carry ibis_backend_name +# duckdb/postgres, so they need no entry.) +_IBIS_TO_SQLGLOT = { + "mssql": "tsql", + "motherduck": "duckdb", + "singlestoredb": "singlestore", + "impala": "hive", + "pyspark": "spark", +} + + +class TestGenericRenameTable: + def test_renames_on_duckdb(self): + con = ibis.duckdb.connect() + con.create_table("old", pl.DataFrame({"id": [1]})) + _generic_rename_table(con, "old", "new") + names = con.list_tables() + assert "new" in names and "old" not in names + + def test_renames_on_sqlite(self): + con = ibis.sqlite.connect() + con.create_table("old", pl.DataFrame({"id": [1]})) + _generic_rename_table(con, "old", "new") + assert "new" in con.list_tables() + + def test_rejects_dotted_names(self): + con = ibis.duckdb.connect() + con.create_table("old", pl.DataFrame({"id": [1]})) + with pytest.raises(ValueError, match="simple"): + _generic_rename_table(con, "a.old", "new") + + def test_backend_rename_table_returns_self(self): + with IbisBackend(dialect="duckdb", database=":memory:") as be: + be.create_table("old", pl.DataFrame({"id": [1]})) + assert be.rename_table("old", "new") is be + assert "new" in be.list_tables() + + +class TestRenameGoldenPerDialect: + """Registry-iterating render assertion — every dialect renders a rename.""" + + @pytest.mark.parametrize("name", list(DIALECTS)) + def test_every_dialect_renders_rename(self, name): + d = _IBIS_TO_SQLGLOT.get( + DIALECTS[name].ibis_backend_name, + DIALECTS[name].ibis_backend_name, + ) + sql = build_rename_sql("old", "new", dialect=d) + # tsql renders sp_rename; everyone else an ALTER ... RENAME + assert ("sp_rename" in sql.lower()) or ("rename" in sql.lower()) From 818647e4bafe98949e57252aaa792b5a770facd8 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:15:30 +1000 Subject: [PATCH 13/23] feat(ibis): conditional-predicate compiler (sentinel join->AST->remap) Co-Authored-By: Claude Sonnet 4.6 --- src/mountainash_data/backends/ibis/_render.py | 153 ++++++++++++++++++ .../ibis/test_upsert_condition_render.py | 87 ++++++++++ 2 files changed, 240 insertions(+) create mode 100644 tests/test_unit/backends/ibis/test_upsert_condition_render.py diff --git a/src/mountainash_data/backends/ibis/_render.py b/src/mountainash_data/backends/ibis/_render.py index f400a53..0e2af22 100644 --- a/src/mountainash_data/backends/ibis/_render.py +++ b/src/mountainash_data/backends/ibis/_render.py @@ -6,8 +6,12 @@ from __future__ import annotations +import dataclasses import typing as t +import ibis +import ibis.expr.operations as ops +import ibis.expr.types as ir from sqlglot import exp @@ -29,3 +33,152 @@ def qualified_name(parts: list[str], dialect: t.Any) -> str: def render_type(type_mapper: t.Any, dtype: t.Any) -> str: """Render an ibis dtype to SQL via the connection's type-mapper.""" return type_mapper.to_string(dtype) + + +# --------------------------------------------------------------------------- +# Conditional-predicate compiler (§6.1) +# --------------------------------------------------------------------------- + +INCOMING_SENTINEL = "__ma_incoming__" +EXISTING_SENTINEL = "__ma_existing__" + +# Op classes whose presence makes a predicate invalid in a WHERE/WHEN MATCHED +_FORBIDDEN_OPS = (ops.Reduction, ops.WindowFunction) + +# Subquery/EXISTS op classes — use getattr guard so missing attrs don't crash +# at import time if ibis version changes. All three exist in ibis 12. +_SUBQUERY_OPS: tuple[type, ...] = tuple( + c + for c in ( + getattr(ops, "ExistsSubquery", None), + getattr(ops, "InSubquery", None), + getattr(ops, "ScalarSubquery", None), + ) + if c is not None +) + + +def validate_predicate(expr: ir.BooleanValue) -> None: + """Reject predicates that cannot live in a row-level WHERE/WHEN MATCHED. + + Raises: + ValueError: if `expr` contains an aggregation, window function, or + subquery/EXISTS op. + """ + node = expr.op() + for n in node.find(_FORBIDDEN_OPS): # type: ignore[arg-type] + raise ValueError( + "update_condition must be a scalar row predicate; found " + f"{type(n).__name__} (aggregation/window). Use the upsert_hook " + "override for conditions outside this grammar." + ) + # Detect subqueries/EXISTS by SPECIFIC subquery op types, NOT ops.Relation. + # ops.Relation also matches the two allowed sentinel tables, so testing for + # it would reject every valid predicate (Codex finding). + for n in node.find(_SUBQUERY_OPS): # type: ignore[arg-type] + raise ValueError( + "update_condition may not contain subqueries/EXISTS/third-table " + "references; use the upsert_hook override." + ) + + +@dataclasses.dataclass(frozen=True) +class ConditionAliases: + """How each side's columns are referenced in the rendered clause. + + ``incoming_quoted=False`` is used for the ON CONFLICT ``EXCLUDED`` + pseudo-relation, which must NOT be a quoted identifier (Postgres exposes it + as the special unquoted ``excluded``; quoting it risks referencing the + wrong object). + """ + + incoming: str # e.g. "excluded" (on conflict) or "src" (merge) + existing: str # e.g. "tgt" + incoming_quoted: bool = True + existing_quoted: bool = True + + +def validate_condition( + target_schema: t.Any, + target_name: str, + predicate: t.Callable[[ir.Table, ir.Table], ir.BooleanValue], +) -> None: + """Grammar + sentinel-collision validation only (no rendering). + + Used for the unconditional §10.5 check in ``_generic_upsert``. + + Raises: + ValueError: if *target_name* collides with a reserved sentinel, or if + the predicate contains a forbidden op (aggregation/window/subquery). + """ + if target_name in (INCOMING_SENTINEL, EXISTING_SENTINEL): + raise ValueError( + f"target table name {target_name!r} collides with a reserved sentinel." + ) + incoming = ibis.table(target_schema, name=INCOMING_SENTINEL) + existing = ibis.table(target_schema, name=EXISTING_SENTINEL) + validate_predicate(predicate(incoming, existing)) + + +def compile_condition( + ibis_conn: t.Any, + target_schema: t.Any, + target_name: str, + predicate: t.Callable[[ir.Table, ir.Table], ir.BooleanValue], + *, + aliases: ConditionAliases, +) -> exp.Expression: + """Render an ``(incoming, existing) -> bool`` predicate to a sqlglot ON + sub-AST, remapping incoming/existing columns to *aliases*. + + The mechanism (§6.1): bind two sentinel-named ibis tables, join them on + the predicate, compile to sqlglot, extract the join ``ON`` sub-AST, then + transform each column's table qualifier to the caller's chosen alias. + + Args: + ibis_conn: Live ibis connection whose compiler drives rendering. + target_schema: The ibis schema shared by both sides (e.g. the target + table's schema). + target_name: Real target table name — rejected if it collides with a + reserved sentinel (spec §6.1 step 0). + predicate: ``(incoming_table, existing_table) -> BooleanValue``. + aliases: How to label each side in the rendered SQL. + + Returns: + A sqlglot ``Expression`` representing the ON clause with sentinel + names replaced by *aliases*. + + Raises: + ValueError: on sentinel collision, or forbidden predicate grammar. + """ + validate_condition(target_schema, target_name, predicate) + + incoming = ibis.table(target_schema, name=INCOMING_SENTINEL) + existing = ibis.table(target_schema, name=EXISTING_SENTINEL) + pred = predicate(incoming, existing) + + joined = existing.join(incoming, pred, how="inner") + ast = ibis_conn.compiler.to_sqlglot(joined) + ast = ast if isinstance(ast, exp.Expression) else ast[0] + + # Build sentinel-alias → (target_alias, quoted) map, keyed by whatever + # alias ibis assigned to each sentinel table in the compiled AST. + remap: dict[str, tuple[str, bool]] = {} + for tbl in ast.find_all(exp.Table): + if tbl.name == INCOMING_SENTINEL: + remap[tbl.alias_or_name] = (aliases.incoming, aliases.incoming_quoted) + elif tbl.name == EXISTING_SENTINEL: + remap[tbl.alias_or_name] = (aliases.existing, aliases.existing_quoted) + + join = next(ast.find_all(exp.Join), None) + if join is None or join.args.get("on") is None: + raise ValueError("could not extract join ON predicate from compiled AST") + on = join.args["on"].copy() + + def _remap(n: exp.Expression) -> exp.Expression: + if isinstance(n, exp.Column) and n.table in remap: + alias, quoted = remap[n.table] + n.set("table", exp.to_identifier(alias, quoted=quoted)) + return n + + return on.transform(_remap) diff --git a/tests/test_unit/backends/ibis/test_upsert_condition_render.py b/tests/test_unit/backends/ibis/test_upsert_condition_render.py new file mode 100644 index 0000000..2358f74 --- /dev/null +++ b/tests/test_unit/backends/ibis/test_upsert_condition_render.py @@ -0,0 +1,87 @@ +"""The update_condition ibis-expression predicate compiler (§6.1).""" + +import ibis +import pytest + +from mountainash_data.backends.ibis._render import ( + ConditionAliases, + compile_condition, + dialect_of, + validate_predicate, +) + +_SCHEMA = ibis.schema({"id": "int64", "updated_at": "timestamp", "v": "string"}) + +# ON CONFLICT: incoming is the unquoted `excluded` pseudo-relation; existing is `tgt`. +_ONCONFLICT = ConditionAliases(incoming="excluded", existing="tgt", incoming_quoted=False) +# MERGE: both sides are normal quoted aliases. +_MERGE = ConditionAliases(incoming="src", existing="tgt") + + +def _render(con, predicate, aliases, *, target_name="t"): + ast = compile_condition(con, _SCHEMA, target_name, predicate, aliases=aliases) + return ast.sql(dialect=dialect_of(con)) + + +class TestCompileCondition: + def test_on_conflict_alias_mapping_unquoted_excluded(self): + con = ibis.duckdb.connect() + sql = _render(con, lambda inc, exi: inc.updated_at > exi.updated_at, _ONCONFLICT) + # EXCLUDED is the unquoted pseudo-relation; existing is quoted "tgt" + assert "excluded." in sql.lower() and '"EXCLUDED"' not in sql + assert '"tgt"."updated_at"' in sql + + def test_merge_alias_mapping_duckdb(self): + con = ibis.duckdb.connect() + sql = _render(con, lambda inc, exi: inc.updated_at > exi.updated_at, _MERGE) + assert '"src"."updated_at"' in sql and '"tgt"."updated_at"' in sql + + def test_function_predicate_renders_per_dialect(self): + con = ibis.duckdb.connect() + sql = _render(con, lambda inc, exi: inc.v.upper() != exi.v.upper(), _MERGE) + assert "UPPER(" in sql.upper() + + def test_constant_predicate_renders(self): + con = ibis.duckdb.connect() + sql = _render(con, lambda inc, exi: inc.id > 0, _MERGE) + assert '"src"."id"' in sql + + def test_null_check_predicate_renders(self): + con = ibis.duckdb.connect() + sql = _render(con, lambda inc, exi: inc.v.notnull(), _MERGE) + assert "NULL" in sql.upper() + + def test_rejects_target_name_colliding_with_sentinel(self): + con = ibis.duckdb.connect() + with pytest.raises(ValueError, match="sentinel"): + _render( + con, + lambda inc, exi: inc.id > exi.id, + _MERGE, + target_name="__ma_incoming__", + ) + + def test_rejects_aggregate_predicate(self): + with pytest.raises(ValueError, match="aggregat|window|scalar|subquer|row predicate"): + validate_predicate( + ibis.table(_SCHEMA, name="x").v.count() > 0 # aggregation + ) + + def test_rejects_window_predicate(self): + """WindowFunction op triggers the forbidden-ops check.""" + schema = ibis.schema({"id": "int64", "updated_at": "timestamp", "v": "string"}) + t = ibis.table(schema, name="x") + # row_number() over window produces a WindowFunction op + win_expr = t.id.sum().over(ibis.window()) > 0 + with pytest.raises(ValueError, match="aggregat|window|scalar|subquer|row predicate"): + validate_predicate(win_expr) + + def test_rejects_subquery_predicate(self): + """InSubquery op triggers the subquery rejection check.""" + schema = ibis.schema({"id": "int64", "updated_at": "timestamp", "v": "string"}) + t = ibis.table(schema, name="x") + t2 = ibis.table(schema, name="y") + # .isin(other_table_col) produces InSubquery op + isin_expr = t.id.isin(t2.id) + with pytest.raises(ValueError, match="aggregat|window|scalar|subquer|row predicate"): + validate_predicate(isin_expr) From c3ef8531db018ee8060446052ca8dacade25f503 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:19:01 +1000 Subject: [PATCH 14/23] test(ibis): isolate window-only predicate (rank) + assert alias remap in null-check Applies Task 5 review findings: window-rejection test now uses t.id.rank() (pure WindowFunction, no Reduction) instead of sum().over() which triggered both; null-check test now asserts the "src"."v" alias remap occurred. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../backends/ibis/test_upsert_condition_render.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_unit/backends/ibis/test_upsert_condition_render.py b/tests/test_unit/backends/ibis/test_upsert_condition_render.py index 2358f74..a9cbd1a 100644 --- a/tests/test_unit/backends/ibis/test_upsert_condition_render.py +++ b/tests/test_unit/backends/ibis/test_upsert_condition_render.py @@ -50,6 +50,8 @@ def test_null_check_predicate_renders(self): con = ibis.duckdb.connect() sql = _render(con, lambda inc, exi: inc.v.notnull(), _MERGE) assert "NULL" in sql.upper() + # the alias remap must have applied: the incoming column is qualified by src + assert '"src"."v"' in sql def test_rejects_target_name_colliding_with_sentinel(self): con = ibis.duckdb.connect() @@ -71,8 +73,9 @@ def test_rejects_window_predicate(self): """WindowFunction op triggers the forbidden-ops check.""" schema = ibis.schema({"id": "int64", "updated_at": "timestamp", "v": "string"}) t = ibis.table(schema, name="x") - # row_number() over window produces a WindowFunction op - win_expr = t.id.sum().over(ibis.window()) > 0 + # rank() is an analytic function -> a pure WindowFunction op (no + # Reduction), so this isolates the window arm of the forbidden check. + win_expr = t.id.rank() > 0 with pytest.raises(ValueError, match="aggregat|window|scalar|subquer|row predicate"): validate_predicate(win_expr) From 893c1524f658659094555d6990dcdcb39c690d20 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:25:05 +1000 Subject: [PATCH 15/23] feat(ibis): generic upsert ON CONFLICT branch + compiled-subquery staging Co-Authored-By: Claude Sonnet 4.6 --- src/mountainash_data/backends/ibis/_render.py | 27 +++ .../backends/ibis/operations.py | 210 +++++++++++++++++- .../backends/ibis/test_upsert_render.py | 76 +++++++ 3 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 tests/test_unit/backends/ibis/test_upsert_render.py diff --git a/src/mountainash_data/backends/ibis/_render.py b/src/mountainash_data/backends/ibis/_render.py index 0e2af22..d1444f7 100644 --- a/src/mountainash_data/backends/ibis/_render.py +++ b/src/mountainash_data/backends/ibis/_render.py @@ -120,6 +120,33 @@ def validate_condition( validate_predicate(predicate(incoming, existing)) +def compiled_source( + ibis_conn: t.Any, obj: t.Any, target_schema: t.Any +) -> tuple[str, list[str]]: + """Compile `obj` to a SELECT subquery, casting each column to the target + type and projecting in target-column order. Returns (sql, columns). + + Columns present in the target but absent from the source are omitted; + columns present in the source but absent from the target raise ValueError. + + ``_register_in_memory_tables`` is called before ``compile`` so that + memtable-backed expressions (the common case when `obj` is a DataFrame) + are staged in the backend catalog. Without this step, ``compile`` emits + SQL referencing ``ibis_polars_memtable_`` which is not registered, + causing a CatalogException at ``raw_sql`` time. This matches ibis's own + memtable-staging mechanism (``SQLBackend._register_in_memory_tables``). + """ + src = obj if isinstance(obj, ir.Table) else ibis.memtable(obj) + src_cols = set(src.columns) + extra = src_cols - set(target_schema.names) + if extra: + raise ValueError(f"source columns absent from target: {sorted(extra)}") + cols = [c for c in target_schema.names if c in src_cols] + projected = src.select([src[c].cast(target_schema[c]).name(c) for c in cols]) + ibis_conn._register_in_memory_tables(projected) # REQUIRED: stage memtables + return ibis_conn.compile(projected), cols + + def compile_condition( ibis_conn: t.Any, target_schema: t.Any, diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index 0c4b98b..f4eab53 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -20,7 +20,16 @@ CONST_INDEX_TYPE, ) from sqlglot import exp -from mountainash_data.backends.ibis._render import dialect_of, quote_identifier +from mountainash_data.backends.ibis._render import ( + ConditionAliases, + compile_condition, + compiled_source, + dialect_of, + qualified_name, + quote_identifier, + validate_condition, +) +from mountainash_data.backends.ibis.dialects._registry import UpsertStyle # =========================================================================== @@ -474,3 +483,202 @@ def duckdb_family_upsert( ibis_conn.drop_table(staging_table, force=True) except Exception: pass + + +# =========================================================================== +# GENERIC UPSERT — dialect-agnostic dispatcher +# =========================================================================== + + +def build_on_conflict_sql( + *, + dialect: t.Any, + target: str, + cols: list[str], + conflict: list[str], + update: list[str], + conflict_action: str, + source_sql: str, + condition_sql: str | None = None, +) -> str: + """Pure builder: render an INSERT … ON CONFLICT statement for *dialect*. + + Takes all pre-computed parts explicitly so registry golden tests (Task 7/10) + can render the ON CONFLICT family without a live connection. + + Args: + dialect: sqlglot dialect (from ``dialect_of(ibis_conn)``). + target: Fully-qualified, already-quoted target table reference. + cols: Ordered list of source/insert column names (unquoted). + conflict: Conflict-key column names (unquoted). + update: Columns to update on conflict (unquoted; ignored for NOTHING). + conflict_action: ``"UPDATE"`` or ``"NOTHING"``. + source_sql: Compiled SELECT subquery SQL string (from ``compiled_source``). + condition_sql: Optional rendered WHERE condition for the DO UPDATE clause. + + Returns: + A complete ``INSERT INTO … ON CONFLICT …`` SQL string. + """ + col_list = ", ".join(quote_identifier(c, dialect) for c in cols) + conflict_list = ", ".join(quote_identifier(c, dialect) for c in conflict) + # EXCLUDED is the unquoted pseudo-relation (Postgres/DuckDB/SQLite convention). + excl = "EXCLUDED" + + if conflict_action == "NOTHING": + action = f"ON CONFLICT ({conflict_list}) DO NOTHING" + else: + set_sql = ", ".join( + f"{quote_identifier(c, dialect)} = {excl}.{quote_identifier(c, dialect)}" + for c in update + ) + where = f" WHERE {condition_sql}" if condition_sql else "" + action = f"ON CONFLICT ({conflict_list}) DO UPDATE SET {set_sql}{where}" + + # ``WHERE true`` is required by SQLite to disambiguate INSERT … SELECT … ON + # CONFLICT (its parser errors near DO without it); harmless on duckdb/postgres. + return ( + f"INSERT INTO {target} AS tgt ({col_list}) " + f"SELECT {col_list} FROM ({source_sql}) AS __src WHERE true {action}" + ) + + +def _render_on_conflict( + ibis_conn: t.Any, + name: str, + obj: t.Any, + *, + target_schema: t.Any, + conflict: list[str], + update: list[str], + conflict_action: str, + update_condition: t.Any, + database: str | None, + schema: str | None, +) -> str: + """Thin wrapper: derive dialect/source_sql/condition_sql from the live + connection and delegate to ``build_on_conflict_sql``.""" + dialect = dialect_of(ibis_conn) + source_sql, cols = compiled_source(ibis_conn, obj, target_schema) + parts = [p for p in (database, schema, name) if p] + target = qualified_name(parts, dialect) + + condition_sql: str | None = None + if update_condition is not None and conflict_action == "UPDATE": + # EXCLUDED is the unquoted pseudo-relation for the incoming row; tgt is + # the target alias used in INSERT INTO … AS tgt. + aliases = ConditionAliases( + incoming="EXCLUDED", existing="tgt", incoming_quoted=False + ) + condition_sql = compile_condition( + ibis_conn, target_schema, name, update_condition, aliases=aliases, + ).sql(dialect=dialect) + + return build_on_conflict_sql( + dialect=dialect, + target=target, + cols=cols, + conflict=conflict, + update=update, + conflict_action=conflict_action, + source_sql=source_sql, + condition_sql=condition_sql, + ) + + +def _generic_upsert( + ibis_conn: t.Any, + name: str, + obj: t.Any, + *, + style: t.Any, + conflict_columns: t.Any, + update_columns: t.Any, + conflict_action: str, + update_condition: t.Any, + database: str | None, + schema: str | None, +) -> None: + """Dialect-agnostic upsert dispatcher. + + Validation precedence (spec §10): + 1. style (unknown → NotImplementedError) + 2. target existence + 3. identifier validation (name, database) + 4. conflict_action validity + 5. update_condition — validated UNCONDITIONALLY even under NOTHING + (malformed predicate must error regardless of action path) + 6. updatable columns check + + MERGE and ON_DUPLICATE_KEY raise ``NotImplementedError`` placeholders + (Tasks 7/8 fill them). Public ``be.upsert()`` dispatch is NOT flipped yet + (Task 9); tests call this directly. + """ + # §10.1 — style check first + if style is None: + raise NotImplementedError( + f"Dialect (connection {type(ibis_conn).__name__}) does not support upsert" + ) + + # §10.2 — target existence + _tables = ibis_conn.list_tables(database=database) if database is not None else ibis_conn.list_tables() + if name not in _tables: + raise ValueError(f"target table {name!r} does not exist") + + # §10.3 — identifier validation + _validate_simple_identifier(name, kind="name") + if database is not None: + _validate_simple_identifier(database, kind="database") + + # §10.4 — conflict_action validity + if conflict_action not in ("UPDATE", "NOTHING"): + raise ValueError( + f"conflict_action must be UPDATE or NOTHING, got {conflict_action!r}" + ) + + target_schema = ibis_conn.table(name, database=database).schema() + conflict = _normalize_columns(conflict_columns) + + # conflict column existence + missing = [c for c in conflict if c not in target_schema.names] + if missing: + raise ValueError(f"conflict_columns absent from target: {missing}") + + if update_columns is None: + update = [c for c in target_schema.names if c not in conflict] + else: + update = _normalize_columns(update_columns) + missing_u = [c for c in update if c not in target_schema.names] + if missing_u: + raise ValueError(f"update_columns absent from target: {missing_u}") + + # §10.5 — update_condition validated UNCONDITIONALLY (even under NOTHING) + if update_condition is not None: + if style is UpsertStyle.ON_DUPLICATE_KEY: + raise ValueError( + "update_condition is not supported for the MySQL ON DUPLICATE KEY family" + ) + validate_condition(target_schema, name, update_condition) + if conflict_action == "NOTHING": + warnings.warn("update_condition is ignored when conflict_action='NOTHING'") + + # §10.6 — updatable columns check + if conflict_action == "UPDATE" and not update: + raise ValueError( + "no columns to update; provide update_columns or non-key columns" + ) + + # Dispatch + if style is UpsertStyle.ON_CONFLICT: + stmt = _render_on_conflict( + ibis_conn, name, obj, target_schema=target_schema, conflict=conflict, + update=update, conflict_action=conflict_action, + update_condition=update_condition, database=database, schema=schema, + ) + elif style is UpsertStyle.MERGE: + raise NotImplementedError("unimplemented style: MERGE") # Task 7 + elif style is UpsertStyle.ON_DUPLICATE_KEY: + raise NotImplementedError("unimplemented style: ON_DUPLICATE_KEY") # Task 8 + else: + raise NotImplementedError(f"unknown upsert_style: {style!r}") + + ibis_conn.raw_sql(stmt) diff --git a/tests/test_unit/backends/ibis/test_upsert_render.py b/tests/test_unit/backends/ibis/test_upsert_render.py new file mode 100644 index 0000000..3094ebb --- /dev/null +++ b/tests/test_unit/backends/ibis/test_upsert_render.py @@ -0,0 +1,76 @@ +"""Generic upsert — ON CONFLICT family (sqlite/duckdb).""" + +import ibis +import polars as pl +import pytest + +from mountainash_data.backends.ibis.dialects._registry import UpsertStyle +from mountainash_data.backends.ibis.operations import _generic_upsert + + +def _seed(con): + con.create_table("t", pl.DataFrame({"id": [1, 2], "v": ["a", "b"]})) + con.raw_sql("CREATE UNIQUE INDEX ux_t_id ON t (id)") + + +class TestOnConflictUpsert: + def test_insert_and_update_duckdb(self): + con = ibis.duckdb.connect() + _seed(con) + _generic_upsert( + con, "t", pl.DataFrame({"id": [2, 3], "v": ["B", "c"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns=["id"], + update_columns=None, conflict_action="UPDATE", + update_condition=None, database=None, schema=None, + ) + rows = dict(con.table("t").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "a", 2: "B", 3: "c"} + + def test_do_nothing_duckdb(self): + con = ibis.duckdb.connect() + _seed(con) + _generic_upsert( + con, "t", pl.DataFrame({"id": [2], "v": ["X"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns="id", + update_columns=None, conflict_action="NOTHING", + update_condition=None, database=None, schema=None, + ) + assert con.table("t").filter(ibis._.id == 2).execute()["v"].iloc[0] == "b" + + def test_composite_key_sqlite(self): + con = ibis.sqlite.connect() + con.create_table("t", pl.DataFrame({"a": [1], "b": [1], "v": ["x"]})) + # needs a composite unique index for ON CONFLICT to detect + con.raw_sql("CREATE UNIQUE INDEX ux ON t (a, b)") + _generic_upsert( + con, "t", pl.DataFrame({"a": [1], "b": [1], "v": ["y"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns=["a", "b"], + update_columns=None, conflict_action="UPDATE", + update_condition=None, database=None, schema=None, + ) + assert con.table("t").execute()["v"].iloc[0] == "y" + + def test_conditional_update_only_when_newer_duckdb(self): + con = ibis.duckdb.connect() + con.create_table("t", pl.DataFrame({"id": [1], "ver": [5], "v": ["old"]})) + con.raw_sql("CREATE UNIQUE INDEX ux ON t (id)") + _generic_upsert( + con, "t", pl.DataFrame({"id": [1], "ver": [3], "v": ["stale"]}), + style=UpsertStyle.ON_CONFLICT, conflict_columns=["id"], + update_columns=None, conflict_action="UPDATE", + update_condition=lambda inc, exi: inc.ver > exi.ver, + database=None, schema=None, + ) + # incoming ver(3) is NOT newer than existing(5) -> unchanged + assert con.table("t").execute()["v"].iloc[0] == "old" + + def test_unknown_style_raises_notimplemented(self): + con = ibis.duckdb.connect() + _seed(con) + with pytest.raises(NotImplementedError): + _generic_upsert( + con, "t", pl.DataFrame({"id": [9], "v": ["z"]}), + style=None, conflict_columns=["id"], update_columns=None, + conflict_action="UPDATE", update_condition=None, + database=None, schema=None, + ) From 3bd2553c851ff5755f2de5e8cb6bf6c336f476d8 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:30:54 +1000 Subject: [PATCH 16/23] fix(ibis): validate schema identifier in _generic_upsert; clarify NOTHING condition skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies Task 6 review findings: §10.3 now validates the schema param via _validate_simple_identifier (consistent with name/database); added a comment in _render_on_conflict explaining the intentional condition skip under NOTHING. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/mountainash_data/backends/ibis/operations.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index f4eab53..a9dfa09 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -562,6 +562,10 @@ def _render_on_conflict( parts = [p for p in (database, schema, name) if p] target = qualified_name(parts, dialect) + # update_condition only shapes the DO UPDATE arm; ON CONFLICT … DO NOTHING + # has no WHERE, so the condition is intentionally not compiled here. The + # caller-facing validate/warn for a condition under NOTHING lives in + # _generic_upsert (the entry point); this branch just skips rendering it. condition_sql: str | None = None if update_condition is not None and conflict_action == "UPDATE": # EXCLUDED is the unquoted pseudo-relation for the incoming row; tgt is @@ -624,10 +628,12 @@ def _generic_upsert( if name not in _tables: raise ValueError(f"target table {name!r} does not exist") - # §10.3 — identifier validation + # §10.3 — identifier validation (every part that reaches qualified_name) _validate_simple_identifier(name, kind="name") if database is not None: _validate_simple_identifier(database, kind="database") + if schema is not None: + _validate_simple_identifier(schema, kind="schema") # §10.4 — conflict_action validity if conflict_action not in ("UPDATE", "NOTHING"): From fca343da670e67ee4bc99223df63920362e2eac0 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:35:33 +1000 Subject: [PATCH 17/23] feat(ibis): generic upsert MERGE branch (composite keys + conflict_action) Co-Authored-By: Claude Sonnet 4.6 --- .../backends/ibis/operations.py | 92 ++++++++++++++++++- tests/test_integration/test_write_ops_live.py | 41 +++++++++ .../backends/ibis/test_upsert_render.py | 71 +++++++++++++- 3 files changed, 200 insertions(+), 4 deletions(-) diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index a9dfa09..acef763 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -589,6 +589,92 @@ def _render_on_conflict( ) +def build_merge_sql( + *, + dialect: t.Any, + target: str, + cols: list[str], + conflict: list[str], + update: list[str], + conflict_action: str, + source_sql: str, + condition_sql: str | None = None, +) -> str: + """Pure builder: render a MERGE INTO … statement for *dialect*. + + Takes all pre-computed parts explicitly so registry golden tests can render + any MERGE-family dialect without a live connection. + + Args: + dialect: sqlglot dialect (from ``dialect_of(ibis_conn)``). + target: Fully-qualified, already-quoted target table reference. + cols: Ordered list of source/insert column names (unquoted). + conflict: Conflict-key column names (unquoted). + update: Columns to update on match (unquoted; ignored for NOTHING). + conflict_action: ``"UPDATE"`` or ``"NOTHING"``. + source_sql: Compiled SELECT subquery SQL string (from ``compiled_source``). + condition_sql: Optional rendered condition for the WHEN MATCHED clause. + + Returns: + A complete ``MERGE INTO … USING … ON … WHEN …`` SQL string. + """ + q = lambda c: quote_identifier(c, dialect) # noqa: E731 + on = " AND ".join(f"tgt.{q(c)} = src.{q(c)}" for c in conflict) + not_matched = ( + f"WHEN NOT MATCHED THEN INSERT ({', '.join(q(c) for c in cols)}) " + f"VALUES ({', '.join(f'src.{q(c)}' for c in cols)})" + ) + clauses: list[str] = [] + if conflict_action == "UPDATE": + set_sql = ", ".join(f"{q(c)} = src.{q(c)}" for c in update) + cond = f" AND {condition_sql}" if condition_sql else "" + clauses.append(f"WHEN MATCHED{cond} THEN UPDATE SET {set_sql}") + clauses.append(not_matched) + return ( + f"MERGE INTO {target} AS tgt USING ({source_sql}) AS src " + f"ON {on} " + " ".join(clauses) + ) + + +def _render_merge( + ibis_conn: t.Any, + name: str, + obj: t.Any, + *, + target_schema: t.Any, + conflict: list[str], + update: list[str], + conflict_action: str, + update_condition: t.Any, + database: str | None, + schema: str | None, +) -> str: + """Thin wrapper: derive dialect/source_sql/condition_sql from the live + connection and delegate to ``build_merge_sql``.""" + dialect = dialect_of(ibis_conn) + source_sql, cols = compiled_source(ibis_conn, obj, target_schema) + parts = [p for p in (database, schema, name) if p] + target = qualified_name(parts, dialect) + + condition_sql: str | None = None + if update_condition is not None and conflict_action == "UPDATE": + aliases = ConditionAliases(incoming="src", existing="tgt") + condition_sql = compile_condition( + ibis_conn, target_schema, name, update_condition, aliases=aliases, + ).sql(dialect=dialect) + + return build_merge_sql( + dialect=dialect, + target=target, + cols=cols, + conflict=conflict, + update=update, + conflict_action=conflict_action, + source_sql=source_sql, + condition_sql=condition_sql, + ) + + def _generic_upsert( ibis_conn: t.Any, name: str, @@ -681,7 +767,11 @@ def _generic_upsert( update_condition=update_condition, database=database, schema=schema, ) elif style is UpsertStyle.MERGE: - raise NotImplementedError("unimplemented style: MERGE") # Task 7 + stmt = _render_merge( + ibis_conn, name, obj, target_schema=target_schema, conflict=conflict, + update=update, conflict_action=conflict_action, + update_condition=update_condition, database=database, schema=schema, + ) elif style is UpsertStyle.ON_DUPLICATE_KEY: raise NotImplementedError("unimplemented style: ON_DUPLICATE_KEY") # Task 8 else: diff --git a/tests/test_integration/test_write_ops_live.py b/tests/test_integration/test_write_ops_live.py index 1e60bea..8b9c1ba 100644 --- a/tests/test_integration/test_write_ops_live.py +++ b/tests/test_integration/test_write_ops_live.py @@ -3,6 +3,9 @@ import polars as pl import pytest +from mountainash_data.backends.ibis.dialects._registry import UpsertStyle +from mountainash_data.backends.ibis.operations import _generic_upsert + @pytest.mark.integration def test_rename_table_live_postgres(postgres_backend): @@ -20,3 +23,41 @@ def test_rename_table_live_mysql(mysql_backend): be.rename_table("ren_old", "ren_new") assert "ren_new" in be.list_tables() be.drop_table("ren_new", force=True) + + +@pytest.mark.integration +def test_merge_insert_and_update_postgres(postgres_backend): + """MERGE UPDATE: existing row updated, new row inserted.""" + be = postgres_backend + con = be._require_connected()._ibis_conn + con.raw_sql("DROP TABLE IF EXISTS mrg") + con.create_table("mrg", pl.DataFrame({"id": [1, 2], "v": ["a", "b"]})) + _generic_upsert( + con, "mrg", pl.DataFrame({"id": [2, 3], "v": ["B", "c"]}), + style=UpsertStyle.MERGE, conflict_columns=["id"], update_columns=None, + conflict_action="UPDATE", update_condition=None, database=None, schema=None, + ) + rows = dict( + con.table("mrg").order_by("id").execute()[["id", "v"]].itertuples(index=False) + ) + assert rows == {1: "a", 2: "B", 3: "c"} + con.raw_sql("DROP TABLE mrg") + + +@pytest.mark.integration +def test_merge_nothing_postgres(postgres_backend): + """MERGE NOTHING: existing row NOT updated, new row inserted.""" + be = postgres_backend + con = be._require_connected()._ibis_conn + con.raw_sql("DROP TABLE IF EXISTS mrg_nothing") + con.create_table("mrg_nothing", pl.DataFrame({"id": [1], "v": ["a"]})) + _generic_upsert( + con, "mrg_nothing", pl.DataFrame({"id": [1, 2], "v": ["X", "b"]}), + style=UpsertStyle.MERGE, conflict_columns=["id"], update_columns=None, + conflict_action="NOTHING", update_condition=None, database=None, schema=None, + ) + rows = dict( + con.table("mrg_nothing").order_by("id").execute()[["id", "v"]].itertuples(index=False) + ) + assert rows == {1: "a", 2: "b"}, f"Expected {{1:'a', 2:'b'}}, got {rows}" + con.raw_sql("DROP TABLE mrg_nothing") diff --git a/tests/test_unit/backends/ibis/test_upsert_render.py b/tests/test_unit/backends/ibis/test_upsert_render.py index 3094ebb..1cd8644 100644 --- a/tests/test_unit/backends/ibis/test_upsert_render.py +++ b/tests/test_unit/backends/ibis/test_upsert_render.py @@ -1,11 +1,22 @@ -"""Generic upsert — ON CONFLICT family (sqlite/duckdb).""" +"""Generic upsert — ON CONFLICT family (sqlite/duckdb) + MERGE golden tests.""" import ibis import polars as pl import pytest -from mountainash_data.backends.ibis.dialects._registry import UpsertStyle -from mountainash_data.backends.ibis.operations import _generic_upsert +from mountainash_data.backends.ibis.dialects._registry import DIALECTS, UpsertStyle +from mountainash_data.backends.ibis.operations import _generic_upsert, build_merge_sql + +# ibis backend name -> sqlglot dialect name (identity unless listed). +# Mirrors _IBIS_TO_SQLGLOT in test_rename_table_render.py; kept here to avoid +# a cross-test-module import (tests/ has no top-level __init__.py). +_IBIS_TO_SQLGLOT = { + "mssql": "tsql", + "motherduck": "duckdb", + "singlestoredb": "singlestore", + "impala": "hive", + "pyspark": "spark", +} def _seed(con): @@ -74,3 +85,57 @@ def test_unknown_style_raises_notimplemented(self): conflict_action="UPDATE", update_condition=None, database=None, schema=None, ) + + +def _sqlglot_dialect(spec: object) -> str: + """Map a DialectSpec to its sqlglot dialect name.""" + ibis_name: str = spec.ibis_backend_name # type: ignore[attr-defined] + return _IBIS_TO_SQLGLOT.get(ibis_name, ibis_name) + + +@pytest.mark.parametrize( + "name", + [n for n, s in DIALECTS.items() if s.upsert_style is UpsertStyle.MERGE], +) +def test_merge_golden_per_dialect(name: str) -> None: + """Every MERGE-family dialect renders a valid MERGE INTO statement.""" + d = _sqlglot_dialect(DIALECTS[name]) + sql = build_merge_sql( + dialect=d, + target=f'"{name}"', + cols=["id", "v"], + conflict=["id"], + update=["v"], + conflict_action="UPDATE", + source_sql="SELECT 1 AS id, 'a' AS v", + ) + assert sql.startswith("MERGE INTO"), f"{name}: expected MERGE INTO, got: {sql[:60]}" + assert "WHEN MATCHED THEN UPDATE SET" in sql, f"{name}: missing WHEN MATCHED: {sql}" + assert "WHEN NOT MATCHED THEN INSERT" in sql, f"{name}: missing WHEN NOT MATCHED: {sql}" + # backtick quoting is used by mysql, bigquery, and databricks dialects + _BACKTICK_DIALECTS = {"mysql", "bigquery", "databricks"} + uses_backtick = "`" in sql + assert uses_backtick == (d in _BACKTICK_DIALECTS), ( + f"{name} (dialect={d}): unexpected quoting style in: {sql}" + ) + + +@pytest.mark.parametrize( + "name", + [n for n, s in DIALECTS.items() if s.upsert_style is UpsertStyle.MERGE], +) +def test_merge_golden_nothing_omits_matched(name: str) -> None: + """MERGE with conflict_action=NOTHING omits the WHEN MATCHED clause.""" + d = _sqlglot_dialect(DIALECTS[name]) + sql = build_merge_sql( + dialect=d, + target=f'"{name}"', + cols=["id", "v"], + conflict=["id"], + update=["v"], + conflict_action="NOTHING", + source_sql="SELECT 1 AS id, 'a' AS v", + ) + assert sql.startswith("MERGE INTO"), f"{name}: expected MERGE INTO" + assert "WHEN MATCHED" not in sql, f"{name}: NOTHING should omit WHEN MATCHED: {sql}" + assert "WHEN NOT MATCHED THEN INSERT" in sql, f"{name}: missing WHEN NOT MATCHED" From 5a7f687d3b630cc0006f55046bda9ec190e9343e Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:39:38 +1000 Subject: [PATCH 18/23] test(ibis): drop dead mysql member from MERGE backtick set; enable local ibis[mysql] Task 7 review fix: mysql is ON_DUPLICATE_KEY (never in the MERGE-only parametrization), so it is removed from _BACKTICK_DIALECTS. Also adds ibis-framework[mysql] to the local test env (libmariadb-dev now installed) so MySQL live tests run locally, not just in CI. Co-Authored-By: Claude Opus 4.8 (1M context) --- hatch.toml | 3 ++- tests/test_unit/backends/ibis/test_upsert_render.py | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/hatch.toml b/hatch.toml index dc10e56..d6ce865 100644 --- a/hatch.toml +++ b/hatch.toml @@ -129,9 +129,10 @@ dependencies = [ # ibis 12 uses psycopg (psycopg3) for postgres — needs psycopg-binary for # a pure-Python/bundled-libpq build that works without system libpq-dev. # mysql/mariadb (ibis-framework[mysql]) requires mysqlclient which needs - # libmariadb-dev system headers — installed via apt in CI, skipped locally. + # libmariadb-dev system headers — installed via apt in CI and locally. "psycopg-binary>=3.1.0", "ibis-framework[postgres]>=12.0.0", + "ibis-framework[mysql]>=12.0.0", "mountainash_settings @ {root:uri}/../mountainash-settings", "mountainash @ {root:uri}/../mountainash", diff --git a/tests/test_unit/backends/ibis/test_upsert_render.py b/tests/test_unit/backends/ibis/test_upsert_render.py index 1cd8644..71563c0 100644 --- a/tests/test_unit/backends/ibis/test_upsert_render.py +++ b/tests/test_unit/backends/ibis/test_upsert_render.py @@ -112,8 +112,10 @@ def test_merge_golden_per_dialect(name: str) -> None: assert sql.startswith("MERGE INTO"), f"{name}: expected MERGE INTO, got: {sql[:60]}" assert "WHEN MATCHED THEN UPDATE SET" in sql, f"{name}: missing WHEN MATCHED: {sql}" assert "WHEN NOT MATCHED THEN INSERT" in sql, f"{name}: missing WHEN NOT MATCHED: {sql}" - # backtick quoting is used by mysql, bigquery, and databricks dialects - _BACKTICK_DIALECTS = {"mysql", "bigquery", "databricks"} + # backtick-quoting MERGE-family dialects. mysql also backticks but is + # ON_DUPLICATE_KEY (never reaches this MERGE-only parametrization), so it + # is intentionally not listed here. + _BACKTICK_DIALECTS = {"bigquery", "databricks"} uses_backtick = "`" in sql assert uses_backtick == (d in _BACKTICK_DIALECTS), ( f"{name} (dialect={d}): unexpected quoting style in: {sql}" From 3bc46affe5fd7b746a9878a022a9f8d0bf19181e Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:45:06 +1000 Subject: [PATCH 19/23] feat(ibis): generic upsert ON DUPLICATE KEY + MySQL prove-safe preflight Co-Authored-By: Claude Sonnet 4.6 --- .../backends/ibis/operations.py | 165 +++++++++++++++++- .../test_upsert_mysql_preflight.py | 70 ++++++++ .../backends/ibis/test_upsert_render.py | 51 +++++- 3 files changed, 284 insertions(+), 2 deletions(-) create mode 100644 tests/test_integration/test_upsert_mysql_preflight.py diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index acef763..310b3a4 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -675,6 +675,165 @@ def _render_merge( ) +def _mysql_validate_conflict_key( + ibis_conn: t.Any, + name: str, + conflict: list[str], + database: str | None, +) -> None: + """Prove the safe MySQL/MariaDB ON DUPLICATE KEY case or raise (spec §6.2). + + Fails closed on: no unique index, >1 unique index, prefix index (SUB_PART), + functional/expression index (COLUMN_NAME IS NULL), a unique index whose + ORDERED columns don't exactly equal conflict_columns, or any nullable + conflict column. + + NOTE: do NOT select EXPRESSION from information_schema.STATISTICS — that + column is MySQL-8-only; on MariaDB 12.x it errors ``Unknown column + 'EXPRESSION'``. Functional/expression index parts have a NULL COLUMN_NAME + on both MariaDB and MySQL 8; detect them that way instead. + + NOTE: ``ibis_conn.current_database`` is a PROPERTY in ibis >=12 (no parens). + """ + db = database or ibis_conn.current_database + rows = ibis_conn.raw_sql( + "SELECT INDEX_NAME, SEQ_IN_INDEX, COLUMN_NAME, SUB_PART, NON_UNIQUE " + "FROM information_schema.STATISTICS " + f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}' " + "ORDER BY INDEX_NAME, SEQ_IN_INDEX" + ).fetchall() + uniques: dict[str, list[tuple[t.Any, t.Any]]] = {} + for index_name, _seq, column_name, sub_part, non_unique in rows: + if int(non_unique) == 0: + uniques.setdefault(index_name, []).append((column_name, sub_part)) + if not uniques: + raise ValueError(f"table {name!r} has no unique/PK index for conflict_columns") + if len(uniques) > 1: + raise ValueError( + f"table {name!r} has multiple unique indexes {list(uniques)}; MySQL " + f"ON DUPLICATE KEY detects on any of them — ambiguous for " + f"conflict_columns={conflict}. Use the upsert_hook override." + ) + (idx_name, parts), = uniques.items() + if any(col is None for col, _ in parts): # NULL COLUMN_NAME = functional/expression part + raise ValueError( + f"unique index {idx_name!r} is a functional/expression index; cannot " + f"prove it matches conflict_columns={conflict}. Use the upsert_hook override." + ) + if any(sub is not None for _, sub in parts): + raise ValueError( + f"unique index {idx_name!r} has a prefix (SUB_PART); it detects on a " + f"truncated value, not the full column. Use the upsert_hook override." + ) + if [c for c, _ in parts] != list(conflict): + raise ValueError( + f"unique index {idx_name!r} columns {[c for c, _ in parts]} do not " + f"exactly match conflict_columns={list(conflict)}; refusing to guess. " + f"Use the upsert_hook override." + ) + # nullable check + cols_meta = ibis_conn.raw_sql( + "SELECT COLUMN_NAME, IS_NULLABLE FROM information_schema.COLUMNS " + f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}'" + ).fetchall() + nullable = {c for c, isn in cols_meta if isn == "YES"} + bad = [c for c in conflict if c in nullable] + if bad: + raise ValueError( + f"conflict columns {bad} are nullable; MySQL unique indexes are " + f"NULL-distinct, so duplicates would insert instead of update. Make " + f"them NOT NULL or use the upsert_hook override." + ) + + +def build_on_duplicate_key_sql( + *, + dialect: t.Any, + target: str, + cols: list[str], + conflict: list[str], + update: list[str], + conflict_action: str, + source_sql: str, +) -> str: + """Pure builder: render an INSERT … ON DUPLICATE KEY UPDATE statement. + + Takes all pre-computed parts explicitly so registry golden tests can render + the ON_DUPLICATE_KEY family without a live connection. + + Args: + dialect: sqlglot dialect (from ``dialect_of(ibis_conn)``). + target: Fully-qualified, already-quoted target table reference. + cols: Ordered list of source/insert column names (unquoted). + conflict: Conflict-key column names (unquoted). Used only for the + NOTHING self-assign no-op (first conflict column). + update: Columns to update on duplicate (unquoted; ignored for NOTHING). + conflict_action: ``"UPDATE"`` or ``"NOTHING"``. + source_sql: Compiled SELECT subquery SQL string (from ``compiled_source``). + + Returns: + A complete ``INSERT INTO … ON DUPLICATE KEY UPDATE …`` SQL string. + + Note: + ``VALUES(col)`` is valid on the MariaDB 12.x target. MySQL 8.0.20+ + deprecates ``VALUES()`` in favour of a row alias; that switch is + out-of-scope for the MariaDB-tested target here. + + For ``conflict_action="NOTHING"`` the self-assign ``k0 = k0`` is used + as a documented no-op (it is not a true no-op on MySQL — the row is + still "touched" — but it suppresses the update semantics with minimal + side-effects, per spec §6.2). + """ + q = lambda c: quote_identifier(c, dialect) # noqa: E731 + col_list = ", ".join(q(c) for c in cols) + + if conflict_action == "NOTHING": + k0 = q(conflict[0]) + set_sql = f"{k0} = {k0}" # self-assign; see §6.2 (not a true no-op) + else: + set_sql = ", ".join(f"{q(c)} = VALUES({q(c)})" for c in update) + + return ( + f"INSERT INTO {target} ({col_list}) SELECT {col_list} FROM ({source_sql}) AS __src " + f"ON DUPLICATE KEY UPDATE {set_sql}" + ) + + +def _render_on_duplicate_key( + ibis_conn: t.Any, + name: str, + obj: t.Any, + *, + target_schema: t.Any, + conflict: list[str], + update: list[str], + conflict_action: str, + update_condition: t.Any, + database: str | None, + schema: str | None, +) -> str: + """Thin wrapper: run the MySQL prove-safe preflight, then render the SQL. + + Delegates SQL construction to ``build_on_duplicate_key_sql`` so the pure + builder is testable without a live MySQL connection. + """ + _mysql_validate_conflict_key(ibis_conn, name, conflict, database) + dialect = dialect_of(ibis_conn) + source_sql, cols = compiled_source(ibis_conn, obj, target_schema) + parts = [p for p in (database, schema, name) if p] + target = qualified_name(parts, dialect) + + return build_on_duplicate_key_sql( + dialect=dialect, + target=target, + cols=cols, + conflict=conflict, + update=update, + conflict_action=conflict_action, + source_sql=source_sql, + ) + + def _generic_upsert( ibis_conn: t.Any, name: str, @@ -773,7 +932,11 @@ def _generic_upsert( update_condition=update_condition, database=database, schema=schema, ) elif style is UpsertStyle.ON_DUPLICATE_KEY: - raise NotImplementedError("unimplemented style: ON_DUPLICATE_KEY") # Task 8 + stmt = _render_on_duplicate_key( + ibis_conn, name, obj, target_schema=target_schema, conflict=conflict, + update=update, conflict_action=conflict_action, + update_condition=update_condition, database=database, schema=schema, + ) else: raise NotImplementedError(f"unknown upsert_style: {style!r}") diff --git a/tests/test_integration/test_upsert_mysql_preflight.py b/tests/test_integration/test_upsert_mysql_preflight.py new file mode 100644 index 0000000..a25a39c --- /dev/null +++ b/tests/test_integration/test_upsert_mysql_preflight.py @@ -0,0 +1,70 @@ +"""MySQL ON DUPLICATE KEY preflight: prove-safe-or-raise (spec §6.2). + +Calls `_generic_upsert(...)` DIRECTLY against the raw mariadb connection — the +`be.upsert()` dispatch is not flipped until Task 9, so testing the generic +function directly is what keeps this task self-contained (Codex finding). +""" + +import polars as pl +import pytest + +from mountainash_data.backends.ibis.dialects._registry import UpsertStyle +from mountainash_data.backends.ibis.operations import _generic_upsert + + +def _raw(be): + # The fixture yields a connected IbisBackend; reach its raw ibis conn. + return be._require_connected()._ibis_conn + + +def _odk(con, name, df, conflict): + _generic_upsert( + con, name, df, style=UpsertStyle.ON_DUPLICATE_KEY, + conflict_columns=conflict, update_columns=None, conflict_action="UPDATE", + update_condition=None, database=None, schema=None, + ) + + +@pytest.mark.integration +def test_single_pk_proceeds(mysql_backend): + con = _raw(mysql_backend) + con.raw_sql("DROP TABLE IF EXISTS odk_ok") + con.raw_sql("CREATE TABLE odk_ok (id INT PRIMARY KEY, v VARCHAR(16) NOT NULL)") + con.raw_sql("INSERT INTO odk_ok VALUES (1, 'a')") + _odk(con, "odk_ok", pl.DataFrame({"id": [1, 2], "v": ["A", "b"]}), ["id"]) + rows = dict(con.table("odk_ok").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "A", 2: "b"} + con.raw_sql("DROP TABLE odk_ok") + + +@pytest.mark.integration +def test_multiple_unique_raises(mysql_backend): + con = _raw(mysql_backend) + con.raw_sql("DROP TABLE IF EXISTS odk_multi") + con.raw_sql( + "CREATE TABLE odk_multi " + "(id INT PRIMARY KEY, email VARCHAR(64) NOT NULL UNIQUE, v VARCHAR(16) NOT NULL)" + ) + with pytest.raises(ValueError, match="unique"): + _odk(con, "odk_multi", pl.DataFrame({"id": [1], "email": ["x"], "v": ["a"]}), ["id"]) + con.raw_sql("DROP TABLE odk_multi") + + +@pytest.mark.integration +def test_prefix_index_raises(mysql_backend): + con = _raw(mysql_backend) + con.raw_sql("DROP TABLE IF EXISTS odk_prefix") + con.raw_sql("CREATE TABLE odk_prefix (email VARCHAR(64) NOT NULL, v VARCHAR(16) NOT NULL, UNIQUE (email(10)))") + with pytest.raises(ValueError, match="prefix|SUB_PART"): + _odk(con, "odk_prefix", pl.DataFrame({"email": ["x"], "v": ["a"]}), ["email"]) + con.raw_sql("DROP TABLE odk_prefix") + + +@pytest.mark.integration +def test_nullable_conflict_column_raises(mysql_backend): + con = _raw(mysql_backend) + con.raw_sql("DROP TABLE IF EXISTS odk_null") + con.raw_sql("CREATE TABLE odk_null (k INT NULL UNIQUE, v VARCHAR(16) NOT NULL)") + with pytest.raises(ValueError, match="nullable|NOT NULL"): + _odk(con, "odk_null", pl.DataFrame({"k": [1], "v": ["a"]}), ["k"]) + con.raw_sql("DROP TABLE odk_null") diff --git a/tests/test_unit/backends/ibis/test_upsert_render.py b/tests/test_unit/backends/ibis/test_upsert_render.py index 71563c0..51bed21 100644 --- a/tests/test_unit/backends/ibis/test_upsert_render.py +++ b/tests/test_unit/backends/ibis/test_upsert_render.py @@ -5,7 +5,11 @@ import pytest from mountainash_data.backends.ibis.dialects._registry import DIALECTS, UpsertStyle -from mountainash_data.backends.ibis.operations import _generic_upsert, build_merge_sql +from mountainash_data.backends.ibis.operations import ( + _generic_upsert, + build_merge_sql, + build_on_duplicate_key_sql, +) # ibis backend name -> sqlglot dialect name (identity unless listed). # Mirrors _IBIS_TO_SQLGLOT in test_rename_table_render.py; kept here to avoid @@ -141,3 +145,48 @@ def test_merge_golden_nothing_omits_matched(name: str) -> None: assert sql.startswith("MERGE INTO"), f"{name}: expected MERGE INTO" assert "WHEN MATCHED" not in sql, f"{name}: NOTHING should omit WHEN MATCHED: {sql}" assert "WHEN NOT MATCHED THEN INSERT" in sql, f"{name}: missing WHEN NOT MATCHED" + + +# --------------------------------------------------------------------------- +# ON DUPLICATE KEY golden tests (pure builder — no live MySQL required) +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "name", + [n for n, s in DIALECTS.items() if s.upsert_style is UpsertStyle.ON_DUPLICATE_KEY], +) +def test_on_duplicate_key_golden_update(name: str) -> None: + """Every ON_DUPLICATE_KEY dialect renders INSERT … ON DUPLICATE KEY UPDATE … VALUES(…).""" + d = _sqlglot_dialect(DIALECTS[name]) + sql = build_on_duplicate_key_sql( + dialect=d, + target=f"`{name}`", + cols=["id", "v"], + conflict=["id"], + update=["v"], + conflict_action="UPDATE", + source_sql="SELECT 1 AS id, 'a' AS v", + ) + assert "ON DUPLICATE KEY UPDATE" in sql, f"{name}: missing ON DUPLICATE KEY UPDATE: {sql}" + assert "VALUES(" in sql, f"{name}: missing VALUES(: {sql}" + + +@pytest.mark.parametrize( + "name", + [n for n, s in DIALECTS.items() if s.upsert_style is UpsertStyle.ON_DUPLICATE_KEY], +) +def test_on_duplicate_key_golden_nothing(name: str) -> None: + """ON_DUPLICATE_KEY with conflict_action=NOTHING uses self-assign no-op.""" + d = _sqlglot_dialect(DIALECTS[name]) + sql = build_on_duplicate_key_sql( + dialect=d, + target=f"`{name}`", + cols=["id", "v"], + conflict=["id"], + update=["v"], + conflict_action="NOTHING", + source_sql="SELECT 1 AS id, 'a' AS v", + ) + assert "ON DUPLICATE KEY UPDATE" in sql, f"{name}: missing ON DUPLICATE KEY UPDATE: {sql}" + # NOTHING uses self-assign: the first conflict column appears twice with = + assert "VALUES(" not in sql, f"{name}: NOTHING should not use VALUES(): {sql}" From 850b8d2f941552a667f353189a679e5d8d4cf38a Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:49:15 +1000 Subject: [PATCH 20/23] fix(ibis): harden mysql preflight against injection; assert ODK self-assign MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 8 review fixes: _mysql_validate_conflict_key now re-validates name/database via _validate_simple_identifier (defense-in-depth — it interpolates them into the introspection SQL; the public path validates upstream but a direct caller would not). NOTHING golden now asserts the `id` = `id` self-assign (guards against a future empty-SET regression, which is a syntax error). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/mountainash_data/backends/ibis/operations.py | 6 ++++++ tests/test_unit/backends/ibis/test_upsert_render.py | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index 310b3a4..4007a70 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -695,6 +695,12 @@ def _mysql_validate_conflict_key( NOTE: ``ibis_conn.current_database`` is a PROPERTY in ibis >=12 (no parens). """ + # Defense-in-depth: name/database are validated in _generic_upsert, but this + # function interpolates them into the introspection SQL, so re-validate here + # to stay safe for any direct caller (name is the upstream guard's contract). + _validate_simple_identifier(name, kind="name") + if database is not None: + _validate_simple_identifier(database, kind="database") db = database or ibis_conn.current_database rows = ibis_conn.raw_sql( "SELECT INDEX_NAME, SEQ_IN_INDEX, COLUMN_NAME, SUB_PART, NON_UNIQUE " diff --git a/tests/test_unit/backends/ibis/test_upsert_render.py b/tests/test_unit/backends/ibis/test_upsert_render.py index 51bed21..8ef287e 100644 --- a/tests/test_unit/backends/ibis/test_upsert_render.py +++ b/tests/test_unit/backends/ibis/test_upsert_render.py @@ -188,5 +188,7 @@ def test_on_duplicate_key_golden_nothing(name: str) -> None: source_sql="SELECT 1 AS id, 'a' AS v", ) assert "ON DUPLICATE KEY UPDATE" in sql, f"{name}: missing ON DUPLICATE KEY UPDATE: {sql}" - # NOTHING uses self-assign: the first conflict column appears twice with = + # NOTHING uses self-assign on the first conflict column (NOT an empty SET, + # which is a syntax error). Both ODK dialects (mysql/singlestore) backtick. + assert "`id` = `id`" in sql, f"{name}: NOTHING should self-assign `id` = `id`: {sql}" assert "VALUES(" not in sql, f"{name}: NOTHING should not use VALUES(): {sql}" From c8e6da3997664ff4af64cb16e254c71557204914 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:54:41 +1000 Subject: [PATCH 21/23] feat(ibis): cutover upsert to generic dispatch; retire duckdb_family_upsert - IbisBackend.upsert() now dispatches hook-or-generic: calls hook if registered, otherwise falls through to _generic_upsert with dialect's upsert_style. - Retype update_condition: str | None -> t.Any = None (ConditionPredicate | None). - Remove upsert_hook=duckdb_family_upsert from sqlite/duckdb/motherduck registry entries; all three carry upsert_style=ON_CONFLICT so they route through the generic renderer. - Delete duckdb_family_upsert from operations.py; remove now-unused imports (uuid, mountainash/ma, CONST_CONFLICT_ACTION). - Add live be.upsert() dispatch round-trips: postgres (ON_CONFLICT) + mysql (ON_DUPLICATE_KEY). - Update tests: test_duckdb_dialect_routes_generic_upsert (hook retired, style asserted); test_upsert_unsupported_dialect_raises now uses clickhouse (no style/hook). Co-Authored-By: Claude Sonnet 4.6 --- src/mountainash_data/backends/ibis/backend.py | 38 ++++--- .../backends/ibis/dialects/_registry.py | 4 - .../backends/ibis/operations.py | 99 ++----------------- tests/test_integration/test_write_ops_live.py | 26 +++++ tests/test_unit/backends/ibis/test_backend.py | 21 ++-- 5 files changed, 69 insertions(+), 119 deletions(-) diff --git a/src/mountainash_data/backends/ibis/backend.py b/src/mountainash_data/backends/ibis/backend.py index 0987cb4..e8acb35 100644 --- a/src/mountainash_data/backends/ibis/backend.py +++ b/src/mountainash_data/backends/ibis/backend.py @@ -11,7 +11,7 @@ import typing as t from mountainash_data.backends.ibis.dialects._registry import DIALECTS, DialectSpec -from mountainash_data.backends.ibis.operations import _generic_add_columns, _generic_rename_table +from mountainash_data.backends.ibis.operations import _generic_add_columns, _generic_rename_table, _generic_upsert from mountainash_data.core.inspection import ( CatalogInfo, NamespaceInfo, @@ -524,24 +524,32 @@ def upsert( conflict_columns: list[str] | str, update_columns: list[str] | str | None = None, conflict_action: str = "UPDATE", - update_condition: str | None = None, + update_condition: t.Any = None, # ConditionPredicate | None database: str | None = None, schema: str | None = None, ) -> IbisBackend: - if self._spec.upsert_hook is None: - raise NotImplementedError( - f"Dialect {self.dialect!r} does not support upsert" - ) conn = self._require_connected() - self._spec.upsert_hook( - conn._ibis_conn, name, obj, - conflict_columns=conflict_columns, - update_columns=update_columns, - conflict_action=conflict_action, - update_condition=update_condition, - database=database, - schema=schema, - ) + hook = self._spec.upsert_hook + if hook is not None: + hook( + conn._ibis_conn, name, obj, + conflict_columns=conflict_columns, + update_columns=update_columns, + conflict_action=conflict_action, + update_condition=update_condition, + database=database, + schema=schema, + ) + else: + _generic_upsert( + conn._ibis_conn, name, obj, style=self._spec.upsert_style, + conflict_columns=conflict_columns, + update_columns=update_columns, + conflict_action=conflict_action, + update_condition=update_condition, + database=database, + schema=schema, + ) return self def add_columns( diff --git a/src/mountainash_data/backends/ibis/dialects/_registry.py b/src/mountainash_data/backends/ibis/dialects/_registry.py index 168708a..de2cd2d 100644 --- a/src/mountainash_data/backends/ibis/dialects/_registry.py +++ b/src/mountainash_data/backends/ibis/dialects/_registry.py @@ -648,7 +648,6 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: sqlite_get_list_indexes_sql, motherduck_get_index_exists_sql, motherduck_get_list_indexes_sql, - duckdb_family_upsert, duckdb_family_create_index, duckdb_family_drop_index, ) @@ -662,7 +661,6 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: connection_builder=_build_sqlite_connection, get_index_exists_sql=sqlite_get_index_exists_sql, get_list_indexes_sql=sqlite_get_list_indexes_sql, - upsert_hook=duckdb_family_upsert, upsert_style=UpsertStyle.ON_CONFLICT, create_index_hook=duckdb_family_create_index, drop_index_hook=duckdb_family_drop_index, @@ -674,7 +672,6 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: connection_builder=_build_duckdb_connection, get_index_exists_sql=duckdb_get_index_exists_sql, get_list_indexes_sql=duckdb_get_list_indexes_sql, - upsert_hook=duckdb_family_upsert, upsert_style=UpsertStyle.ON_CONFLICT, create_index_hook=duckdb_family_create_index, drop_index_hook=duckdb_family_drop_index, @@ -686,7 +683,6 @@ def _build_pyspark_connection(**config: t.Any) -> t.Any: connection_builder=_build_motherduck_connection, get_index_exists_sql=motherduck_get_index_exists_sql, get_list_indexes_sql=motherduck_get_list_indexes_sql, - upsert_hook=duckdb_family_upsert, upsert_style=UpsertStyle.ON_CONFLICT, create_index_hook=duckdb_family_create_index, drop_index_hook=duckdb_family_drop_index, diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index 4007a70..2f1303f 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -3,20 +3,17 @@ Contains: - Module-level helper functions: _generate_index_name, _format_qualified_table, _normalize_columns - Per-dialect SQL functions: duckdb, sqlite, motherduck index SQL generators -- Standalone hook functions: duckdb_family_create_index, duckdb_family_drop_index, - duckdb_family_upsert +- Standalone hook functions: duckdb_family_create_index, duckdb_family_drop_index +- Generic dispatcher: _generic_upsert (dialect-agnostic, replaces duckdb_family_upsert) """ import typing as t import contextlib import warnings -import uuid import ibis -import mountainash as ma from mountainash_data.core.constants import ( - CONST_CONFLICT_ACTION, CONST_INDEX_TYPE, ) from sqlglot import exp @@ -400,91 +397,6 @@ def duckdb_family_drop_index( cur.execute(drop_sql) -def duckdb_family_upsert( - ibis_conn: t.Any, - table_name: str, - df: t.Any, - *, - conflict_columns: list[str] | str, - update_columns: list[str] | str | None = None, - conflict_action: str = CONST_CONFLICT_ACTION.UPDATE, - update_condition: str | None = None, - database: str | None = None, - schema: str | None = None, -) -> None: - """Perform upsert using INSERT ... ON CONFLICT syntax (DuckDB/SQLite).""" - conflict_cols = _normalize_columns(conflict_columns) - all_columns = ma.relation(df).columns - - if update_columns is None: - update_cols = [col for col in all_columns if col not in conflict_cols] - else: - update_cols = _normalize_columns(update_columns) - - tables = ibis_conn.list_tables() - if table_name not in tables: - raise ValueError(f"Target table '{table_name}' does not exist") - - if conflict_action not in [CONST_CONFLICT_ACTION.UPDATE, CONST_CONFLICT_ACTION.NOTHING]: - raise ValueError( - f"conflict_action must be '{CONST_CONFLICT_ACTION.UPDATE}' or " - f"'{CONST_CONFLICT_ACTION.NOTHING}', got '{conflict_action}'" - ) - - if conflict_action == CONST_CONFLICT_ACTION.NOTHING: - if update_cols or update_condition: - warnings.warn( - "update_columns and update_condition are ignored when " - "conflict_action='NOTHING'" - ) - - staging_table = f"temp_upsert_{uuid.uuid4().hex[:8]}" - qualified_table = _format_qualified_table(table_name, database=database, schema=schema) - - all_cols_sql = ", ".join(all_columns) - conflict_cols_sql = ", ".join(conflict_cols) - - if conflict_action == CONST_CONFLICT_ACTION.UPDATE: - if not update_cols: - raise ValueError( - "No columns to update. Either provide update_columns or ensure " - "dataframe has columns beyond conflict_columns" - ) - update_set_sql = ", ".join([f"{col} = EXCLUDED.{col}" for col in update_cols]) - where_sql = f" WHERE {update_condition}" if update_condition else "" - on_conflict_sql = ( - f"ON CONFLICT ({conflict_cols_sql}) DO UPDATE SET {update_set_sql}{where_sql}" - ) - else: - on_conflict_sql = f"ON CONFLICT ({conflict_cols_sql}) DO NOTHING" - - upsert_sql = f""" - INSERT INTO {qualified_table} ({all_cols_sql}) - SELECT {all_cols_sql} FROM {staging_table} - WHERE true - {on_conflict_sql} - """ - - if hasattr(ibis_conn.con, 'register'): - with contextlib.closing(ibis_conn.con.cursor()) as cur: - cur.execute("BEGIN TRANSACTION") - cur.register(staging_table, df) - cur.execute(upsert_sql) - cur.unregister(staging_table) - cur.execute("COMMIT") - else: - ibis_conn.create_table(staging_table, df, temp=True, overwrite=True) - try: - with contextlib.closing(ibis_conn.con.cursor()) as cur: - cur.execute(upsert_sql) - ibis_conn.con.commit() - finally: - try: - ibis_conn.drop_table(staging_table, force=True) - except Exception: - pass - - # =========================================================================== # GENERIC UPSERT — dialect-agnostic dispatcher # =========================================================================== @@ -864,9 +776,10 @@ def _generic_upsert( (malformed predicate must error regardless of action path) 6. updatable columns check - MERGE and ON_DUPLICATE_KEY raise ``NotImplementedError`` placeholders - (Tasks 7/8 fill them). Public ``be.upsert()`` dispatch is NOT flipped yet - (Task 9); tests call this directly. + Covers all three SQL families: ON_CONFLICT (DuckDB/SQLite/Postgres/RisingWave), + MERGE (MSSQL/Oracle/Snowflake/BigQuery/Redshift/Trino/Databricks/Exasol), + ON_DUPLICATE_KEY (MySQL/SingleStoreDB). Public ``be.upsert()`` dispatches here + when no dialect hook is registered. """ # §10.1 — style check first if style is None: diff --git a/tests/test_integration/test_write_ops_live.py b/tests/test_integration/test_write_ops_live.py index 8b9c1ba..9d8caca 100644 --- a/tests/test_integration/test_write_ops_live.py +++ b/tests/test_integration/test_write_ops_live.py @@ -61,3 +61,29 @@ def test_merge_nothing_postgres(postgres_backend): ) assert rows == {1: "a", 2: "b"}, f"Expected {{1:'a', 2:'b'}}, got {rows}" con.raw_sql("DROP TABLE mrg_nothing") + + +@pytest.mark.integration +def test_upsert_via_dispatch_postgres(postgres_backend): + """be.upsert() public dispatch — ON_CONFLICT via generic path (postgres).""" + be = postgres_backend + be.create_table("up_pg", pl.DataFrame({"id": [1, 2], "v": ["a", "b"]}), overwrite=True) + be._require_connected()._ibis_conn.raw_sql("ALTER TABLE up_pg ADD PRIMARY KEY (id)") + be.upsert("up_pg", pl.DataFrame({"id": [2, 3], "v": ["B", "c"]}), conflict_columns=["id"]) + rows = dict(be.table("up_pg").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "a", 2: "B", 3: "c"} + be.drop_table("up_pg", force=True) + + +@pytest.mark.integration +def test_upsert_via_dispatch_mysql(mysql_backend): + """be.upsert() public dispatch — ON_DUPLICATE_KEY via generic path (mysql/mariadb).""" + be = mysql_backend + con = be._require_connected()._ibis_conn + con.raw_sql("DROP TABLE IF EXISTS up_my") + con.raw_sql("CREATE TABLE up_my (id INT PRIMARY KEY, v VARCHAR(16) NOT NULL)") + con.raw_sql("INSERT INTO up_my VALUES (1, 'a')") + be.upsert("up_my", pl.DataFrame({"id": [1, 2], "v": ["A", "b"]}), conflict_columns=["id"]) + rows = dict(con.table("up_my").order_by("id").execute()[["id", "v"]].itertuples(index=False)) + assert rows == {1: "A", 2: "b"} + con.raw_sql("DROP TABLE up_my") diff --git a/tests/test_unit/backends/ibis/test_backend.py b/tests/test_unit/backends/ibis/test_backend.py index cfe9bf7..1e4f191 100644 --- a/tests/test_unit/backends/ibis/test_backend.py +++ b/tests/test_unit/backends/ibis/test_backend.py @@ -218,10 +218,12 @@ def test_get_connection_accessor(): # DialectSpec hooks # --------------------------------------------------------------------------- -def test_duckdb_dialect_has_upsert_hook(): - """DuckDB DialectSpec must have upsert_hook wired.""" +def test_duckdb_dialect_routes_generic_upsert(): + """DuckDB DialectSpec has no hook (retired) but has upsert_style=ON_CONFLICT for generic path.""" + from mountainash_data.backends.ibis.dialects._registry import UpsertStyle spec = DIALECTS["duckdb"] - assert spec.upsert_hook is not None + assert spec.upsert_hook is None + assert spec.upsert_style == UpsertStyle.ON_CONFLICT def test_sqlite_dialect_has_create_index_hook(): @@ -369,10 +371,15 @@ def test_upsert_duckdb(): def test_upsert_unsupported_dialect_raises(): - """upsert() on a dialect without upsert_hook must raise NotImplementedError.""" - backend = IbisBackend(dialect="postgres") - # Can't actually connect to postgres, so mock the connection state + """upsert() on a dialect with no upsert_style and no hook must raise NotImplementedError. + + clickhouse has neither upsert_style nor upsert_hook, so _generic_upsert receives + style=None and raises NotImplementedError — the correct sentinel for unsupported dialects. + postgres now has upsert_style=ON_CONFLICT so it routes through _generic_upsert successfully. + """ + backend = IbisBackend(dialect="clickhouse") + # Can't actually connect, so mock the connection state from mountainash_data.backends.ibis.backend import IbisConnection - backend._conn = IbisConnection(None, DIALECTS["postgres"]) + backend._conn = IbisConnection(None, DIALECTS["clickhouse"]) with pytest.raises(NotImplementedError, match="does not support upsert"): backend.upsert("t", {}, conflict_columns=["id"]) From 49362d8921a99e491fc3ae6b7e75d05ad4225644 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 00:57:31 +1000 Subject: [PATCH 22/23] docs(ibis): annotate upsert_style=None as the unsupported sentinel Task 9 review Minor: make explicit that a DialectSpec with no upsert_style (and no upsert_hook) means upsert is unsupported -> NotImplementedError. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/mountainash_data/backends/ibis/dialects/_registry.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/mountainash_data/backends/ibis/dialects/_registry.py b/src/mountainash_data/backends/ibis/dialects/_registry.py index de2cd2d..bca0004 100644 --- a/src/mountainash_data/backends/ibis/dialects/_registry.py +++ b/src/mountainash_data/backends/ibis/dialects/_registry.py @@ -50,6 +50,7 @@ class DialectSpec: get_index_exists_sql: t.Optional[GetIndexExistsSql] = None get_list_indexes_sql: t.Optional[GetListIndexesSql] = None upsert_hook: t.Optional[UpsertHook] = None + # None = upsert not supported (no hook + no style -> NotImplementedError). upsert_style: t.Optional[UpsertStyle] = None create_index_hook: t.Optional[CreateIndexHook] = None drop_index_hook: t.Optional[DropIndexHook] = None From 66348efb31639a7824278466b4bcd7d29a54e7b6 Mon Sep 17 00:00:00 2001 From: Nathaniel Ramm Date: Tue, 30 Jun 2026 10:11:07 +1000 Subject: [PATCH 23/23] fix(ibis): close MySQL-preflight injection surface (allowlist + escaped literals) Final whole-branch review finding. _validate_simple_identifier now enforces a charset allowlist ([A-Za-z_][A-Za-z0-9_$]*) instead of only rejecting dots, so quote/semicolon/whitespace-bearing names can't reach the preflight's information_schema string-literal interpolation. As defense in depth, the preflight also renders name/database via sqlglot exp.Literal.string(). Adds 16 covering tests (injection payloads -> ValueError; safe names accepted). Also: align the ON CONFLICT condition test alias to production casing ("EXCLUDED"); refresh the operations.py module docstring (drop the retired duckdb_family_upsert reference). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../backends/ibis/operations.py | 51 ++++++++++++++----- .../ibis/test_upsert_condition_render.py | 5 +- .../backends/ibis/test_upsert_render.py | 31 +++++++++++ 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/src/mountainash_data/backends/ibis/operations.py b/src/mountainash_data/backends/ibis/operations.py index 2f1303f..b21319e 100644 --- a/src/mountainash_data/backends/ibis/operations.py +++ b/src/mountainash_data/backends/ibis/operations.py @@ -4,11 +4,13 @@ - Module-level helper functions: _generate_index_name, _format_qualified_table, _normalize_columns - Per-dialect SQL functions: duckdb, sqlite, motherduck index SQL generators - Standalone hook functions: duckdb_family_create_index, duckdb_family_drop_index -- Generic dispatcher: _generic_upsert (dialect-agnostic, replaces duckdb_family_upsert) +- Generic, dialect-agnostic write ops: _generic_rename_table, _generic_add_columns, + _generic_upsert (with the three upsert-family renderers + MySQL preflight) """ import typing as t import contextlib +import re import warnings import ibis @@ -128,17 +130,32 @@ def _normalize_to_schema(source: t.Any) -> ibis.Schema: return ibis.memtable(source).schema() -def _validate_simple_identifier(value: str, *, kind: str) -> None: - """Reject dotted/multi-part names — only simple identifiers are supported. +_SIMPLE_IDENTIFIER_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_$]*\Z") + - A dotted ``table_name``/``database`` would otherwise be quoted as a single - literal identifier (``"a.b"``) rather than a namespace, silently violating - the documented contract. Fail loudly instead. +def _validate_simple_identifier(value: str, *, kind: str) -> None: + """Require a simple, safe SQL identifier: ``[A-Za-z_][A-Za-z0-9_$]*``. + + Two guarantees in one check: + + 1. **Namespace correctness** — a dotted ``table_name``/``database`` would be + quoted as a single literal identifier (``"a.b"``) rather than a + namespace, silently violating the documented contract. + 2. **Injection safety** — the MySQL/MariaDB preflight builds + ``information_schema`` queries by interpolating ``name``/``database`` into + SQL string literals. Restricting them to this charset (no quotes, + semicolons, whitespace, or other metacharacters) means a hostile or + malformed identifier cannot break out of that literal context. The + preflight ALSO renders the literals via sqlglot as defense in depth, but + this validator is the primary gate. + + Anything outside the charset fails loudly instead of emitting unsafe SQL. """ - if "." in value: + if not _SIMPLE_IDENTIFIER_RE.match(value): raise ValueError( - f"{kind} {value!r} must be a simple (non-dotted) identifier; " - f"multi-part qualified names are out of scope." + f"{kind} {value!r} must be a simple identifier (letters, digits, " + f"underscore, $; starting with a letter or underscore); dotted, " + f"quoted, or whitespace-bearing names are out of scope." ) @@ -607,17 +624,23 @@ def _mysql_validate_conflict_key( NOTE: ``ibis_conn.current_database`` is a PROPERTY in ibis >=12 (no parens). """ - # Defense-in-depth: name/database are validated in _generic_upsert, but this - # function interpolates them into the introspection SQL, so re-validate here - # to stay safe for any direct caller (name is the upstream guard's contract). + # Primary gate: name/database must be simple identifiers (charset-allowlisted + # by _validate_simple_identifier). _generic_upsert validates them upstream; + # re-validate here so a direct caller is equally safe. _validate_simple_identifier(name, kind="name") if database is not None: _validate_simple_identifier(database, kind="database") db = database or ibis_conn.current_database + # Defense in depth: these values go into SQL *string literals*, so render + # them as escaped literals via sqlglot rather than bare f-string interpolation + # (belt-and-suspenders behind the allowlist above). + dialect = dialect_of(ibis_conn) + name_lit = exp.Literal.string(name).sql(dialect=dialect) + db_lit = exp.Literal.string(db).sql(dialect=dialect) rows = ibis_conn.raw_sql( "SELECT INDEX_NAME, SEQ_IN_INDEX, COLUMN_NAME, SUB_PART, NON_UNIQUE " "FROM information_schema.STATISTICS " - f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}' " + f"WHERE TABLE_SCHEMA = {db_lit} AND TABLE_NAME = {name_lit} " "ORDER BY INDEX_NAME, SEQ_IN_INDEX" ).fetchall() uniques: dict[str, list[tuple[t.Any, t.Any]]] = {} @@ -652,7 +675,7 @@ def _mysql_validate_conflict_key( # nullable check cols_meta = ibis_conn.raw_sql( "SELECT COLUMN_NAME, IS_NULLABLE FROM information_schema.COLUMNS " - f"WHERE TABLE_SCHEMA = '{db}' AND TABLE_NAME = '{name}'" + f"WHERE TABLE_SCHEMA = {db_lit} AND TABLE_NAME = {name_lit}" ).fetchall() nullable = {c for c, isn in cols_meta if isn == "YES"} bad = [c for c in conflict if c in nullable] diff --git a/tests/test_unit/backends/ibis/test_upsert_condition_render.py b/tests/test_unit/backends/ibis/test_upsert_condition_render.py index a9cbd1a..1ea4077 100644 --- a/tests/test_unit/backends/ibis/test_upsert_condition_render.py +++ b/tests/test_unit/backends/ibis/test_upsert_condition_render.py @@ -12,8 +12,9 @@ _SCHEMA = ibis.schema({"id": "int64", "updated_at": "timestamp", "v": "string"}) -# ON CONFLICT: incoming is the unquoted `excluded` pseudo-relation; existing is `tgt`. -_ONCONFLICT = ConditionAliases(incoming="excluded", existing="tgt", incoming_quoted=False) +# ON CONFLICT: incoming is the unquoted EXCLUDED pseudo-relation; existing is `tgt`. +# Use the exact casing production passes (_render_on_conflict uses "EXCLUDED"). +_ONCONFLICT = ConditionAliases(incoming="EXCLUDED", existing="tgt", incoming_quoted=False) # MERGE: both sides are normal quoted aliases. _MERGE = ConditionAliases(incoming="src", existing="tgt") diff --git a/tests/test_unit/backends/ibis/test_upsert_render.py b/tests/test_unit/backends/ibis/test_upsert_render.py index 8ef287e..69f776d 100644 --- a/tests/test_unit/backends/ibis/test_upsert_render.py +++ b/tests/test_unit/backends/ibis/test_upsert_render.py @@ -7,6 +7,7 @@ from mountainash_data.backends.ibis.dialects._registry import DIALECTS, UpsertStyle from mountainash_data.backends.ibis.operations import ( _generic_upsert, + _validate_simple_identifier, build_merge_sql, build_on_duplicate_key_sql, ) @@ -192,3 +193,33 @@ def test_on_duplicate_key_golden_nothing(name: str) -> None: # which is a syntax error). Both ODK dialects (mysql/singlestore) backtick. assert "`id` = `id`" in sql, f"{name}: NOTHING should self-assign `id` = `id`: {sql}" assert "VALUES(" not in sql, f"{name}: NOTHING should not use VALUES(): {sql}" + + +class TestIdentifierValidationHardening: + """_validate_simple_identifier is the primary gate against SQL injection in + the MySQL preflight's string-literal interpolation (final-review finding).""" + + @pytest.mark.parametrize( + "bad", + [ + "x'y", # single quote — would break out of a literal + "x' OR '1'='1", # classic injection payload + "a.b", # dotted (namespace) — still rejected + "a b", # whitespace + "tbl;DROP TABLE x", # statement separator + "tbl--", # comment + "1abc", # leading digit + "tbl`name", # backtick + 'tbl"name', # double quote + "", # empty + ], + ) + def test_rejects_unsafe_identifiers(self, bad: str) -> None: + with pytest.raises(ValueError, match="simple identifier"): + _validate_simple_identifier(bad, kind="name") + + @pytest.mark.parametrize( + "good", ["users", "_private", "T1", "wearables_events", "col$x", "a1_b2"] + ) + def test_accepts_safe_identifiers(self, good: str) -> None: + _validate_simple_identifier(good, kind="name") # no raise