Skip to content

Commit fe7efa6

Browse files
committed
Batch Monitored Tables N+1 fix + hide Review&Approve when approvals off (B2-141/B2-142)
- B2-141: the Monitored Tables list computed draft check-counts with a per-never-approved-binding render loop (~3N + 2*sum(R) sequential OLTP round-trips). Batched to 3 constant grouped queries (list_applied_rules_many, get_rules_many, get_versions_many) with count parity preserved. - B2-142: hide the Review & Approve sidebar item and its trailing divider when the app-wide approvals mode is 'disabled' (no review queue). Co-authored-by: Isaac
1 parent d74ec0d commit fe7efa6

9 files changed

Lines changed: 475 additions & 50 deletions

File tree

app/src/databricks_labs_dqx_app/backend/routes/v1/monitored_tables.py

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -156,26 +156,42 @@ def _apply_snapshot_check_counts(
156156
frozen snapshot, resolved for ALL such bindings in one batched
157157
:meth:`MonitoredTableVersionService.snapshot_counts_many` call;
158158
* never-approved binding (``version == 0``, no snapshot) -> the live render
159-
count (exactly what a draft run would execute).
160-
161-
Mirrors :class:`DataProductService`'s pinned-vs-live member-count split.
159+
count (exactly what a draft run would execute), resolved for ALL such
160+
bindings in one batched
161+
:meth:`Materializer.render_binding_checks_counts_many` call.
162+
163+
Both branches are query-bounded regardless of how many bindings the list
164+
holds (B2-141): the never-approved branch previously called
165+
``render_binding_checks`` once per binding, fanning out to ~``3N + 2·ΣR``
166+
sequential OLTP round-trips on a fresh (all-draft) install; it now costs a
167+
constant handful of grouped queries. Mirrors
168+
:class:`DataProductService`'s pinned-vs-live member-count split.
162169
"""
163170
pins = [(s.table.binding_id, s.table.version) for s in summaries if s.table.version > 0]
164171
snapshot_counts = version_svc.snapshot_counts_many(pins) if pins else {}
172+
173+
# Bindings that must fall back to the live draft-render count: every
174+
# never-approved binding, plus any approved one whose frozen snapshot row
175+
# is missing (so the overview still reflects what a draft run would run).
176+
live_bindings: list[tuple[str, str]] = []
177+
for summary in summaries:
178+
version = summary.table.version
179+
if version > 0 and snapshot_counts.get((summary.table.binding_id, version)) is not None:
180+
continue
181+
live_bindings.append((summary.table.binding_id, summary.table.table_fqn))
182+
try:
183+
live_counts = materializer.render_binding_checks_counts_many(live_bindings)
184+
except MaterializationError:
185+
live_counts = {}
186+
165187
for summary in summaries:
166188
version = summary.table.version
167189
if version > 0:
168190
snapshot = snapshot_counts.get((summary.table.binding_id, version))
169191
if snapshot is not None:
170192
summary.check_count = snapshot[1]
171193
continue
172-
# Never approved (or an approved binding whose snapshot row is missing):
173-
# fall back to the live render count so the overview still reflects
174-
# what a draft run would execute.
175-
try:
176-
summary.check_count = len(materializer.render_binding_checks(summary.table.binding_id))
177-
except MaterializationError:
178-
summary.check_count = 0
194+
summary.check_count = live_counts.get(summary.table.binding_id, 0)
179195

180196

181197
@router.get(

app/src/databricks_labs_dqx_app/backend/services/materializer.py

Lines changed: 125 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858

5959
import json
6060
import logging
61+
from collections.abc import Mapping
6162
from typing import Any
6263

6364
from databricks.labs.dqx.errors import UnsafeSqlQueryError
@@ -373,8 +374,55 @@ def materialize_binding(self, binding_id: str) -> list[str]:
373374
self._cleanup_orphans(applied_ids=applied_ids, written_ids=written_ids)
374375
return sorted(written_ids)
375376

377+
def _resolve_registry(
378+
self,
379+
applied: AppliedRule,
380+
*,
381+
rules: Mapping[str, RegistryRule] | None = None,
382+
versions: Mapping[tuple[str, int], RuleVersion] | None = None,
383+
) -> tuple[RegistryRule, int, RuleVersion] | None:
384+
"""Resolve an applied rule to its ``(registry_rule, version_number, snapshot)``.
385+
386+
This is the ONLY place the ``get_rule`` + ``get_version`` OLTP lookups
387+
happen for a render. When *rules* / *versions* preloaded maps are
388+
supplied (batch path), they are consulted instead of the per-id
389+
``RegistryService`` calls — so a batch caller pays two grouped queries
390+
for N applications rather than ``2N`` sequential round-trips. The
391+
resolution logic (missing rule, unpublished version, missing snapshot)
392+
is identical either way, so single- and batch-path renders stay
393+
byte-identical.
394+
395+
Returns ``None`` (with the same warning logs as before) when the applied
396+
rule can't be resolved at all.
397+
"""
398+
registry_rule = rules.get(applied.rule_id) if rules is not None else self._registry.get_rule(applied.rule_id)
399+
if registry_rule is None or not applied.id:
400+
logger.warning("Skipping applied rule %s: registry rule %s not found", applied.id, applied.rule_id)
401+
return None
402+
403+
version_number = applied.pinned_version or registry_rule.version
404+
if version_number <= 0:
405+
logger.warning("Skipping applied rule %s: rule %s has no published version", applied.id, applied.rule_id)
406+
return None
407+
408+
if versions is not None:
409+
version_snapshot = versions.get((applied.rule_id, version_number))
410+
else:
411+
version_snapshot = self._registry.get_version(applied.rule_id, version_number)
412+
if version_snapshot is None:
413+
logger.warning(
414+
"Skipping applied rule %s: version %d of rule %s not found", applied.id, version_number, applied.rule_id
415+
)
416+
return None
417+
return registry_rule, version_number, version_snapshot
418+
376419
def _iter_rendered_checks(
377-
self, table_fqn: str, applied: AppliedRule
420+
self,
421+
table_fqn: str,
422+
applied: AppliedRule,
423+
*,
424+
rules: Mapping[str, RegistryRule] | None = None,
425+
versions: Mapping[tuple[str, int], RuleVersion] | None = None,
378426
) -> list[tuple[str, str, dict[str, Any]]] | None:
379427
"""Resolve + render an applied rule's mapping groups WITHOUT writing anything.
380428
@@ -393,24 +441,20 @@ def _iter_rendered_checks(
393441
order — possibly EMPTY when every group failed to render, which the
394442
materializer treats as "this application now renders no rows" and
395443
cleans up accordingly.
396-
"""
397-
registry_rule = self._registry.get_rule(applied.rule_id)
398-
if registry_rule is None or not applied.id:
399-
logger.warning("Skipping applied rule %s: registry rule %s not found", applied.id, applied.rule_id)
400-
return None
401-
402-
version_number = applied.pinned_version or registry_rule.version
403-
if version_number <= 0:
404-
logger.warning("Skipping applied rule %s: rule %s has no published version", applied.id, applied.rule_id)
405-
return None
406444
407-
version_snapshot = self._registry.get_version(applied.rule_id, version_number)
408-
if version_snapshot is None:
409-
logger.warning(
410-
"Skipping applied rule %s: version %d of rule %s not found", applied.id, version_number, applied.rule_id
411-
)
445+
Registry resolution (the ``get_rule`` + ``get_version`` round-trips) is
446+
delegated to :meth:`_resolve_registry`; passing a preloaded
447+
*rules*/*versions* map lets a batch caller resolve every application's
448+
registry rows in two grouped queries and share them here without any
449+
per-application round-trip (see :meth:`render_binding_checks_many`).
450+
"""
451+
resolved = self._resolve_registry(applied, rules=rules, versions=versions)
452+
if resolved is None:
412453
return None
413-
454+
registry_rule, version_number, version_snapshot = resolved
455+
# ``_resolve_registry`` already rejected a falsy ``applied.id`` (returning
456+
# None), so it is a real str here — narrow it for the type checker.
457+
applied_id = applied.id or ""
414458
effective_severity = (
415459
applied.severity_override or get_rule_severity(version_snapshot.user_metadata) or _DEFAULT_SEVERITY
416460
)
@@ -424,7 +468,7 @@ def _iter_rendered_checks(
424468
rendered_mode = version_snapshot.mode or registry_rule.mode
425469
rendered: list[tuple[str, str, dict[str, Any]]] = []
426470
for idx, group in enumerate(applied.column_mapping):
427-
row_id = f"{applied.id}-{idx}"
471+
row_id = f"{applied_id}-{idx}"
428472
try:
429473
check, is_tableless = render_check(
430474
mode=rendered_mode,
@@ -434,7 +478,7 @@ def _iter_rendered_checks(
434478
per_application_tags=applied.user_metadata,
435479
registry_rule_id=applied.rule_id,
436480
registry_version=version_number,
437-
applied_rule_id=applied.id,
481+
applied_rule_id=applied_id,
438482
app_settings=self._app_settings,
439483
)
440484
except (ValueError, UnsafeSqlQueryError):
@@ -689,6 +733,68 @@ def render_binding_checks(self, binding_id: str) -> list[dict[str, Any]]:
689733
checks.extend(check for _row_id, _row_fqn, check in rendered)
690734
return checks
691735

736+
def render_binding_checks_counts_many(self, bindings: list[tuple[str, str]]) -> dict[str, int]:
737+
"""Batched draft-render CHECK COUNT for many *(binding_id, table_fqn)* pairs.
738+
739+
The bounded-query counterpart of calling :meth:`render_binding_checks`
740+
in a loop: it produces the SAME per-binding count (the number of checks
741+
a draft run would render) but resolves the registry rows for EVERY
742+
binding's applications in a constant handful of grouped queries instead
743+
of ``~3N + 2·ΣR`` sequential round-trips (N bindings, R applied rules
744+
each):
745+
746+
1. ONE grouped ``dq_applied_rules`` query for all bindings' applications
747+
(:meth:`MonitoredTableService.list_applied_rules_many`);
748+
2. ONE ``dq_rules`` ``IN (...)`` query for the distinct rule ids
749+
(:meth:`RegistryService.get_rules_many`);
750+
3. ONE ``dq_rule_versions`` predicate-OR query for the distinct
751+
``(rule_id, version)`` pairs (:meth:`RegistryService.get_versions_many`).
752+
753+
Rendering then runs purely in-memory per binding through the SAME
754+
:meth:`_iter_rendered_checks` path — so each count is byte-identical to
755+
what the per-binding :meth:`render_binding_checks` would return. A
756+
binding whose applications all fail to resolve counts 0. Bindings not
757+
present in *bindings* are absent from the result; an empty input issues
758+
no query and returns ``{{}}``.
759+
"""
760+
if not bindings:
761+
return {}
762+
binding_fqns = {binding_id: table_fqn for binding_id, table_fqn in bindings}
763+
applied_by_binding = self._monitored_tables.list_applied_rules_many(list(binding_fqns))
764+
765+
rule_ids: set[str] = set()
766+
for applied_rules in applied_by_binding.values():
767+
for applied in applied_rules:
768+
if applied.id:
769+
rule_ids.add(applied.rule_id)
770+
rules = self._registry.get_rules_many(rule_ids)
771+
772+
version_pairs: set[tuple[str, int]] = set()
773+
for applied_rules in applied_by_binding.values():
774+
for applied in applied_rules:
775+
if not applied.id:
776+
continue
777+
registry_rule = rules.get(applied.rule_id)
778+
if registry_rule is None:
779+
continue
780+
version_number = applied.pinned_version or registry_rule.version
781+
if version_number > 0:
782+
version_pairs.add((applied.rule_id, version_number))
783+
versions = self._registry.get_versions_many(version_pairs)
784+
785+
counts: dict[str, int] = {}
786+
for binding_id, table_fqn in binding_fqns.items():
787+
count = 0
788+
for applied in applied_by_binding.get(binding_id, []):
789+
if not applied.id:
790+
continue
791+
rendered = self._iter_rendered_checks(table_fqn, applied, rules=rules, versions=versions)
792+
if rendered is None:
793+
continue
794+
count += len(rendered)
795+
counts[binding_id] = count
796+
return counts
797+
692798
def rematerialize_for_rule(self, rule_id: str) -> list[str]:
693799
"""Re-materialize every binding with a FOLLOWING (unpinned) application of *rule_id*.
694800

app/src/databricks_labs_dqx_app/backend/services/monitored_table_service.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -629,6 +629,35 @@ def _list_applied_rules(self, binding_id: str) -> list[AppliedRuleSummary]:
629629
)
630630
return summaries
631631

632+
def list_applied_rules_many(self, binding_ids: list[str]) -> dict[str, list[AppliedRule]]:
633+
"""Applied rules for all *binding_ids* in ONE grouped query (no per-binding round-trip).
634+
635+
Unlike :meth:`_list_applied_rules`, this returns bare
636+
:class:`AppliedRule` rows WITHOUT the per-rule descriptive-tag join
637+
(name/dimension/severity): callers that only need the applications
638+
themselves — e.g. the draft check-count render — must not pay one
639+
``dq_rules`` lookup per applied rule. Each binding's list preserves
640+
``created_at`` order to match :meth:`_list_applied_rules`; a binding
641+
with no applied rows is simply absent from the result. An empty input
642+
issues no query.
643+
"""
644+
if not binding_ids:
645+
return {}
646+
distinct = sorted({b for b in binding_ids if b})
647+
if not distinct:
648+
return {}
649+
in_list = ", ".join(f"'{escape_sql_string(b)}'" for b in distinct)
650+
sql = (
651+
f"SELECT {self._applied_select_cols} FROM {self._applied_table} " # noqa: S608
652+
f"WHERE binding_id IN ({in_list}) ORDER BY binding_id, created_at"
653+
)
654+
rows = self._sql.query(sql)
655+
grouped: dict[str, list[AppliedRule]] = {}
656+
for row in rows:
657+
applied = self._row_to_applied_rule(row)
658+
grouped.setdefault(applied.binding_id, []).append(applied)
659+
return grouped
660+
632661
def _rule_tags(self, rule_id: str) -> tuple[str | None, str | None, str | None]:
633662
"""Look up name/dimension/severity tags for *rule_id* from ``dq_rules``.
634663

app/src/databricks_labs_dqx_app/backend/services/registry_service.py

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import json
2222
import logging
23+
from collections.abc import Collection
2324
from datetime import datetime, timezone
2425
from typing import Any, cast, get_args
2526
from uuid import uuid4
@@ -316,6 +317,62 @@ def get_version(self, rule_id: str, version: int) -> RuleVersion | None:
316317
"""
317318
return self._get_version(rule_id, version)
318319

320+
def get_rules_many(self, rule_ids: Collection[str]) -> dict[str, RegistryRule]:
321+
"""Resolve many rules by id in ONE ``IN (...)`` query (no per-id round-trip).
322+
323+
Mirrors :meth:`get_rule` for a batch of ids — the value at each key is
324+
exactly what :meth:`get_rule` would return for that id. Ids with no
325+
``dq_rules`` row are simply absent from the result, so callers can treat
326+
a missing key the same as a ``None`` from :meth:`get_rule`. Duplicate
327+
ids collapse to one predicate. An empty input issues no query.
328+
329+
Backs list-view batching (e.g. the monitored-tables draft check-count
330+
pass) that must resolve the rules of many applications at once instead
331+
of looping :meth:`get_rule` per application.
332+
"""
333+
distinct = {rid for rid in rule_ids if rid}
334+
if not distinct:
335+
return {}
336+
in_list = ", ".join(f"'{escape_sql_string(rid)}'" for rid in sorted(distinct))
337+
sql = f"SELECT {self._select_cols} FROM {self._table} WHERE rule_id IN ({in_list})" # noqa: S608
338+
rows = self._sql.query(sql)
339+
result: dict[str, RegistryRule] = {}
340+
for row in rows:
341+
rule = self._row_to_rule(row)
342+
result[rule.rule_id] = rule
343+
return result
344+
345+
def get_versions_many(self, pairs: Collection[tuple[str, int]]) -> dict[tuple[str, int], RuleVersion]:
346+
"""Resolve many frozen version snapshots by ``(rule_id, version)`` in ONE query.
347+
348+
The batch counterpart of :meth:`get_version`: the value at each
349+
``(rule_id, version)`` key equals what :meth:`get_version` would return
350+
for that pair. Pairs with no ``dq_rule_versions`` row are absent from
351+
the result. Duplicate pairs collapse to one predicate; an empty input
352+
issues no query. Uses the same predicate-OR shape as
353+
:meth:`_attach_modified` / :meth:`MonitoredTableVersionService.snapshot_counts_many`.
354+
"""
355+
distinct = {(rid, int(ver)) for rid, ver in pairs if rid}
356+
if not distinct:
357+
return {}
358+
definition = self._sql.select_json_text("definition")
359+
user_metadata = self._sql.select_json_text("user_metadata")
360+
created_at = self._sql.ts_text("created_at")
361+
predicates = " OR ".join(
362+
f"(rule_id = '{escape_sql_string(rid)}' AND version = {int(ver)})" for rid, ver in sorted(distinct)
363+
)
364+
sql = (
365+
f"SELECT rule_id, version, {definition} AS definition_json, polarity, " # noqa: S608
366+
f"{user_metadata} AS user_metadata_json, created_by, {created_at}, mode "
367+
f"FROM {self._versions_table} WHERE {predicates}"
368+
)
369+
rows = self._sql.query(sql)
370+
result: dict[tuple[str, int], RuleVersion] = {}
371+
for row in rows:
372+
version = self._row_to_version(row)
373+
result[(version.rule_id, version.version)] = version
374+
return result
375+
319376
def get_rule_with_version(self, rule_id: str) -> tuple[RegistryRule, RuleVersion | None] | None:
320377
"""Get a registry rule plus its current published snapshot, if any.
321378

0 commit comments

Comments
 (0)