DQX Studio: Support for Lakebase storage backend, declarative storage with destroy protection, observer-aligned error/warning metrics#1173
Merged
Conversation
… label filtering, error/warning metrics Storage: move schemas, wheels volume, and Lakebase instance + logical database into databricks.yml with lifecycle.prevent_destroy; replace bootstrap_storage.sh with 'make app-bind' for adopting existing resources in workspaces created by the prior flow. Backend: introduce PgExecutor + Postgres migration runner for OLTP tables (rules, settings, RBAC, comments, schedules, scheduler bookkeeping); keep analytical tables (validation runs, profiling, quarantine, metrics) on Delta. Metrics: persist DQX observer's error_row_count / warning_row_count / input_row_count via dq_validation_runs.error_rows / warning_rows and fix Spark Connect Observation.get mutability bug that was overwriting total_rows with limit-pushed values. UI: rename 'Invalid' -> 'Errors', add 'Warnings' column to quarantine detail, replace 'Has invalid' filter with 'Has failures', surface label badges next to table names in rule selection and schedule editor, add label filtering to Execute Rules and Schedule settings. Migrations: fix FIELD_ALREADY_EXISTS idempotency in MigrationRunner so v3/v4/v5 ADD COLUMN migrations no-op on fresh deploys whose v1 baseline already includes warning_rows / warnings / error_rows. Misc: retention test coverage, custom metrics service, post-deploy grants script reuse, docs rewrite (DEPLOYMENT.md, README.md, CLAUDE.md, installation.mdx) for the new declarative storage model.
Contributor
|
All commits in PR should be signed ('git commit -S ...'). See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
…, drop database_catalogs
The database_catalogs DAB resource is the only one that creates a logical Postgres database, but it also creates a Unity Catalog catalog as a side effect and therefore requires CREATE CATALOG on the metastore — a permission most app deployers don't hold. Drop it. Connect the app to the always-present databricks_postgres admin database instead; per-app isolation comes from the dedicated dqx_studio Postgres schema the app creates inside it on first start. The bundle stays fully declarative with no out-of-band bootstrap steps.
- databricks.yml: remove database_catalogs.lakebase_db and the lakebase_uc_catalog_name variable. Default lakebase_database_name to 'databricks_postgres'. App's database: binding now references ${var.lakebase_database_name} directly.
- Makefile: add BUNDLE_VARS forwarding to 'make app-deploy' so one-off CLI overrides (e.g. lakebase_instance_name during Lakebase's 7-day soft-delete name retention) don't require ad-hoc databricks.yml edits.
- bind_resources.sh: pass --auto-approve explicitly (newer databricks CLI versions reject piped 'yes' confirmation). Drop the lakebase_db bind step (no longer a bundle resource).
- Documentation updates across DEPLOYMENT.md, CLAUDE.md files, and installation.mdx to reflect the new layout: no separate logical-DB provisioning step, uninstall drops a Postgres schema instead of a database.
berrybluecode
force-pushed
the
feat/app-refactor-backend-add-lakebase
branch
from
9b3136b to
1f6e53d
Compare
laurencewells
left a comment
laurencewells
left a comment
Contributor
There was a problem hiding this comment.
Review
Large, well-structured PR. The architectural split (Delta for append/analytical tables, Lakebase for OLTP) is sound, prevent_destroy guards are the right default, and the documentation is thorough. A few issues below — two of them (partial migration commits, service-layer dialect branching) worth addressing before merge.
Summary of items:
OltpExecutortype alias is a dead string — not a real type- Dialect branching is leaking into service code — the abstraction isn't holding
PgExecutor.executecommits per statement — migrations can leave partial committed state- No unit tests for
PgExecutororPgMigrationRunner cast(SqlExecutor, pg_executor)— no structural check enforces the parity contractbind_resources.shswallows bind failures as warnings instead of errors- Schema rename (
dqx_app→dqx_studio) needs a one-liner in the migration guide for users with existing data
Contributor
|
✅ 721/721 passed, 39 skipped, 5h31m42s total Running from acceptance #4683 |
make fmt rewrites GitHub source URLs to match the version in __about__.py. CI merges this branch onto main (now at v0.14.0) and the lingering v0.13.0 refs added on this branch trip git diff --exit-code. Bump them to v0.14.0 explicitly.
The previous OltpExecutor = "SqlExecutor | PgExecutor" was just a string assignment, not a type alias — mypy/pyright treated it as str. Switch to Union["SqlExecutor", "PgExecutor"] (string forward refs because PgExecutor is TYPE_CHECKING-only) and use OltpExecutor | None for _pg_executor and the get/set helpers, so type checkers actually enforce the parity contract between the two executors.
Contributor
|
✅ 182/182 passed, 1 flaky, 6h0m23s total Flaky tests:
Running from anomaly #797 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1173 +/- ##
==========================================
- Coverage 92.08% 92.06% -0.02%
==========================================
Files 101 101
Lines 9546 9546
==========================================
- Hits 8790 8789 -1
- Misses 756 757 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…hardening, plus PgMigrationRunner unit tests and CI fixes
…nal:databrickslabs/dqx into feat/app-refactor-backend-add-lakebase
…es, resilient token refresh, statement timeouts
* OLTP executor protocol: collapse duplicated
`if getattr(self._sql, "dialect", "delta") == "postgres":`
branches across services into `OltpExecutorProtocol` (upsert,
JSON projection, interval syntax). Promote `_Executor` from
`migrations/postgres.py` to the shared protocol; services now
depend on the protocol, not on Delta/Postgres implementations.
Type aliases use PEP 604 `|` throughout.
* pg_executor: introduce `run_trusted_sql(cur, sql)` that
centralises the `cast(LiteralString, sql)` for backend-composed
SQL, removing the per-call
`# pyright: ignore[reportCallIssue, reportArgumentType]`
suppressions. `migrations/postgres.py` routes through it too.
* Token refresh daemon: replace the hard-coded 60s sleep with a
short jittered back-off, expose `last_successful_refresh_at`
and `consecutive_refresh_failures` as observability properties,
and escalate via `os._exit(1)` after
`lakebase_token_refresh_max_failures` consecutive failures so
the supervisor restarts before the pool's 50-minute max_lifetime
recycles the last valid connection. New `AppConfig` knobs:
`lakebase_token_refresh_retry_seconds`,
`lakebase_token_refresh_retry_jitter`,
`lakebase_token_refresh_max_failures`.
* Statement timeouts: `timeout_seconds` was declared but ignored.
Add `_apply_statement_timeout()` which issues
`SET LOCAL statement_timeout = <ms>` per statement, mirroring
the deadline contract `SqlExecutor` enforces via warehouse
polling. Prevents a runaway Postgres query from blocking a pool
connection indefinitely.
* app_settings_service: drop `Installation._unmarshal_type(...)`
(private blueprint API) for a typed Pydantic `TypeAdapter`.
* app.py: replace `globals()["_scheduler_lock_fd"] = fd` with an
explicit module-level `_scheduler_lock_fd: int | None = None`
so the type-checker and casual readers can see the lifetime.
* Linter policy: document the project-wide BLE001 exemption in
`pyproject.toml` (broad-except is the contract for lifespan
teardown, daemon resilience, and per-item route wrappers), and
add `tests/test_lint_policy.py` to prevent regression — fails
CI if anyone adds per-line `# noqa: BLE001` or per-call
`# pyright: ignore` on `cursor.execute`.
* scripts/post_deploy_grants.sh: guard `${EXTRA_VARS[@]}` and
`${BUNDLE_FLAGS[@]}` with the `${arr[@]+...}` idiom so the
script survives macOS bash 3.2 + `set -u` when called without
`--var` overrides or `-t`.
* New tests: `test_pg_executor.py` (back-off, jitter, escalation,
metrics, statement-timeout enforcement), `test_lint_policy.py`
(BLE001 + pyright-ignore policy), `test_migration_runner.py`,
`test_app_scheduler_lease.py`. Existing test files updated for
new call counts and helpers.
basedpyright on CI flagged 8 `reportReturnType` errors in `OltpExecutorProtocol`: methods like `catalog`, `schema`, `fqn`, `q`, `json_literal_expr`, `ts_text`, `query`, and `query_dicts` declared non-`None` return types but had only a docstring as the body. A bare-docstring function implicitly returns `None`, so stricter basedpyright reads the signature as a return-type lie. Add an explicit `...` after each docstring (matching the existing `select_json_text` / `interval_days_expr` stubs). The `None`- returning protocol methods (`execute`, `execute_no_schema`, `upsert`, `upsert_with_audit`) are intentionally left as docstring-only — their implicit `None` return matches the signature. CI-only because the local basedpyright is older and treats a docstring as a sufficient stub body; pinning it via the project config is a separate follow-up.
CI integration: every `TestRouterIntegration.*` test (14/14) was
failing with `ModuleNotFoundError: No module named 'psycopg'`
on first run. Root cause is the import chain triggered when the
`api_client` fixture does `from ...backend.app import app`:
app.py
→ from .migrations.postgres import PgMigrationRunner (top-level, was always loaded)
→ migrations/postgres.py: from ..pg_executor import run_trusted_sql (top-level)
→ pg_executor.py: from psycopg import Connection, Cursor (top-level)
→ ModuleNotFoundError on the dqx-library test environment, which
does not install the Lakebase-only `psycopg` extra.
The companion symbol `build_pg_executor` already follows the right
pattern: imported lazily inside the `if conf.lakebase_enabled:`
branch in `lifespan()`. Apply the same to `PgMigrationRunner` —
both Postgres-only call sites are guarded by the same condition.
Net effect:
* Delta-only environments (incl. the dqx-library integration
test environment) can `from ...backend.app import app` without
needing `psycopg` on the path.
* The Lakebase code path is unchanged: when
`conf.lakebase_enabled` is true the lazy imports run, the
runner/executor build, and migrations apply exactly as before.
Verified locally by simulating the test environment via
`sys.meta_path` blocking `psycopg` / `psycopg_pool` before
importing `app` — the import now succeeds. `make app-check`
clean, `make app-test` 571/571 passing.
Architectural note: `migrations/postgres.py` still imports
`pg_executor` at module level, so any future consumer that
imports `migrations.postgres` directly will still need `psycopg`.
Fixing that requires pushing `run_trusted_sql` into method bodies
or moving its definition out of `pg_executor`; deferred since the
only current consumer (this file) is now correctly gated.
mwojtyczka
requested changes
May 29, 2026
…QL injection contract, escalation coverage Sequential follow-ups to the must-fix review batch in 8cabac9. Each change closes a specific reviewer concern; tests are added where the fix is structural so regressions fail loudly rather than silently. backend/pg_cursor_helpers.py (new) — psycopg-free trust-boundary helpers Splits ``run_trusted_sql`` / ``run_parameterized_sql`` out of ``pg_executor``. ``Cursor`` is imported only under ``TYPE_CHECKING``, so ``migrations.postgres`` (which only needs the helpers) is now importable without psycopg installed — verified with a meta-path import-blocker. Lets ``app.py`` drop the ``PgMigrationRunner`` lazy- import workaround entirely; the remaining lazy ``build_pg_executor`` import is the one that genuinely needs psycopg at runtime. backend/pg_executor.py + migrations/postgres.py — SQL injection contract ``run_trusted_sql``'s docstring now narrowly bounds its trust contract (compile-time literals + ``q()``-quoted identifiers + ``_render_value`` constants only) and explicitly rejects manual ``str.replace("'", "''")`` "escapes". The sibling ``run_parameterized_sql`` covers the "trusted template + runtime values" case via psycopg's native binder, so values are never spliced into the SQL string. The migration runner's ``dq_migrations`` INSERT now uses it instead of f-string-baking ``escaped_desc``. backend/sql_executor.py — Delta ``upsert*`` reserved-word safety ``upsert`` and ``upsert_with_audit`` now route every column-name interpolation through ``self.q()``, matching the Postgres mirror. A future audit column named ``check``, ``order``, ``group`` etc. no longer parse-errors on Delta while working on Postgres. tests/test_pg_executor.py — escalation + Decimal coverage - ``test_loop_invokes_os_exit_after_exactly_max_failures``: end-to- end loop -> escalate -> ``os._exit(1)`` chain with nothing but ``os._exit`` patched. Pins the off-by-one boundary and catches a regression that swaps ``os._exit`` for ``sys.exit`` (a daemon thread silently swallows the latter). - ``test_success_grants_a_fresh_threshold_window_before_escalation``: drives ``fail x (N-1), success, fail x N`` and asserts escalation fires on the 6th call, not the 4th or 5th. Catches a refactor that changes the counter reset to a decrement or drops it. - ``test_decimal_uses_decimal_str_repr``: renamed from ``test_decimal_renders_as_str_no_scientific``. The old name lied — the assertion accepted ``"1E-7"`` (scientific). Parametrised plain + scientific cases and documented why downstream consumers tolerate scientific notation. tests/test_pg_migration_runner.py — content-keyed failure injection Replaces ``side_effect = [None, None, RuntimeError, None, None]`` (positionally coupled to the runner's split logic) with a predicate keyed to a substring of the offending DDL. Two new assertions pin that the targeted statement was actually issued and that nothing ran after it — so the rollback + short-circuit contract stays exercised regardless of how the runner batches statements. Also updates the two INSERT-format assertions for the new parameter-binding contract: assertions now read from the bound ``params`` tuple instead of substring-matching ``"VALUES (10,"``, with a complementary ``"VALUES (%s, %s,"`` check that catches a regression back to f-string interpolation. tests/test_lint_policy.py — structural-property tests for the split - Asserts the helper bodies are the only raw ``cur.execute`` sites (2 in ``pg_cursor_helpers.py``, 0 in ``pg_executor.py``). - Asserts ``pg_cursor_helpers.py`` imports psycopg only under ``TYPE_CHECKING`` (the runtime-free property the whole split relies on). - Asserts ``migrations/postgres.py`` imports the helpers from ``pg_cursor_helpers``, not from ``pg_executor`` — catches a regression that resurrects the transitive psycopg dependency. - Dual-import-identity pin (``canonical is reexport``) on both helpers so a future diverging redefinition in ``pg_executor`` fails loudly. databricks.yml — bdf-vo deployment target Adds the ``bdf-vo`` target (catalog ``data_governance``, instance ``data-governance-lakebase``, capacity ``CU_1``, paused triggers) alongside the existing targets. Verification: ``make fmt`` clean (pylint 10/10), ``make test`` 1139 passed, ``make app-test`` 581 passed, ``make app-check`` TypeScript + basedpyright zero errors. Also smoke-tested ``migrations.postgres`` imports cleanly with psycopg blocked at the import-system level.
…nal:databrickslabs/dqx into feat/app-refactor-backend-add-lakebase # Conflicts: # app/tests/test_lint_policy.py # app/tests/test_pg_executor.py # app/tests/test_pg_migration_runner.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… label filtering, error/warning metrics
Storage: move schemas, wheels volume, and Lakebase instance + logical database into databricks.yml with lifecycle.prevent_destroy; replace bootstrap_storage.sh with 'make app-bind' for adopting existing resources in workspaces created by the prior flow.
Backend: introduce PgExecutor + Postgres migration runner for OLTP tables (rules, settings, RBAC, comments, schedules, scheduler bookkeeping); keep analytical tables (validation runs, profiling, quarantine, metrics) on Delta.
Metrics: persist DQX observer's error_row_count / warning_row_count / input_row_count via dq_validation_runs.error_rows / warning_rows and fix Spark Connect Observation.get mutability bug that was overwriting total_rows with limit-pushed values.
UI: rename 'Invalid' -> 'Errors', add 'Warnings' column to quarantine detail, replace 'Has invalid' filter with 'Has failures', surface label badges next to table names in rule selection and schedule editor, add label filtering to Execute Rules and Schedule settings.
Migrations: fix FIELD_ALREADY_EXISTS idempotency in MigrationRunner so v3/v4/v5 ADD COLUMN migrations no-op on fresh deploys whose v1 baseline already includes warning_rows / warnings / error_rows.
Misc: retention test coverage, custom metrics service, post-deploy grants script reuse, docs rewrite (DEPLOYMENT.md, README.md, CLAUDE.md, installation.mdx) for the new declarative storage model.
Changes
Linked issues
Resolves #..
Tests
Documentation and Demos