feat(data-quality): add configurable Data Mart quality checks - #1474
feat(data-quality): add configurable Data Mart quality checks#1474kalnik-a-a wants to merge 10 commits into
Conversation
762ff61 to
f78acbb
Compare
f78acbb to
bb9fe3b
Compare
bb9fe3b to
9c59d04
Compare
laskevych
left a comment
There was a problem hiding this comment.
Review of the Data Quality feature. Process: multi-angle pass over the full diff (engine/SQL dialects, run lifecycle, API/persistence, web) with per-finding verification against the actual code; several initially-suspected issues were dropped as intentional or already guarded (e.g. CANCELLED→SUCCESS on late completion is test-pinned; the latest-run anti-join is already covered by the existing (dataMartId, createdAt) index).
One blocking finding has no inline anchor because the file is not part of this PR's diff:
🔴 Critical (blocking) — packages/api-client/src/runs.ts:16: PROJECT_DATA_MART_RUN_TYPE_VALUES was not updated with DATA_QUALITY, while the backend's GET /data-marts/runs now returns DQ runs (listVisibleByProject has no type filter). parseProjectRunHistory throws OWOXApiError when any row fails isProjectDataMartRun — it never skips rows — so a single Data Quality run makes client.runs.list() fail for the whole page for every consumer of the published @owox/api-client. Please add 'DATA_QUALITY' to the set, and consider skipping unknown run types instead of failing the entire response so the next run type doesn't repeat this. (The web app is unaffected — it uses its own client/enum — which is why tests didn't catch it.)
Inline: 3 more blocking comments, 12 important (non-blocking unless marked), 3 questions, 10 suggestions. Deliberately not flagged (checked, would be churn): the "already-quoted" unwrap heuristic (codebase-wide convention), reusing the type-alias parsers (they have their own quirks and don't fit DQ equality semantics), reusing IdentifierEscaperFacade for column paths (it splits on dots), and assorted micro-duplications.
| @@ -81,9 +99,78 @@ function ModelCanvasViewContent() { | |||
| () => (filtered ? mergeBidirectionalEdges(filtered.edges) : []), | |||
| [filtered] | |||
| ); | |||
| const selectedStorageType = dataStorages.find(storage => storage.id === filters.storageId)?.type; | |||
| const bulkActionDataMarts = useMemo( | |||
There was a problem hiding this comment.
🔴 Critical (blocking): On the canvas, bulkActionDataMarts is built from all filtered nodes (filtered?.nodes ?? []; the search query does not narrow it, and the service pages through every mart), while the canvas has no selection at all (elementsSelectable={false}). Yet the shared button tooltip says "Bulk actions for selected data marts" and the delete dialog says "You're about to delete N selected data marts. This action cannot be undone."
A user can delete every mart matching the current status filter believing they're deleting a selection. Please either add real selection on the canvas or change the copy/confirmation to make "applies to all N filtered data marts" unmistakable. Minor related point: deletion is soft on the backend, so "cannot be undone" is also inaccurate in the other direction.
There was a problem hiding this comment.
The canvas behavior is intentional: the status and relationship controls define the Data Marts shown on the canvas, and Actions apply to all of those Data Marts. There is no separate selection model. I fixed the misleading shared copy so canvas confirmations explicitly say that the action applies to all N Data Marts shown by the current canvas filters, while the table keeps selected-item wording. Search remains a highlight-only control. I also removed the inaccurate irreversible-delete wording because Data Mart deletion is soft delete.
| runId: string, | ||
| identity: DataQualitySnapshotReferenceIdentity | ||
| ): string { | ||
| const runToken = runId |
There was a problem hiding this comment.
🔴 Critical (blocking): Technical views are named per run (dq_<runToken>_<identityHash>, token derived from runId), created via plain CREATE OR REPLACE VIEW on all five storages, and persisted into dataQualitySnapshot.technicalViews — but nothing ever drops them (I found no DROP/cleanup path in the backend, and per-run names mean CREATE OR REPLACE never reuses an object).
A daily scheduled DQ run on a SQL-definition mart with two SQL relationship targets leaks ~1,100 permanent views per year into the customer's warehouse. Before this ships we need a cleanup strategy: drop views at run finish, use a deterministic per-mart/per-definition name, or a retention job that consumes the already-persisted technicalViews references.
There was a problem hiding this comment.
Agreed. I removed DQ-owned per-run view creation, generated dq_* names, persisted technicalViews, and the snapshot-specific Create View path. SQL-based DQ sources and relationship targets now use the existing stable canonical view through DataMartTableReferenceService, so repeated runs do not create additional warehouse objects. The accepted trade-off is that SQL runs use the current canonical definition, matching the existing report/sample-data behavior.
| return 23 * 60 * 60; | ||
| } | ||
|
|
||
| protected getRunTypes(): string[] { |
There was a problem hiding this comment.
🟡 Important (blocking): The generic paths this handler inherits from BaseRunTriggerHandlerService — cleanupOrphanedRuns (base:131-155) and failDataMartRunSafely (base:96-110) — set run.status = FAILED but never touch dataQualitySummary, which stays in its lifecycle state QUEUED.
The latest-run endpoints then report state = QUEUED forever: the workspace polls every 2 s indefinitely, list/canvas render a permanent "Queued" spinner, and Cancel always 409s because the run is already FAILED. These paths need the DQ-specific summary handling that markRunAndSummaryAsExecutionFailed already does (e.g. set EXECUTION_FAILED). Also, the persisted error text is the connector concurrency message, which is misleading on a DATA_QUALITY run.
There was a problem hiding this comment.
Fixed through a small base-handler terminalization hook with a DQ-specific override. An orphaned DQ run now persists FAILED, EXECUTION_FAILED, finishedAt, and a safe DQ execution error in the same save, so summary polling reaches a terminal state.
| } | ||
| : current | ||
| ); | ||
| if (addedRule?.scope.type === 'FIELD') { |
There was a problem hiding this comment.
🟡 Important: onAddCheck updates the draft with rules.map(rule => rule.key === ruleKey ? {...enabled} : rule) — it can only flip existing rules, never append. When a dirty draft predates a rule key that appeared in a refetched effectiveConfig (the guard at ~137 intentionally preserves dirty drafts, while the picker lists fields from the fresh config), the update is a silent no-op — but the "added … — not saved yet" toast still fires because addedRule is looked up in the fresh config. Save then persists a config without the check the user believes they added. Consider appending the rule when it's missing from the draft.
There was a problem hiding this comment.
Fixed. Add Check now enables an existing dirty-draft rule or appends a stored copy when the rule appeared in a newer effective config. The success toast is emitted only after an actual draft change. A dirty-draft plus config-refetch regression test covers the case.
| ); | ||
| if (!claim.affected) | ||
| throw new Error(`Data Quality run ${dataMartRunId} could not be claimed`); | ||
| dataMartRun.status = DataMartRunStatus.RUNNING; |
There was a problem hiding this comment.
🟡 Important: claimRun accepts an already-RUNNING run with no ownership/liveness check, and recoverStuckTriggers resets a PROCESSING trigger after 1 h without knowing whether the executor is still alive (nothing refreshes trigger.modifiedAt during execution). Any run whose checks exceed 1 h wall-time gets a second concurrent executor re-running the remaining checks — duplicate warehouse queries and spend.
The connector handler explicitly guards this case ("already RUNNING, skipping duplicate trigger"); the DQ handler should distinguish crash recovery from a live executor the same way, or heartbeat modifiedAt between checks. (Consumption is deduplicated by processRunId and appends are serialized/upserted, so the damage is cost, not data corruption.)
There was a problem hiding this comment.
Fixed without changing the shared scheduler recovery policy. The DQ handler now refreshes the processing trigger heartbeat while execution is alive and always clears the heartbeat in finally. Heartbeat updates neither trigger status nor version, so normal runner completion and cancellation keep their existing optimistic transitions.
| }; | ||
| } | ||
|
|
||
| function collectCurrentFields( |
There was a problem hiding this comment.
💡 Suggestion (non-blocking): This re-implements the compiler's schema-walking layer as near-verbatim private copies (collectCurrentFields vs collectFieldList, descendantsRequireFlattening, isRepeatedScalar, path helpers, the REPEATED→COMPLEX normalization, reason strings) — and they've already drifted ("…in the current Output Schema" here at :154 vs "…in the Output Schema" compiler:182). These layers must agree by design: the resolver predicts applicability for the UI, the compiler enforces it at run time. A shared module for the field walker + applicability predicates would prevent divergence. (The existing data-mart-schema.utils.ts is not a drop-in — it prunes isHiddenForReporting — so this needs a new shared DQ module.)
Same spirit: compilePkUniqueness/compileDuplicateRows are copy-paste twins that could share one duplicate-group builder, and the zero-filled summary template exists in three places (run service, summary service, DataQualityWorkspace).
There was a problem hiding this comment.
I kept the resolver/compiler boundary unchanged in this PR because a shared walker refactor touches applicability and provider SQL compilation simultaneously. I fixed the concrete inconsistency reported for freshness and retained cross-layer contract coverage. A shared walker should be extracted in a focused follow-up with explicit nested-field and provider invariants.
| effectiveRules.push({ | ||
| ...savedRule, | ||
| isApplicable: false, | ||
| notApplicableReason: staleScopeReason(savedRule.scope), |
There was a problem hiding this comment.
💡 Suggestion (non-blocking): Two small resolver issues: (1) a saved DATA_FRESHNESS rule whose field still exists but lost eligibility (type changed, or became repeated) falls into this stale-key branch and gets "Field scope no longer exists in the current Output Schema" — a false message, since the same field still produces the other field-category rules; a dedicated "field is no longer eligible for freshness" reason would be accurate. (2) storageTypeForSchema's default: return null (~389-404) is the one silent branch in an otherwise loud dispatch chain — for a future storage type it marks every field check not-applicable in the UI while the compiler accepts them; consider throwing there.
There was a problem hiding this comment.
Fixed. If the field still exists but no longer has a freshness-eligible type, the rule now says that the field is no longer eligible for Data Freshness. A genuinely missing field keeps the removed-field reason. Unknown future schema discriminators now fail exhaustively instead of silently becoming not applicable.
| return matchesBigQueryStorageType(actualNativeType, expectedNativeType, expectedMode); | ||
| case DataStorageType.AWS_ATHENA: | ||
| return ( | ||
| normalizeAthenaStorageType(actualNativeType) === |
There was a problem hiding this comment.
💡 Suggestion (non-blocking): matchesProviderStorageType for Athena/Redshift/Databricks returns true when both sides normalize to null (two unrecognized types "match"), while the BigQuery variant guards actual !== null and Snowflake guards !expected. Unreachable today (schema types are enum-validated and fully mapped), but it's a landmine for the next enum member — align the three on the fail-closed guard.
There was a problem hiding this comment.
Fixed for Athena, Redshift, and Databricks. A normalized type must now be known on both sides before equality is considered; null === null can no longer produce a false match. Provider contract tests cover the unknown-type cases.
| ]) | ||
| ) | ||
| ); | ||
| const key = stableExampleKey(values); |
There was a problem hiding this comment.
💡 Suggestion (non-blocking): Two minor example-fidelity issues: parseExamples dedups by serialized value, so NULL_RATE examples on a mart without PK fields all serialize to {null_value: null} and collapse to one row though the header promises up to 3 (consider a row locator when no PKs exist); and unwrapProviderScalar (~274-280) treats any {value: <scalar>} object as a provider wrapper, so a genuine STRUCT<value INT64> example cell is silently flattened to its scalar.
There was a problem hiding this comment.
I kept NULL deduplication intentionally: without a stable PK, three identical null objects do not provide additional diagnostic value, and the UI promises up to three distinct examples. I also did not guess whether a single-key { value: ... } object is a provider wrapper or a real STRUCT. Correct unwrapping requires provider/native column metadata and should be handled in a dedicated follow-up rather than with a shape-only heuristic.
| const runs = await this.repository | ||
| .createQueryBuilder('run') | ||
| .innerJoin('run.dataMart', 'dataMart') | ||
| .leftJoin( |
There was a problem hiding this comment.
💡 Suggestion (non-blocking): Several hot-path queries hydrate more than they use: getLatest/getActiveRunId (2 s-polled) load definitionRun/logs/errors when the response needs id + summary + timestamps (getActiveRunId needs only id); getLatestByDataMartIds here (every list/canvas request) does .getMany() with default selection; and the DQ branch of GetDataMartRunService fetches the run twice (type probe, then heavy re-query — addSelect of the DQ columns in the first query would do). .select() projections would trim the endpoints most exposed to polling. Optional: the ideal composite index for the latest-per-mart anti-join is (dataMartId, type, createdAt) — (dataMartId, createdAt) already exists, so this is a nice-to-have.
There was a problem hiding this comment.
Fixed the low-risk parts. Active-run lookup selects only the ID; latest and bulk summary lookups select only compact summary/timestamp fields; detail retrieval no longer performs a probe followed by a second DQ query; and per-check persistence no longer reloads snapshot/Data Mart/storage. Generic Data Mart list/canvas polling was also removed in favor of the dedicated compact summaries endpoint. I did not add an index without measurements because the existing (dataMartId, createdAt) index already serves the latest-run lookup pattern.
|
Addressing the blocking API-client finding from the review summary: fixed in |
Summary
This PR introduces the first version of Data Quality checks for Data Marts. Users can configure checks, run them manually or on a schedule, and inspect the results through the same Run History used by connector and report runs.
The feature is available for BigQuery, Legacy BigQuery, Athena, Snowflake, Redshift, and Databricks Data Marts.
What changed
Data Quality engine and storage support
future_valuescheck are not part of this version.Run lifecycle, persistence, and API
DATA_QUALITYas a regularDataMartRuntype.DataMartRun; no separate run-result entity is introduced.Web experience
Run semantics
violationCountrepresents the affected rows, groups, or schema-level violations for that rule.RUNNING; runs that never start are not charged.Validation
.env.testscredentials were configuredScreenshots
Model Canvas
Output Schema Badges
Data Quality Tab Layout
Checks Configuration Section
Latest Report Section
Run History
Triggers