fix(engine): complete dry-run mirror delete previews on Snowflake/ClickHouse/Databricks (#1044) - #1060
Conversation
…ckHouse/Databricks (#1044) fetch_tracked_state()/fetch_all_keys() previously covered only Postgres/MySQL despite all five SQL destinations supporting both mirror strategies since v0.8.4. Each warehouse now reads through its own connection/state-table/identifier/parameter conventions -- the real DELETE/finalize paths are untouched, this is read-only. Databricks also gains QueryableDestination (get_table_name/ execute_test_query), needed to reach true-diff mode instead of sample mode -- a genuine, minimal, additive capability gap it had before. A failed mirror-only read no longer collapses to a successful zero-delete preview: DiffResult.delete_preview_unavailable_reason distinguishes "read failed" from "read succeeded, zero deletions", surfaced in both text ("preview unavailable") and JSON output. Closes #1044.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…n in mirror preview Codex review caught two real defects in the new #1044 preview code: 1. _fetch_all_keys_databricks built one query with all scope values, exceeding Databricks' 255-native-parameter-marker limit above 255 single-column (or 255/n composite) scope values. Now chunks via the same _rows_per_chunk() every other Databricks write path uses. 2. _fetch_all_keys_clickhouse selected raw-typed key columns, but the real ClickHouse mirror DELETE (_build_mirror_delete) compares keys as strings on both sides specifically because a typed column (e.g. UUID) doesn't compare equal to the source's plain value. The SELECT now wraps key columns in toString() to match, and _preview_destination_mirror_deletes compares dest/source keys by their string form (safe for all dialects -- a no-op when both sides already round-trip the same Python type). Filed #1061 for a third, pre-existing finding (tracked-mirror preview ignores mirror.scope) -- confirmed on origin/main today, unrelated to this PR, affects Postgres/MySQL too since #693 shipped.
|
Review found 3 issues, all verified: Fixed:
Filed as a follow-up, not fixed here — #1061: the tracked-mirror preview (`_preview_tracked_mirror_deletes`) ignores `mirror.scope` entirely. Verified this exists on `origin/main` today, unchanged by this PR — it's been live since #693 shipped tracked+scope, affecting Postgres/MySQL already. This PR only changed how `fetch_tracked_state()` is populated (adding 3 dialects); it didn't touch the scope-filtering logic, which was already absent. Fixing a pre-existing, dialect-agnostic bug is out of scope for a PR about adding dialect coverage. Re-verified: 112 targeted tests pass, full suite 3,680 passed / 39 skipped (same 2 pre-existing unrelated failures), ruff/mypy clean. |
…ollapse, and scope Decimal fix (#1044) Second Codex review pass on #1060 found three more real issues: 1. _fetch_rows_by_keys_databricks (the primary add/update keyed lookup, separate function from the mirror-scope reader already fixed) used the generic default batch_size=1000, unbounded by Databricks' 255 native-parameter limit. Now capped via _rows_per_chunk, same as every other Databricks write path. 2. Enabling Databricks' QueryableDestination surfaced a pre-existing gap: compute_diff() passes columns=[] for the replace-mode full scan, and dict(zip([], row)) collapses every row to {} -- silently understating deletions. Fixed for Databricks by deriving columns from cursor.description. Same gap exists on Postgres/MySQL/ Snowflake/ClickHouse (pre-existing, unrelated to this PR) -- filed as #1062. 3. My own previous fix (comparing dest/source keys by string form) was too broad -- coercing every dialect this way can turn two natively-equal values into a false mismatch. Narrowed to ClickHouse only, where it's actually needed to match toString(). Also filed #1061 for a third pre-existing finding (tracked-mirror preview ignores mirror.scope, confirmed live on Postgres/MySQL since #693, unrelated to this PR).
|
Third review pass found three more, all verified and fixed:
Also re-confirmed the fourth finding (tracked-mirror preview ignoring `mirror.scope`) is the same pre-existing issue already filed as #1061 — not re-fixing here for the reasons already given. Re-verified: 115 targeted tests pass, full suite 3,683 passed / 39 skipped (same 2 pre-existing unrelated failures), ruff/mypy clean. |
Closes #1044.
Problem
drt run --dry-run --diff's mirror DELETE preview (fetch_tracked_state()/fetch_all_keys()indrt/destinations/query.py) only had Postgres/MySQL branches, despite all five SQL destinations (Postgres, MySQL, Snowflake, ClickHouse, Databricks) supporting both mirror strategies since v0.8.4. A failed/unsupported read also silently collapsed to the same empty list as "genuinely nothing to delete" — indistinguishable to the operator.Fix
fetch_tracked_state()andfetch_all_keys()now have real Snowflake/ClickHouse/Databricks branches — each is a plain read-onlySELECT, independent of how each dialect's actual DELETE/mirror-finalize statement is phrased (ClickHouse'sALTER TABLE ... DELETEmutation, Databricks' staged-key anti-join). The real mirror finalize/DELETE code paths are untouched.QueryableDestination(get_table_name/execute_test_query) — needed to reach true-diff mode instead of sample mode, and a genuine pre-existing capability gap closed as a side effect (this also meansdrt test's custom SQL query feature now works on Databricks for the first time).DiffResult.delete_preview_unavailable_reason(new field) distinguishes a failed/unsupported read from a successful zero-delete read. Text output:- Deleted (mirror DELETE): preview unavailable+ the error. JSON:nullon success (including zero deletions), an error string otherwise.Verification
pytest tests/unit/test_destination_query.py tests/unit/test_diff.py tests/unit/test_cli_diff.py— 109 passed.test_cli_version.py, directory-depth artifact of this checkout).ruff check drt tests/mypy drtclean.docs/guides/dry-run-and-diff.md) updated; stale "Snowflake needs feat(snowflake): add lookups + query support (enable --diff for Snowflake destination) #468" / "refactor: introduce Destination.fetch_existing() Protocol method (replace hardcoded _QUERYABLE_TYPES) #469 hardcoded types" follow-up notes removed since both are now resolved.🤖 Generated with Claude Code