From f398c4275d03cc45ade98ce6a01083c1cfade49f Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 16:16:40 +0530 Subject: [PATCH 01/12] fix(dq): check test parameters against their definitions and run boolean and open-window tests correctly --- .../mysql/postDataMigrationSQLScript.sql | 33 ++++ .../postgres/postDataMigrationSQLScript.sql | 52 ++++++ ...cked-against-their-definition-at-create.md | 40 ++++ ...et-parameter-is-read-in-its-column-type.md | 32 ++++ .../validations/base_test_handler.py | 16 ++ .../checkers/between_bounds_checker.py | 22 ++- .../column/base/columnValuesToBeInSet.py | 10 +- .../column/base/columnValuesToBeNotInSet.py | 10 +- .../column/pandas/columnValuesToBeInSet.py | 7 +- .../column/pandas/columnValuesToBeNotInSet.py | 7 +- .../columnValueLengthsToBeBetween.py | 25 +-- .../sqlalchemy/columnValuesToBeBetween.py | 25 +-- .../sqlalchemy/columnValuesToBeInSet.py | 8 +- .../sqlalchemy/columnValuesToBeNotInSet.py | 7 +- .../mixins/pandas_validator_mixin.py | 5 + .../validations/mixins/sqa_validator_mixin.py | 29 +++ .../data_quality/validations/utils.py | 31 ++++ .../src/metadata/profiler/orm/registry.py | 9 +- .../observability/data_quality/conftest.py | 85 +++++++++ .../test_validations_databases.py | 79 +++++++- .../data_quality/test_validations_datalake.py | 47 +++++ .../test_between_row_violations.py | 37 ++++ .../validations/test_row_threshold.py | 14 +- .../data_quality/validations/test_utils.py | 68 +++++++ .../it/tests/TestCaseResourceIT.java | 55 ++++++ .../jdbi3/TestCaseDefinitionConformance.java | 95 ++++++++++ .../service/jdbi3/TestCaseRepository.java | 4 + .../json/data/tests/tableCustomSQLQuery.json | 2 +- .../service/jdbi3/SeededTestDefinitions.java | 27 +++ .../TestCaseDefinitionConformanceTest.java | 171 ++++++++++++++++++ .../jdbi3/TestCaseThresholdValidatorTest.java | 22 +-- .../ui/playwright/e2e/Pages/Entity.spec.ts | 18 +- 32 files changed, 972 insertions(+), 120 deletions(-) create mode 100644 docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md create mode 100644 docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md create mode 100644 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java create mode 100644 openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SeededTestDefinitions.java create mode 100644 openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java diff --git a/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql b/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql index ecbe3e6f6daf..4fbba710583e 100644 --- a/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql +++ b/bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql @@ -642,3 +642,36 @@ SET json = JSON_ARRAY_APPEND( WHERE name = 'tableDiff' AND JSON_TYPE(JSON_EXTRACT(json, '$.parameterDefinition')) = 'ARRAY' AND NOT JSON_CONTAINS(json, JSON_OBJECT('name', 'parallelQueries'), '$.parameterDefinition'); + +-- Say how to choose tableCustomSQLQuery.strategy and what columnValuesToBeInSet.matchEnum changes, +-- so users and test generation read it from the definition. Seed data only creates missing definitions. +UPDATE test_definition +SET json = JSON_SET( + json, + REPLACE(JSON_UNQUOTE(JSON_SEARCH(json, 'one', 'strategy', NULL, '$.parameterDefinition[*].name')), '.name', '.description'), + 'How the query result is compared to the threshold. ROWS (default): the query returns the rows that break the rule, such as `SELECT * FROM t WHERE amount < 0`, and the number of rows returned is compared. COUNT: the query returns a single integer, such as `SELECT COUNT(*) FROM t WHERE amount < 0`, and that number is compared. Under ROWS a `SELECT COUNT(*)` query always returns one row. Do not use COUNT with a query that can return NULL or a non-integer, such as SUM or AVG.' +) +WHERE name = 'tableCustomSQLQuery' + AND JSON_SEARCH(json, 'one', 'strategy', NULL, '$.parameterDefinition[*].name') IS NOT NULL; + +UPDATE test_definition +SET json = JSON_SET( + json, + REPLACE(JSON_UNQUOTE(JSON_SEARCH(json, 'one', 'matchEnum', NULL, '$.parameterDefinition[*].name')), '.name', '.description'), + 'If true, every observed value must be allowed. If false or omitted, the test passes when at least one allowed value occurs, even if other values are outside the set.' +) +WHERE name = 'columnValuesToBeInSet' + AND JSON_SEARCH(json, 'one', 'matchEnum', NULL, '$.parameterDefinition[*].name') IS NOT NULL; + +UPDATE test_definition +SET json = JSON_SET( + json, + REPLACE(JSON_UNQUOTE(JSON_SEARCH(json, 'one', 'allowedValues', NULL, '$.parameterDefinition[*].name')), '.name', '.description'), + 'Values used for matching. Neither mode requires every allowed value to occur in the column.' +) +WHERE name = 'columnValuesToBeInSet' + AND JSON_SEARCH(json, 'one', 'allowedValues', NULL, '$.parameterDefinition[*].name') IS NOT NULL; + +UPDATE test_definition +SET json = JSON_SET(json, '$.description', 'Checks column values against an allowed set according to the matchEnum mode.') +WHERE name = 'columnValuesToBeInSet'; diff --git a/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql b/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql index d39c5ea39ef5..48840b5dab69 100644 --- a/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql +++ b/bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql @@ -615,3 +615,55 @@ SET json = jsonb_set( WHERE name = 'tableDiff' AND jsonb_typeof(json->'parameterDefinition') = 'array' AND NOT ((json->'parameterDefinition')::jsonb @> '[{"name": "parallelQueries"}]'::jsonb); + +-- Say how to choose tableCustomSQLQuery.strategy and what columnValuesToBeInSet.matchEnum changes, +-- so users and test generation read it from the definition. Seed data only creates missing definitions. +UPDATE test_definition +SET json = jsonb_set( + json, + '{parameterDefinition}', + ( + SELECT jsonb_agg( + CASE + WHEN param->>'name' = 'strategy' THEN param || jsonb_build_object( + 'description', + 'How the query result is compared to the threshold. ROWS (default): the query returns the rows that break the rule, such as `SELECT * FROM t WHERE amount < 0`, and the number of rows returned is compared. COUNT: the query returns a single integer, such as `SELECT COUNT(*) FROM t WHERE amount < 0`, and that number is compared. Under ROWS a `SELECT COUNT(*)` query always returns one row. Do not use COUNT with a query that can return NULL or a non-integer, such as SUM or AVG.' + ) + ELSE param + END + ORDER BY position + ) + FROM jsonb_array_elements(json->'parameterDefinition') WITH ORDINALITY AS params(param, position) + ) +) +WHERE name = 'tableCustomSQLQuery' + AND json->'parameterDefinition' @> '[{"name": "strategy"}]'::jsonb; + +UPDATE test_definition +SET json = jsonb_set( + json, + '{parameterDefinition}', + ( + SELECT jsonb_agg( + CASE + WHEN param->>'name' = 'matchEnum' THEN param || jsonb_build_object( + 'description', + 'If true, every observed value must be allowed. If false or omitted, the test passes when at least one allowed value occurs, even if other values are outside the set.' + ) + WHEN param->>'name' = 'allowedValues' THEN param || jsonb_build_object( + 'description', + 'Values used for matching. Neither mode requires every allowed value to occur in the column.' + ) + ELSE param + END + ORDER BY position + ) + FROM jsonb_array_elements(json->'parameterDefinition') WITH ORDINALITY AS params(param, position) + ) +) +WHERE name = 'columnValuesToBeInSet' + AND json->'parameterDefinition' @> '[{"name": "matchEnum"}]'::jsonb; + +UPDATE test_definition +SET json = jsonb_set(json, '{description}', to_jsonb('Checks column values against an allowed set according to the matchEnum mode.'::text)) +WHERE name = 'columnValuesToBeInSet'; diff --git a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md new file mode 100644 index 000000000000..171946243c91 --- /dev/null +++ b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md @@ -0,0 +1,40 @@ +# A new test case is checked against its definition's options and column types + +- **Status:** Accepted +- **Revisions:** v1 2026-10-12 (initial) +- **Deciders:** Shreyansh Verma +- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `patch_doesNotRecheckDefinitionConformance`) +- **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line) + +## Context + +`validateTestParameters` checked parameter names, required parameters and min/max rules. Nothing +compared a value with the parameter's optionValues, or the column's type with the definition's +supportedDataTypes (used only to filter the definition listing). Test cases that ingestion then +aborted on (`'SUM' is not a valid Strategy`) or that measured nothing were stored as sent. + +## Decision + +`prepare()` rejects, when `update` is false, a test case that: + +- gives a parameter with optionValues a value that is none of them (compared case-insensitively, + as `TestCaseThresholdValidator` reads thresholdUnit; a null, empty or whitespace value is unset); +- targets a column whose dataType is not in a non-empty supportedDataTypes (an empty list means + every type, as the listing filter reads it; a column with no dataType is accepted). The column is + found case-insensitively, as `validateColumnTestCase` finds it. + +The gate is the one the disabled-definition check uses, so a row stored before these rules stays +editable by PATCH and PUT. + +useDynamicAssertion is not checked against supportsDynamicAssertion: a client can still send it on +a definition without dynamic support (the Collate UI does, from a toggle left on), and rejecting it +would break that client. Test generation already sends it only where it is supported. + +## Consequences + +The UI, MCP `CreateTestCaseTool`, ODCS materialization and API clients all get these checks. ODCS +materialization catches the `IllegalArgumentException` and records the rule as skipped. PUT +`createOrUpdate` prepares with `update` true even for a new row, and the non-dry-run CSV import does +not call `prepare()`, so neither is covered yet; both are follow-ups. The ai-platform test generator +applies the same reading before it sends a request (ADR:ai-platform/2026-10-12-generated-test-parameters-conform-to-their-definition), +so a server rejection there means the two have drifted. diff --git a/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md new file mode 100644 index 000000000000..baf896661dc6 --- /dev/null +++ b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md @@ -0,0 +1,32 @@ +# An allowed or forbidden set is read as a Python literal or JSON, in its column's type + +- **Status:** Accepted +- **Revisions:** v1 2026-10-16 (initial) +- **Deciders:** Shreyansh Verma +- **Guard:** `tests/unit/observability/data_quality/validations/test_utils.py` (`parse_set_values`, `is_boolean`), `test_validations_databases.py` (`..._boolean_strings`, `..._boolean_json`, `..._missing_column`), `test_validations_datalake.py` (`test_string_booleans_on_a_datalake_column_stay_strings`) +- **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line); ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create + +## Context + +`columnValuesToBeInSet` and `columnValuesToBeNotInSet` parsed `allowedValues` / `forbiddenValues` +with `literal_eval`. Clients write these parameters differently: the UI writes a JSON array of +strings, a generated test writes JSON (`[false]`), older test cases hold Python literals +(`[True, False]`). `literal_eval` rejects JSON's lowercase `false` / `true` / `null`, which aborted +the test, and on a BOOLEAN column it kept `"true"` / `"false"` as strings, which an engine such as +Databricks refuses to compare with a boolean. + +## Decision + +`parse_set_values` reads a set as a Python literal and, when it is not one, as JSON; where both +parse a value they agree, so nothing that parsed before reads differently. A value that is neither, +or that is not a list, raises ValueError and the test is Aborted, as before. On a column whose +engine type is boolean, `"true"` / `"false"` in any case become booleans; nothing else is mapped. + +The column type is the engine's (`SQAValidatorMixin.get_set_values_type`). Pandas returns none, so +a datalake set keeps its values as written: pandas infers column types from the values, and a +column of `"True"` / `"False"` strings reads as BOOLEAN. + +## Consequences + +A set written by any client compares the same way, and a set test no longer aborts on JSON. A +boolean column read through pandas still compares as strings; that is deliberate, not an omission. diff --git a/ingestion/src/metadata/data_quality/validations/base_test_handler.py b/ingestion/src/metadata/data_quality/validations/base_test_handler.py index c5d998df259b..1a6fdce1dc42 100644 --- a/ingestion/src/metadata/data_quality/validations/base_test_handler.py +++ b/ingestion/src/metadata/data_quality/validations/base_test_handler.py @@ -23,6 +23,7 @@ from abc import ABC, abstractmethod from collections.abc import Callable from decimal import Decimal +from functools import partial from typing import ( TYPE_CHECKING, TypedDict, @@ -1356,6 +1357,21 @@ def get_max_bound(self, param_name: str) -> float | None: default=float("inf"), ) + def get_set_param_value(self, param_name: str) -> list: + """An allowed or forbidden set, typed by the column it is checked against""" + return self.get_test_case_param_value( + self.test_case.parameterValues, # type: ignore + param_name, + partial(utils.parse_set_values, column_type=self.get_set_values_type()), + ) + + def get_set_values_type(self): + """The column type a set is parsed against, or None to keep its values as written. + + Implementation provided by SQAValidatorMixin or PandasValidatorMixin via MRO chain. + """ + return super().get_set_values_type() # type: ignore + def get_bounds(self, min_param_name: str, max_param_name: str) -> tuple[float | None, float | None]: """Resolve the test case bounds and widen them by the failure threshold. diff --git a/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py b/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py index ed3d77b627c2..7fa3ab6fd315 100644 --- a/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py +++ b/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py @@ -25,6 +25,18 @@ from sqlalchemy.sql.elements import ClauseElement, ColumnElement +def is_unbounded(bound: Any) -> bool: + """Whether that side of the window lets everything through + + An unset bound resolves to ∓inf, and to None for a validator that resolves its bounds + dynamically and found none on that side. Neither excludes a value, so neither needs a + condition -- and None cannot be compared against at all. Only a float can be infinite: + a datetime window -- what a between test on a date column resolves to -- compares fine + and must not be handed to `math.isinf`, which only takes a number. + """ + return bound is None or (isinstance(bound, float) and math.isinf(bound)) + + class BetweenBoundsChecker(BaseValidationChecker): """Checks if value is outside [min_bound, max_bound]""" @@ -33,15 +45,7 @@ def __init__(self, min_bound: float, max_bound: float): self.max_bound = max_bound def _is_unbounded(self, bound: Any) -> bool: - """Whether that side of the window lets everything through - - An unset bound resolves to ∓inf, and to None for a validator that resolves its bounds - dynamically and found none on that side. Neither excludes a value, so neither needs a - condition -- and None cannot be compared against at all. Only a float can be infinite: - a datetime window -- what a between test on a date column resolves to -- compares fine - and must not be handed to `math.isinf`, which only takes a number. - """ - return bound is None or (isinstance(bound, float) and math.isinf(bound)) + return is_unbounded(bound) def _check_violations(self, values): """Core violation check logic - works for both scalar and Series. diff --git a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py index 4f6d32f0dd5d..87bdfa51d5c4 100644 --- a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeInSet.py @@ -15,7 +15,6 @@ import traceback from abc import abstractmethod -from ast import literal_eval from sqlalchemy import Column @@ -55,9 +54,8 @@ def _run_validation(self) -> TestCaseResult: Returns: TestCaseResult: The test case result for the overall validation """ - test_params = self._get_test_parameters() - try: + test_params = self._get_test_parameters() column: SQALikeColumn | Column = self.get_column() count_in_set = self._run_results(Metrics.countInSet, column, values=test_params[self.ALLOWED_VALUES]) @@ -104,11 +102,7 @@ def _get_test_parameters(self) -> dict: Returns: dict with keys: allowed_values, match_enum """ - allowed_values = self.get_test_case_param_value( - self.test_case.parameterValues, - self.ALLOWED_VALUES, - literal_eval, - ) + allowed_values = self.get_set_param_value(self.ALLOWED_VALUES) match_enum = utils.get_bool_test_case_param(self.test_case.parameterValues, self.MATCH_ENUM) return { diff --git a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py index c03deb512efd..392c85a57b77 100644 --- a/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/base/columnValuesToBeNotInSet.py @@ -15,7 +15,6 @@ import traceback from abc import abstractmethod -from ast import literal_eval from sqlalchemy import Column @@ -53,9 +52,8 @@ def _run_validation(self) -> TestCaseResult: Returns: TestCaseResult: The test case result for the overall validation """ - test_params = self._get_test_parameters() - try: + test_params = self._get_test_parameters() column: SQALikeColumn | Column = self.get_column() metric_values = self._run_results_with_row_count( Metrics.countInSet, column, values=test_params[self.FORBIDDEN_VALUES] @@ -91,11 +89,7 @@ def _get_test_parameters(self) -> dict: Returns: dict with keys: allowed_values, match_enum """ - forbidden_values = self.get_test_case_param_value( - self.test_case.parameterValues, - self.FORBIDDEN_VALUES, - literal_eval, - ) + forbidden_values = self.get_set_param_value(self.FORBIDDEN_VALUES) return { self.FORBIDDEN_VALUES: forbidden_values, diff --git a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py index c4f3574e0634..38c2a0d085b3 100644 --- a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeInSet.py @@ -13,7 +13,6 @@ Validator for column value to be in set test case """ -from ast import literal_eval from collections import defaultdict from typing import cast @@ -190,11 +189,7 @@ def compute_row_count(self, column: SQALikeColumn): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, - "allowedValues", - literal_eval, - ) + items = self.get_set_param_value("allowedValues") return f"~{self.get_column().name}.isin({items})" def fetch_failed_rows_sample(self): diff --git a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py index 834828fadb74..7dd708d70367 100644 --- a/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/pandas/columnValuesToBeNotInSet.py @@ -13,7 +13,6 @@ Validator for column value to be not in set test case """ -from ast import literal_eval from collections import defaultdict from typing import cast @@ -178,11 +177,7 @@ def compute_row_count(self, column: SQALikeColumn): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, - "forbiddenValues", - literal_eval, - ) + items = self.get_set_param_value("forbiddenValues") return f"{self.get_column().name}.isin({items})" def fetch_failed_rows_sample(self): diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py index 0ac97b363c60..1ab315f5cbb0 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValueLengthsToBeBetween.py @@ -13,7 +13,6 @@ Validator for column value length to be between test case """ -import math from typing import cast from sqlalchemy import Column @@ -22,6 +21,9 @@ DIMENSION_FAILED_COUNT_KEY, DIMENSION_TOTAL_COUNT_KEY, ) +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.column.base.columnValueLengthsToBeBetween import ( BaseColumnValueLengthsToBeBetweenValidator, ) @@ -87,20 +89,13 @@ def compute_row_count(self, column: Column, min_bound: int, max_bound: int): NotImplementedError: """ row_count = self._compute_row_count(self.runner, column) - filters = [] - if min_bound > -math.inf: - filters.append((LenFn(column), "lt", min_bound)) - if max_bound < math.inf: - filters.append((LenFn(column), "gt", max_bound)) - failed_rows = self._compute_row_count_between( - self.runner, + failed_rows = self._count_rows_outside( + cast(QueryRunner, self.runner), # noqa: TC006 column, - { - "filters": filters, - "or_filter": True, - }, + LenFn(column), + min_bound, + max_bound, ) - return row_count, failed_rows def _build_dimension_metric_values(self, row, metrics_to_compute, test_params=None): @@ -166,9 +161,9 @@ def filter(self): min_bound = self.get_min_bound(self.MIN_BOUND) max_bound = self.get_max_bound(self.MAX_BOUND) filters = [] - if min_bound is not None and min_bound > float("-inf"): + if not is_unbounded(min_bound): filters.append((LenFn(self.get_column()), "lt", min_bound)) - if max_bound is not None and max_bound < float("inf"): + if not is_unbounded(max_bound): filters.append((LenFn(self.get_column()), "gt", max_bound)) return { "filters": filters, diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py index ba3c51592fb1..f7b1bb83e495 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeBetween.py @@ -13,7 +13,6 @@ Validator for column values to be between test case """ -import math from datetime import datetime from typing import cast @@ -23,6 +22,9 @@ DIMENSION_FAILED_COUNT_KEY, DIMENSION_TOTAL_COUNT_KEY, ) +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.column.base.columnValuesToBeBetween import ( BaseColumnValuesToBeBetweenValidator, ) @@ -145,20 +147,13 @@ def compute_row_count(self, column: Column, min_bound: int, max_bound: int): NotImplementedError: """ row_count = self._compute_row_count(self.runner, column) - filters = [] - if not isinstance(min_bound, (int, float)) or min_bound > -math.inf: - filters.append((column, "lt", min_bound)) - if not isinstance(max_bound, (int, float)) or max_bound < math.inf: - filters.append((column, "gt", max_bound)) - failed_rows = self._compute_row_count_between( - self.runner, + failed_rows = self._count_rows_outside( + cast(QueryRunner, self.runner), # noqa: TC006 + column, column, - { - "filters": filters, - "or_filter": True, - }, + min_bound, + max_bound, ) - return row_count, failed_rows def filter(self): @@ -186,9 +181,9 @@ def filter(self): max_bound = self.get_max_bound(self.MAX_BOUND) filters = [] - if min_bound is not None: + if not is_unbounded(min_bound): filters.append((column, "lt", min_bound)) - if max_bound is not None: + if not is_unbounded(max_bound): filters.append((column, "gt", max_bound)) return { "filters": filters, diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py index 7c95e717b42a..35ba462de23d 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeInSet.py @@ -13,8 +13,6 @@ Validator for column value to be in set test case """ -from ast import literal_eval - from sqlalchemy import Column, literal from metadata.data_quality.validations.base_test_handler import ( @@ -125,11 +123,7 @@ def compute_row_count(self, column: Column): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "allowedValues", - literal_eval, - ) + items = self.get_set_param_value("allowedValues") return { "filters": [ ( diff --git a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py index d6f0551ee2a6..cd2dcd3e741d 100644 --- a/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py +++ b/ingestion/src/metadata/data_quality/validations/column/sqlalchemy/columnValuesToBeNotInSet.py @@ -13,7 +13,6 @@ Validator for column value to be not in set test case """ -from ast import literal_eval from typing import cast from sqlalchemy import Column @@ -129,11 +128,7 @@ def compute_row_count(self, column: Column): return self._compute_row_count(self.runner, column) def filter(self): - items = self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "forbiddenValues", - literal_eval, - ) + items = self.get_set_param_value("forbiddenValues") return { "filters": [(self.get_column(), "in", items)], "or_filter": False, diff --git a/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py b/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py index 1dddd2225a03..55647123336a 100644 --- a/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py +++ b/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py @@ -49,6 +49,11 @@ class PandasValidatorMixin: runner: "PandasRunner" + def get_set_values_type(self) -> None: + """None: pandas infers column types from the values (strings "True"/"False" read as + BOOLEAN), so a set keeps the values as written; this also avoids loading a chunk.""" + return + def get_column(self: HasValidatorContext, column_name: str | None = None) -> SQALikeColumn: """Get column object for the given column name diff --git a/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py b/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py index 3d7c49b404f9..ad67b9809bd8 100644 --- a/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py +++ b/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py @@ -40,6 +40,9 @@ DIMENSION_TOTAL_COUNT_KEY, DIMENSION_VALUE_KEY, ) +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.impact_score import ( DEFAULT_TOP_DIMENSIONS, get_impact_score_expression, @@ -76,6 +79,10 @@ class DataQualityQueryType(Enum): class SQAValidatorMixin: """Validator mixin for SQA test cases""" + def get_set_values_type(self): + """The column type the engine declares, so a set compares in that type""" + return self.get_column().type # type: ignore + def get_column(self: HasValidatorContext, column_name: str | None = None) -> Column: """Get column object for the given column name @@ -235,6 +242,28 @@ def _compute_row_count_between( return res + def _count_rows_outside( + self, + runner: QueryRunner, + column: Column, + expression: ColumnElement, + min_bound: Any, + max_bound: Any, + ) -> int | None: + """Rows whose `expression` falls outside [min_bound, max_bound] + + An unbounded side excludes nothing. With both sides unbounded no row is outside, and a + count with no filter at all would count the whole table. + """ + filters = [ + (expression, operator, bound) + for operator, bound in (("lt", min_bound), ("gt", max_bound)) + if not is_unbounded(bound) + ] + return ( + self._compute_row_count_between(runner, column, {"filters": filters, "or_filter": True}) if filters else 0 + ) + def _compute_row_violations(self, runner: QueryRunner, violations_expr: ColumnElement) -> tuple[Any, Any]: """Count the rows read and the violating ones among them, in a single aggregate query diff --git a/ingestion/src/metadata/data_quality/validations/utils.py b/ingestion/src/metadata/data_quality/validations/utils.py index c9c107e4286b..ac489ae35c10 100644 --- a/ingestion/src/metadata/data_quality/validations/utils.py +++ b/ingestion/src/metadata/data_quality/validations/utils.py @@ -2,6 +2,8 @@ Data quality validation utility functions. """ +import json +from ast import literal_eval from collections.abc import Callable from typing import Any, TypeVar from urllib.parse import quote @@ -12,6 +14,7 @@ from sqlalchemy.engine import URL from metadata.generated.schema.tests.testCase import TestCaseParameterValue +from metadata.profiler.orm.registry import is_boolean from metadata.utils.logger import test_suite_logger logger = test_suite_logger() @@ -24,6 +27,34 @@ USERNAME_RESERVED_CHARACTERS = ":/?#" +def parse_set_values(raw: str, column_type) -> list: + """Parse an allowed or forbidden set; on a boolean column "true"/"false" strings become booleans + + Only "true"/"false" (any case) are mapped: "1"/"0", "t"/"f" and "yes"/"no" stay as written. + """ + values = _parse_literal(raw) + if not isinstance(values, (list, tuple, set)): + raise ValueError(f"A set of values must be a list, got {raw}") # noqa: TRY004 the set validators abort on ValueError + return [_as_boolean(value) for value in values] if is_boolean(column_type) else list(values) + + +def _parse_literal(raw: str) -> Any: + """A Python literal, or JSON when it is not one: JSON writes booleans and null in lowercase + + Where both parse a value they agree, so a set written either way reads the same. A value that + is neither raises ValueError (json.JSONDecodeError is one). + """ + try: + value = literal_eval(raw) + except (ValueError, SyntaxError): + value = json.loads(raw) + return value + + +def _as_boolean(value): + return {"true": True, "false": False}.get(value.lower(), value) if isinstance(value, str) else value + + def get_test_case_param_value( test_case_param_vals: list[TestCaseParameterValue], name: str, diff --git a/ingestion/src/metadata/profiler/orm/registry.py b/ingestion/src/metadata/profiler/orm/registry.py index bddd9a332875..be60810ec3b2 100644 --- a/ingestion/src/metadata/profiler/orm/registry.py +++ b/ingestion/src/metadata/profiler/orm/registry.py @@ -18,7 +18,7 @@ from enum import Enum import sqlalchemy -from sqlalchemy import Date, DateTime, Integer, Numeric, Time +from sqlalchemy import Boolean, Date, DateTime, Integer, Numeric, Time from sqlalchemy.sql.sqltypes import Concatenable, LargeBinary, Text from metadata.generated.schema.entity.data.table import DataType @@ -195,6 +195,13 @@ def is_date_time(_type) -> bool: ) +def is_boolean(_type) -> bool: + """ + Check if _type is a boolean, as a sqlalchemy type or an OpenMetadata DataType + """ + return _type == DataType.BOOLEAN if isinstance(_type, DataType) else isinstance(_type, Boolean) + + def is_quantifiable(_type) -> bool: """ Check if sqlalchemy _type is either integer or numeric diff --git a/ingestion/tests/unit/observability/data_quality/conftest.py b/ingestion/tests/unit/observability/data_quality/conftest.py index 4c7e9721f4ed..475f37b1488b 100644 --- a/ingestion/tests/unit/observability/data_quality/conftest.py +++ b/ingestion/tests/unit/observability/data_quality/conftest.py @@ -275,6 +275,32 @@ def test_case_column_value_length_to_be_between_no_min(): ) # type: ignore +@pytest.fixture +def test_case_column_value_length_to_be_between_no_bounds(): + """A lengths window with neither side set lets every row through""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_FNAME, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[], + computePassedFailedRowCount=True, + ) # type: ignore + + +@pytest.fixture +def test_case_column_values_to_be_between_no_bounds(): + """A values window with neither side set lets every row through""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_AGE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[], + computePassedFailedRowCount=True, + ) # type: ignore + + @pytest.fixture def test_case_column_value_max_to_be_between(): """Test case for test column_value_max_to_be_between""" @@ -866,6 +892,65 @@ def test_case_column_value_in_set_boolean(): ) +@pytest.fixture +def test_case_column_values_not_in_set_boolean_strings(): + """The forbidden set as the UI and generated tests write it: strings""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_IS_ACTIVE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="forbiddenValues", value='["true"]'), + ], + computePassedFailedRowCount=True, + ) + + +@pytest.fixture +def test_case_column_values_not_in_set_boolean_json(): + """The forbidden set as a generated test writes it: a JSON array with a lowercase boolean""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_IS_ACTIVE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="forbiddenValues", value="[false]"), + ], + computePassedFailedRowCount=True, + ) + + +@pytest.fixture +def test_case_column_value_in_set_missing_column(): + """A set test on a column the table no longer has""" + return TestCase( + name=TEST_CASE_NAME, + entityLink="<#E::table::service.db.users::columns::dropped_column>", + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="allowedValues", value='["John"]'), + ], + ) + + +@pytest.fixture +def test_case_column_value_in_set_boolean_strings(): + """The allowed set as the UI and generated tests write it: strings""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_IS_ACTIVE, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="allowedValues", value='["true", "false"]'), + ], + computePassedFailedRowCount=True, + ) + + @pytest.fixture def test_case_column_values_to_be_in_set_dimensional_match_enum(): return TestCase( diff --git a/ingestion/tests/unit/observability/data_quality/test_validations_databases.py b/ingestion/tests/unit/observability/data_quality/test_validations_databases.py index 4e187db2af83..f5d57a41b917 100644 --- a/ingestion/tests/unit/observability/data_quality/test_validations_databases.py +++ b/ingestion/tests/unit/observability/data_quality/test_validations_databases.py @@ -42,6 +42,11 @@ "tableCustomSQLQuery", } +ABORTED_BEFORE_ROW_COUNT = { + "test_case_table_custom_sql_unsafe_query_aborted", + "test_case_column_value_in_set_missing_column", +} + # pylint: disable=line-too-long @pytest.mark.parametrize( @@ -109,6 +114,13 @@ (TestCaseResult, None, None, TestCaseStatus.Success, 80.0, 0.0, 100.0, 0.0), None, ), + ( + "test_case_column_value_length_to_be_between_no_bounds", + "columnValueLengthsToBeBetween", + "COLUMN", + (TestCaseResult, None, None, TestCaseStatus.Success, 80.0, 0.0, 100.0, 0.0), + None, + ), ( "test_case_column_value_max_to_be_between", "columnValueMaxToBeBetween", @@ -290,6 +302,45 @@ ), None, ), + ( + "test_case_column_value_in_set_missing_column", + "columnValuesToBeInSet", + "COLUMN", + (TestCaseResult, None, None, TestCaseStatus.Aborted, None, None, None, None), + None, + ), + ( + "test_case_column_values_not_in_set_boolean_strings", + "columnValuesToBeNotInSet", + "COLUMN", + ( + TestCaseResult, + "40", + None, + TestCaseStatus.Failed, + 40.0, + 40.0, + 50.0, + 50.0, + ), + None, + ), + ( + "test_case_column_values_not_in_set_boolean_json", + "columnValuesToBeNotInSet", + "COLUMN", + ( + TestCaseResult, + "30", + None, + TestCaseStatus.Failed, + 50.0, + 30.0, + 62.5, + 37.5, + ), + None, + ), ( "test_case_column_sum_to_be_between", "columnValuesSumToBeBetween", @@ -322,6 +373,13 @@ ), None, ), + ( + "test_case_column_values_to_be_between_no_bounds", + "columnValuesToBeBetween", + "COLUMN", + (TestCaseResult, "30", None, TestCaseStatus.Success, 80, 0, 100, 0), + None, + ), ( "test_case_column_values_to_be_not_null", "columnValuesToBeNotNull", @@ -536,6 +594,22 @@ ), None, ), + ( + "test_case_column_value_in_set_boolean_strings", + "columnValuesToBeInSet", + "COLUMN", + ( + TestCaseResult, + "70", + None, + TestCaseStatus.Success, + 70.0, + 10.0, + 87.5, + 12.5, + ), + None, + ), ( "test_case_column_values_to_be_in_set_dimensional_match_enum", "columnValuesToBeInSet", @@ -1114,10 +1188,7 @@ def test_suite_validation_database( # noqa: C901 if failed_percentage: assert round(res.failedRowsPercentage, 2) == failed_percentage assert res.testCaseStatus == status - if ( - test_case_type in TEST_CASE_SUPPORT_ROW_LEVEL_PASS_FAILED - and test_case_name != "test_case_table_custom_sql_unsafe_query_aborted" - ): + if test_case_type in TEST_CASE_SUPPORT_ROW_LEVEL_PASS_FAILED and test_case_name not in ABORTED_BEFORE_ROW_COUNT: assert res.failedRows is not None assert res.failedRowsPercentage is not None assert res.passedRows is not None diff --git a/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py b/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py index 2936ba695a24..ccb64491f197 100644 --- a/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py +++ b/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py @@ -16,6 +16,7 @@ """ from datetime import datetime, timedelta +from uuid import uuid4 import pytest from freezegun import freeze_time @@ -23,6 +24,8 @@ from metadata.data_quality.validations.base_test_handler import PandasRunner from metadata.generated.schema.tests.basic import TestCaseResult, TestCaseStatus +from metadata.generated.schema.tests.testCase import TestCase, TestCaseParameterValue +from metadata.generated.schema.type.entityReference import EntityReference from metadata.utils.importer import import_test_case_class TEST_CASE_SUPPORT_ROW_LEVEL_PASS_FAILED = { @@ -1294,3 +1297,47 @@ def test_suite_validation_datalake( assert dim.failedRowsPercentage == expected_dim[5] assert dim.impactScore == expected_dim[6] + + +@pytest.mark.parametrize( + "test_case_type,parameter_values,status,failed_rows", + [ + ( + "columnValuesToBeInSet", + [ + TestCaseParameterValue(name="allowedValues", value='["True", "False"]'), + TestCaseParameterValue(name="matchEnum", value="true"), + ], + TestCaseStatus.Success, + 0, + ), + ( + "columnValuesToBeNotInSet", + [TestCaseParameterValue(name="forbiddenValues", value='["False"]')], + TestCaseStatus.Failed, + 1, + ), + ], +) +def test_string_booleans_on_a_datalake_column_stay_strings(test_case_type, parameter_values, status, failed_rows): + """Pandas infers a column of "True"/"False" strings as BOOLEAN; its sets must still compare as strings""" + test_case = TestCase( + name="string_booleans", + entityLink="<#E::table::service.db.users::columns::flag>", + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=parameter_values, + computePassedFailedRowCount=True, + ) # type: ignore + validator = import_test_case_class( + "COLUMN", "pandas", test_case_type, f"{test_case_type[0].upper()}{test_case_type[1:]}Validator" + )( + PandasRunner(dataset=lambda: iter((DataFrame({"flag": ["True", "False", "True", "True"]}),)), raw_dataset=None), + test_case=test_case, + execution_date=EXECUTION_DATE.timestamp(), + ) + + res = validator.run_validation() + + assert res.testCaseStatus == status + assert res.failedRows == failed_rows diff --git a/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py b/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py index 8c799e4348c5..f351ba7236db 100644 --- a/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py +++ b/ingestion/tests/unit/observability/data_quality/validations/test_between_row_violations.py @@ -360,6 +360,43 @@ def test_an_unset_bound_excludes_nothing(pandas_runner, min_bound, max_bound, vi assert counted == violations +@pytest.mark.parametrize( + "validator_cls, entity_link", + [ + (SQAValuesValidator, ENTITY_LINK_VALUE), + (SQALengthsValidator, ENTITY_LINK_LABEL), + ], +) +def test_sqa_an_unbounded_window_fails_no_row(sqa_runner, validator_cls, entity_link): + """With neither bound set nothing is outside the window, so no row may be reported failed""" + validator = validator_cls(sqa_runner, _test_case(entity_link, [], None, None), EXECUTION_DATE.timestamp()) + + assert validator.compute_row_count(validator.get_column(), float("-inf"), float("inf")) == (EXPECTED_ROWS, 0) + + +def test_sqa_one_open_side_counts_only_the_other(sqa_runner): + """Only the lower bound set: the rows below it fail, and no upper side is invented""" + validator = SQAValuesValidator( + sqa_runner, _test_case(ENTITY_LINK_VALUE, [], None, None), EXECUTION_DATE.timestamp() + ) + + assert validator.compute_row_count(validator.get_column(), MIN_BOUND, float("inf")) == (EXPECTED_ROWS, 1) + + +@pytest.mark.parametrize( + "validator_cls, entity_link", + [ + (SQAValuesValidator, ENTITY_LINK_VALUE), + (SQALengthsValidator, ENTITY_LINK_LABEL), + ], +) +def test_sqa_an_unbounded_window_samples_no_filter(sqa_runner, validator_cls, entity_link): + """The failed-row sample is drawn with the same window, so an open window filters nothing""" + validator = validator_cls(sqa_runner, _test_case(entity_link, [], None, None), EXECUTION_DATE.timestamp()) + + assert validator.filter()["filters"] == [] + + def test_the_window_is_never_widened_by_the_threshold(sqa_runner): """The tolerance is spent on rows here, so spending it on the bounds too would double it""" validator = SQAValuesValidator(sqa_runner, values_test_case(threshold=5), EXECUTION_DATE.timestamp()) diff --git a/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py b/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py index a1c49f7f6dee..37795fd9cc46 100644 --- a/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py +++ b/ingestion/tests/unit/observability/data_quality/validations/test_row_threshold.py @@ -18,7 +18,7 @@ from uuid import uuid4 import pytest -from sqlalchemy import event +from sqlalchemy import Column, Integer, MetaData, String, Table, event from sqlalchemy.exc import SQLAlchemyError from metadata.data_quality.validations.base_test_handler import FailureThreshold, ThresholdUnit @@ -64,10 +64,14 @@ def build_test_case(parameter_values, compute_passed_failed_row_count=False, ent def build_validator(validator_class, parameter_values, compute_passed_failed_row_count=False): - """Build a validator whose only live dependency is its test case""" - return validator_class( - MagicMock(), build_test_case(parameter_values, compute_passed_failed_row_count), EXECUTION_DATE - ) + """Build a validator whose only live dependencies are its test case and the declared column + + A set validator parses its values in the column's declared type, so the runner declares the + table; nothing is ever queried. + """ + runner = MagicMock() + runner.dataset = Table("users", MetaData(), Column("nickname", String), Column("age", Integer)) + return validator_class(runner, build_test_case(parameter_values, compute_passed_failed_row_count), EXECUTION_DATE) @contextmanager diff --git a/ingestion/tests/unit/observability/data_quality/validations/test_utils.py b/ingestion/tests/unit/observability/data_quality/validations/test_utils.py index 4b4ce41a4248..4941dae27ba2 100644 --- a/ingestion/tests/unit/observability/data_quality/validations/test_utils.py +++ b/ingestion/tests/unit/observability/data_quality/validations/test_utils.py @@ -1,7 +1,16 @@ +from datetime import datetime + import pytest +import sqlalchemy as sqa +from metadata.data_quality.validations import utils +from metadata.data_quality.validations.checkers.between_bounds_checker import ( + is_unbounded, +) from metadata.data_quality.validations.utils import get_bool_test_case_param +from metadata.generated.schema.entity.data.table import DataType from metadata.generated.schema.tests.testCase import TestCaseParameterValue +from metadata.profiler.orm.registry import is_boolean @pytest.mark.parametrize( @@ -20,3 +29,62 @@ ) def test_get_bool_test_case_param(test_case_param_vals, name, expected): assert get_bool_test_case_param(test_case_param_vals, name) == expected + + +@pytest.mark.parametrize( + "bound, unbounded", + [ + (None, True), + (float("-inf"), True), + (float("inf"), True), + (3, False), + (3.5, False), + (datetime(2021, 7, 1), False), + ], +) +def test_is_unbounded(bound, unbounded): + assert is_unbounded(bound) is unbounded + + +@pytest.mark.parametrize("column_type", [sqa.Boolean(), DataType.BOOLEAN]) +def test_true_and_false_strings_become_booleans_on_a_boolean_column(column_type): + assert utils.parse_set_values(raw='["true", "False"]', column_type=column_type) == [True, False] + + +@pytest.mark.parametrize("column_type", [sqa.Boolean(), DataType.BOOLEAN]) +def test_python_literals_on_a_boolean_column_are_kept(column_type): + assert utils.parse_set_values(raw="[True, False]", column_type=column_type) == [True, False] + + +@pytest.mark.parametrize("column_type", [sqa.String(), DataType.VARCHAR, None]) +def test_strings_on_a_string_or_unknown_column_stay_strings(column_type): + assert utils.parse_set_values(raw='["true", "false"]', column_type=column_type) == ["true", "false"] + + +@pytest.mark.parametrize( + "raw, column_type, expected", + [ + ("[false]", sqa.Boolean(), [False]), + ("[true, false]", DataType.BOOLEAN, [True, False]), + ("[null]", sqa.String(), [None]), + ('["a", 1.5]', None, ["a", 1.5]), + ], +) +def test_a_json_written_set_is_read_like_a_python_one(raw, column_type, expected): + """Generated tests and API clients write JSON, where booleans and null are lowercase""" + assert utils.parse_set_values(raw=raw, column_type=column_type) == expected + + +def test_a_set_that_is_neither_python_nor_json_is_rejected(): + with pytest.raises(ValueError): + utils.parse_set_values(raw="[OPEN, CLOSED]", column_type=sqa.String()) + + +def test_a_set_that_is_not_a_collection_is_rejected(): + with pytest.raises(ValueError, match="must be a list"): + utils.parse_set_values(raw='"true"', column_type=sqa.Boolean()) + + +def test_is_boolean_reads_both_type_systems(): + assert is_boolean(sqa.Boolean()) and is_boolean(DataType.BOOLEAN) + assert not is_boolean(sqa.Integer()) and not is_boolean(DataType.INT) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java index 168212196ed3..d381dc72f1f9 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java @@ -894,6 +894,61 @@ void post_testWithInvalidParameterValue_4xx(TestNamespace ns) { Exception.class, () -> createEntity(request), "Should fail with invalid parameter name"); } + @Test + void post_testWithUnofferedOptionValue_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("bad_strategy")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + ">"); + request.setTestDefinition("tableCustomSQLQuery"); + request.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("SUM"))); + + Exception error = assertThrows(Exception.class, () -> createEntity(request)); + assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); + } + + @Test + void post_testOnUnsupportedColumnType_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("length_on_number")); + // createTable's `id` column is INT, which columnValueLengthsToBeBetween does not support. + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + + Exception error = assertThrows(Exception.class, () -> createEntity(request)); + assertTrue(error.getMessage().contains("does not support column type"), error.getMessage()); + } + + @Test + void patch_doesNotRecheckDefinitionConformance(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("patch_gate")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::name>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + TestCase testCase = createEntity(request); + + // The stored test now targets a column type its definition does not support, as a row created + // before the create-time check, or one whose column was retyped by ingestion, does. + table.getColumns().stream() + .filter(column -> column.getName().equals("name")) + .forEach(column -> column.withDataType(ColumnDataType.INT).withDataLength(null)); + SdkClients.adminClient().tables().update(table.getId().toString(), table); + + testCase.setDescription("edited"); + TestCase updated = patchEntity(testCase.getId().toString(), testCase); + + assertEquals("edited", updated.getDescription()); + } + @Test void test_testCaseWithOwner(TestNamespace ns) { OpenMetadataClient client = SdkClients.adminClient(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java new file mode 100644 index 000000000000..27ff9f0f74de --- /dev/null +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java @@ -0,0 +1,95 @@ +package org.openmetadata.service.jdbi3; + +import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; +import static org.openmetadata.common.utils.CommonUtil.nullOrEmpty; + +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.stream.Collectors; +import org.openmetadata.schema.entity.data.Table; +import org.openmetadata.schema.tests.TestCase; +import org.openmetadata.schema.tests.TestCaseParameter; +import org.openmetadata.schema.tests.TestCaseParameterValue; +import org.openmetadata.schema.tests.TestDefinition; +import org.openmetadata.schema.type.Column; +import org.openmetadata.schema.type.ColumnDataType; +import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; + +/** + * What a new test case must agree with in its definition + * (ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create). + */ +final class TestCaseDefinitionConformance { + private TestCaseDefinitionConformance() {} + + static void validate( + TestCase test, TestDefinition definition, Table table, EntityLink entityLink) { + validateOptionValues( + listOrEmpty(test.getParameterValues()), listOrEmpty(definition.getParameterDefinition())); + validateColumnType(definition, table, entityLink); + } + + private static void validateOptionValues( + List values, List parameters) { + Map> options = optionsByParameter(parameters); + values.stream() + .filter(value -> options.containsKey(value.getName()) && !isUnset(value.getValue())) + .filter(value -> !isOption(value.getValue(), options.get(value.getName()))) + .findFirst() + .ifPresent( + value -> { + throw new IllegalArgumentException( + String.format( + "Parameter '%s' must be one of %s, but was '%s'.", + value.getName(), options.get(value.getName()), value.getValue())); + }); + } + + private static Map> optionsByParameter(List parameters) { + return parameters.stream() + .filter(parameter -> !nullOrEmpty(parameter.getOptionValues())) + .collect( + Collectors.toMap( + TestCaseParameter::getName, + TestCaseParameter::getOptionValues, + (first, duplicate) -> first)); + } + + /** A blank value is unset, as the test generator reads it, so it is not checked here. */ + private static boolean isUnset(String value) { + return nullOrEmpty(value) || value.isBlank(); + } + + // Case-insensitive, as TestCaseThresholdValidator already reads thresholdUnit. + private static boolean isOption(String value, List options) { + return options.stream().anyMatch(option -> String.valueOf(option).equalsIgnoreCase(value)); + } + + private static void validateColumnType( + TestDefinition definition, Table table, EntityLink entityLink) { + List supported = listOrEmpty(definition.getSupportedDataTypes()); + Optional type = + column(table, entityLink.getArrayFieldName()).map(Column::getDataType); + boolean unsupported = + !supported.isEmpty() && type.isPresent() && !supported.contains(type.get()); + if (unsupported) { + throw new IllegalArgumentException( + String.format( + "Test definition '%s' does not support column type %s. Supported types are: %s", + definition.getName(), type.get(), supported)); + } + } + + // validateColumnTestCase matches the column name case-insensitively; this must find the same one. + private static Optional column(Table table, String name) { + return Optional.ofNullable(name) + .flatMap( + columnName -> + listOrEmpty(table.getColumns()).stream() + .filter(Objects::nonNull) + .filter(candidate -> candidate.getName().equalsIgnoreCase(columnName)) + .findFirst()); + } +} diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java index 8a714f862771..36a2d3118843 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java @@ -898,6 +898,10 @@ public void prepare(TestCase test, boolean update) { testDefinition.getTestPlatforms(), testDefinition); validateColumnTestCase(table, entityLink, testDefinition.getEntityType()); + // Same gate as the disabled-definition check: a row stored before these rules stays editable. + if (!update) { + TestCaseDefinitionConformance.validate(test, testDefinition, table, entityLink); + } validateDimensionColumns(test, table); setDataQualityDimension(test, testDefinition); diff --git a/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json b/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json index 93dab7e88294..05a7e9e0ad27 100644 --- a/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json +++ b/openmetadata-service/src/main/resources/json/data/tests/tableCustomSQLQuery.json @@ -16,7 +16,7 @@ { "name": "strategy", "displayName": "Strategy", - "description": "Strategy to use to run the custom SQL query (i.e. `SELECT COUNT()` or `SELECT (defaults to ROWS)", + "description": "How the query result is compared to the threshold. ROWS (default): the query returns the rows that break the rule, such as `SELECT * FROM t WHERE amount < 0`, and the number of rows returned is compared. COUNT: the query returns a single integer, such as `SELECT COUNT(*) FROM t WHERE amount < 0`, and that number is compared. Under ROWS a `SELECT COUNT(*)` query always returns one row. Do not use COUNT with a query that can return NULL or a non-integer, such as SUM or AVG.", "dataType": "ARRAY", "optionValues": ["ROWS", "COUNT"], "required": false diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SeededTestDefinitions.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SeededTestDefinitions.java new file mode 100644 index 000000000000..aa36b0da6bc0 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SeededTestDefinitions.java @@ -0,0 +1,27 @@ +package org.openmetadata.service.jdbi3; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Pattern; +import org.openmetadata.common.utils.CommonUtil; +import org.openmetadata.schema.tests.TestDefinition; +import org.openmetadata.schema.utils.JsonUtils; + +/** The test definitions as they are seeded, read straight off the classpath. */ +final class SeededTestDefinitions { + private SeededTestDefinitions() {} + + static List all() throws IOException { + List definitions = new ArrayList<>(); + for (String resource : + CommonUtil.getResources(Pattern.compile(".*json/data/tests/.*\\.json$"))) { + definitions.add( + JsonUtils.readValue( + CommonUtil.getResourceAsStream( + SeededTestDefinitions.class.getClassLoader(), resource), + TestDefinition.class)); + } + return definitions; + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java new file mode 100644 index 000000000000..71ce1de95b03 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java @@ -0,0 +1,171 @@ +package org.openmetadata.service.jdbi3; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.openmetadata.schema.entity.data.Table; +import org.openmetadata.schema.tests.TestCase; +import org.openmetadata.schema.tests.TestCaseParameter; +import org.openmetadata.schema.tests.TestCaseParameterValue; +import org.openmetadata.schema.tests.TestDefinition; +import org.openmetadata.schema.type.Column; +import org.openmetadata.schema.type.ColumnDataType; +import org.openmetadata.schema.type.TestDefinitionEntityType; +import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; + +class TestCaseDefinitionConformanceTest { + + private static final Table TABLE = + new Table() + .withColumns( + List.of( + new Column().withName("Amount").withDataType(ColumnDataType.NUMERIC), + new Column().withName("note").withDataType(ColumnDataType.VARCHAR), + new Column().withName("untyped"))); + + private static final TestDefinition CUSTOM_SQL = + new TestDefinition() + .withName("tableCustomSQLQuery") + .withEntityType(TestDefinitionEntityType.TABLE) + .withParameterDefinition( + List.of( + new TestCaseParameter() + .withName("strategy") + .withOptionValues(List.of("ROWS", "COUNT")))); + + private static final TestDefinition LENGTHS = + new TestDefinition() + .withName("columnValueLengthsToBeBetween") + .withEntityType(TestDefinitionEntityType.COLUMN) + .withSupportedDataTypes(List.of(ColumnDataType.VARCHAR, ColumnDataType.STRING)) + .withSupportsDynamicAssertion(true); + + private static EntityLink table() { + return EntityLink.parse("<#E::table::svc.db.sch.orders>"); + } + + private static EntityLink column(String name) { + return EntityLink.parse("<#E::table::svc.db.sch.orders::columns::" + name + ">"); + } + + private static TestCase withParameter(String name, String value) { + return new TestCase() + .withParameterValues(List.of(new TestCaseParameterValue().withName(name).withValue(value))); + } + + @Test + void anOptionValueIsAccepted() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", "COUNT"), CUSTOM_SQL, TABLE, table())); + } + + @Test + void anOptionValueInAnotherCaseIsAcceptedAsThresholdUnitAlreadyIs() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", "count"), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aValueThatIsNoneOfTheOptionsIsRejected() { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", "[\"ROWS\"]"), CUSTOM_SQL, TABLE, table())); + assertTrue(error.getMessage().contains("must be one of [ROWS, COUNT]"), error.getMessage()); + } + + @Test + void aBlankOptionalValueIsTreatedAsUnset() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", ""), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aWhitespaceValueIsTreatedAsUnsetAsTheGeneratorReadsIt() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("strategy", " "), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aParameterTheDefinitionGivesNoOptionsIsNotChecked() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + withParameter("sqlExpression", "SELECT 1"), CUSTOM_SQL, TABLE, table())); + } + + @Test + void aColumnOfAnUnsupportedTypeIsRejectedWhateverTheNameCase() { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validate( + new TestCase(), LENGTHS, TABLE, column("amount"))); + assertTrue(error.getMessage().contains("NUMERIC"), error.getMessage()); + } + + @Test + void aColumnOfASupportedTypeIsAccepted() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate(new TestCase(), LENGTHS, TABLE, column("note"))); + } + + @Test + void aColumnOfUnknownTypeIsAccepted() { + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + new TestCase(), LENGTHS, TABLE, column("untyped"))); + } + + @Test + void anEmptySupportedTypeListAcceptsEveryColumn() { + TestDefinition anyType = + new TestDefinition() + .withName("columnValuesToBeNotNull") + .withEntityType(TestDefinitionEntityType.COLUMN); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validate( + new TestCase(), anyType, TABLE, column("Amount"))); + } + + @Test + void aTableTestIsNotCheckedAgainstColumnTypes() { + assertDoesNotThrow( + () -> TestCaseDefinitionConformance.validate(new TestCase(), LENGTHS, TABLE, table())); + } + + /** + * ai-platform reads a bound as a parameter with a validationRule; every dynamic seed must mark + * its bounds so. + */ + @Test + void everySeededDynamicDefinitionMarksItsBoundsWithAValidationRule() throws IOException { + for (TestDefinition definition : SeededTestDefinitions.all()) { + if (Boolean.TRUE.equals(definition.getSupportsDynamicAssertion())) { + long bounds = + definition.getParameterDefinition().stream() + .filter(parameter -> parameter.getValidationRule() != null) + .count(); + assertTrue(bounds >= 2, definition.getName() + " has " + bounds + " ruled bounds"); + } + } + } +} diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java index e6859905213e..b75cfba7d81e 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseThresholdValidatorTest.java @@ -5,23 +5,19 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; -import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeSet; -import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.MethodSource; -import org.openmetadata.common.utils.CommonUtil; import org.openmetadata.schema.tests.TestCaseParameter; import org.openmetadata.schema.tests.TestDefinition; import org.openmetadata.schema.type.TestCaseParameterDataType; -import org.openmetadata.schema.utils.JsonUtils; class TestCaseThresholdValidatorTest { @@ -476,20 +472,6 @@ void leavesTableCustomSqlQueryComparisonBoundsAlone() { /** Definitions that declare `thresholdUnit` without their `threshold` being a tolerance. */ private static final Set NOT_A_TOLERANCE = Set.of("tableCustomSQLQuery"); - /** The definitions as they are seeded, read straight off the classpath. */ - private static List seededDefinitions() throws IOException { - List definitions = new ArrayList<>(); - for (String resource : - CommonUtil.getResources(Pattern.compile(".*json/data/tests/.*\\.json$"))) { - definitions.add( - JsonUtils.readValue( - CommonUtil.getResourceAsStream( - TestCaseThresholdValidatorTest.class.getClassLoader(), resource), - TestDefinition.class)); - } - return definitions; - } - /** * The classification is a named list, so a newly seeded definition that opts in to failure * thresholds has to be placed in one of the three buckets rather than silently inheriting @@ -498,7 +480,7 @@ private static List seededDefinitions() throws IOException { @Test void everySeededThresholdDefinitionIsClassified() throws IOException { Set optedIn = - seededDefinitions().stream() + SeededTestDefinitions.all().stream() .filter( definition -> definition.getParameterDefinition().stream() @@ -519,7 +501,7 @@ void everySeededThresholdDefinitionIsClassified() throws IOException { /** The seeded definitions, as shipped, land on the side of the cap this test declares. */ @Test void theSeededDefinitionsAreCappedAccordingToTheirClass() throws IOException { - for (TestDefinition definition : seededDefinitions()) { + for (TestDefinition definition : SeededTestDefinitions.all()) { String name = definition.getName(); // `matchEnum` has its own rule; satisfy it so this test only exercises the cap. Map aboveTheCap = diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts index 08193e47c19c..b064ad163d6d 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Entity.spec.ts @@ -1764,20 +1764,20 @@ pickEntityMatrix( const testCase1 = await tableEntity.createTestCase(apiContext, { name: `pw_col_dq_success_${uuid()}`, entityLink: `<#E::table::${tableFQN}::columns::${columnName}>`, - testDefinition: 'columnValueLengthsToBeBetween', + testDefinition: 'columnValuesToBeBetween', parameterValues: [ - { name: 'minLength', value: 1 }, - { name: 'maxLength', value: 10 }, + { name: 'minValue', value: 1 }, + { name: 'maxValue', value: 10 }, ], }); const testCase2 = await tableEntity.createTestCase(apiContext, { name: `pw_col_dq_failed_${uuid()}`, entityLink: `<#E::table::${tableFQN}::columns::${columnName}>`, - testDefinition: 'columnValueLengthsToBeBetween', + testDefinition: 'columnValuesToBeBetween', parameterValues: [ - { name: 'minLength', value: 100 }, - { name: 'maxLength', value: 200 }, + { name: 'minValue', value: 100 }, + { name: 'maxValue', value: 200 }, ], }); @@ -1903,10 +1903,10 @@ pickEntityMatrix( const testCase = await tableEntity.createTestCase(apiContext, { name: `pw_col_incident_${uuid()}`, entityLink: `<#E::table::${tableFQN}::columns::${columnName}>`, - testDefinition: 'columnValueLengthsToBeBetween', + testDefinition: 'columnValuesToBeBetween', parameterValues: [ - { name: 'minLength', value: 100 }, - { name: 'maxLength', value: 200 }, + { name: 'minValue', value: 100 }, + { name: 'maxValue', value: 200 }, ], }); From d6215060c012a2f5c82274d3d3be8a5cb6703317 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 16:59:36 +0530 Subject: [PATCH 02/12] refactor(dq): call is_unbounded directly in the between checker --- .../checkers/between_bounds_checker.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py b/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py index 7fa3ab6fd315..41861015e365 100644 --- a/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py +++ b/ingestion/src/metadata/data_quality/validations/checkers/between_bounds_checker.py @@ -44,9 +44,6 @@ def __init__(self, min_bound: float, max_bound: float): self.min_bound = min_bound self.max_bound = max_bound - def _is_unbounded(self, bound: Any) -> bool: - return is_unbounded(bound) - def _check_violations(self, values): """Core violation check logic - works for both scalar and Series. @@ -65,9 +62,9 @@ def _check_violations(self, values): # builds only those conditions: an unset bound excludes nothing, and a None one cannot # be compared against at all. outside = None - if not self._is_unbounded(self.min_bound): + if not is_unbounded(self.min_bound): outside = values < self.min_bound - if not self._is_unbounded(self.max_bound): + if not is_unbounded(self.max_bound): above = values > self.max_bound outside = above if outside is None else (outside | above) @@ -112,9 +109,9 @@ def build_violation_sqa(self, metrics: list["ClauseElement"]) -> "ClauseElement" for expr in metrics: expr_conditions = [] - if not self._is_unbounded(self.min_bound): + if not is_unbounded(self.min_bound): expr_conditions.append(and_(expr.isnot(None), expr < self.min_bound)) - if not self._is_unbounded(self.max_bound): + if not is_unbounded(self.max_bound): expr_conditions.append(and_(expr.isnot(None), expr > self.max_bound)) if expr_conditions: @@ -142,9 +139,9 @@ def build_row_level_violations_sqa(self, column: "ClauseElement") -> "ColumnElem # Build condition: value NOT NULL AND (value < min OR value > max) conditions = [] - if not self._is_unbounded(self.min_bound): + if not is_unbounded(self.min_bound): conditions.append(and_(column.isnot(None), column < self.min_bound)) - if not self._is_unbounded(self.max_bound): + if not is_unbounded(self.max_bound): conditions.append(and_(column.isnot(None), column > self.max_bound)) if not conditions: From d03d713f738c42d530d6fc15451604ea87fb1f29 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 17:22:13 +0530 Subject: [PATCH 03/12] create the orphan-cleanup test case with a definition its column supports --- .../it/tests/OrphanedTimeSeriesCleanupIT.java | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java index 57ad6bcc39a4..d9ec78744054 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/OrphanedTimeSeriesCleanupIT.java @@ -55,7 +55,6 @@ import org.openmetadata.schema.type.ColumnDataType; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.fluent.AIApplications; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.sdk.network.HttpMethod; import org.openmetadata.sdk.network.RequestOptions; import org.openmetadata.service.Entity; @@ -334,18 +333,13 @@ private Table createTable(TestNamespace ns, String prefix) throws Exception { private TestCase createTestCase(Table table, String name) throws Exception { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. CreateTestCase createTestCase = new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn); + .withTestDefinition("columnValuesToBeNotNull"); return client.testCases().create(createTestCase); } From c9258bb57557166c387365a3dc2ec5cd47bf7f1e Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 19:04:46 +0530 Subject: [PATCH 04/12] docs(dq): say a non-list set value is newly rejected --- ...26-10-16-a-set-parameter-is-read-in-its-column-type.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md index baf896661dc6..e04e670dfe39 100644 --- a/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md +++ b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md @@ -18,9 +18,11 @@ Databricks refuses to compare with a boolean. ## Decision `parse_set_values` reads a set as a Python literal and, when it is not one, as JSON; where both -parse a value they agree, so nothing that parsed before reads differently. A value that is neither, -or that is not a list, raises ValueError and the test is Aborted, as before. On a column whose -engine type is boolean, `"true"` / `"false"` in any case become booleans; nothing else is mapped. +parse a value they agree, so nothing that parsed before reads differently. A value that is neither +raises ValueError and the test is Aborted, as before. A value that parses but is not a list (a bare +string or number) is now rejected the same way, rather than compared as a single value. On a +column whose engine type is boolean, `"true"` / `"false"` in any case become booleans; nothing +else is mapped. The column type is the engine's (`SQAValidatorMixin.get_set_values_type`). Pandas returns none, so a datalake set keeps its values as written: pandas infers column types from the values, and a From d9b5c5d93fc600d2c2b9392dae3a83ed266153fa Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 16 Oct 2026 13:57:10 +0000 Subject: [PATCH 05/12] chore(playwright): auto-refresh impact-map.generated.json --- .github/playwright/impact-map.generated.json | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/.github/playwright/impact-map.generated.json b/.github/playwright/impact-map.generated.json index 098342271d1b..9220db711969 100644 --- a/.github/playwright/impact-map.generated.json +++ b/.github/playwright/impact-map.generated.json @@ -804,6 +804,7 @@ "playwright/e2e/Features/DataQuality/Profiler.spec.ts", "playwright/e2e/Features/DataQuality/ProfilerIngestionForm.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseImportExportBasic.spec.ts", @@ -1721,6 +1722,7 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", @@ -2035,6 +2037,7 @@ "playwright/e2e/Features/DataAssetRulesEnabled.spec.ts", "playwright/e2e/Features/DataQuality/ProfilerIngestionForm.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/ExploreQuickFilters.spec.ts", "playwright/e2e/Features/IngestionListNameSorting.spec.ts", "playwright/e2e/Features/LandingPageWidgets/LandingPagePlatformHealth.spec.ts", @@ -3085,6 +3088,7 @@ "playwright/e2e/Features/DataQuality/Profiler.spec.ts", "playwright/e2e/Features/DataQuality/ProfilerIngestionForm.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseImportExportBasic.spec.ts", @@ -3307,6 +3311,7 @@ "playwright/e2e/Features/AdvancedSearch.spec.ts", "playwright/e2e/Features/ContextCenterDashboard.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerSqlQuery.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DomainFilterQueryFilter.spec.ts", "playwright/e2e/Features/LandingPageWidgets/LandingPageContextCenter.spec.ts", "playwright/e2e/Features/LandingPageWidgets/LandingPageCriticalAnnouncements.spec.ts", @@ -3497,6 +3502,7 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", @@ -4113,6 +4119,7 @@ "playwright/e2e/Features/DataQuality/DataQualityListAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/DataQualityPermissions.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerDateFilter.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseImportExportBasic.spec.ts", @@ -4845,6 +4852,7 @@ "playwright/e2e/Features/AIMode/ConnectionServiceDetails.spec.ts", "playwright/e2e/Features/Announcements/SystemAnnouncements.spec.ts", "playwright/e2e/Features/BulkEditEntity.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/OnlineUsers.spec.ts", "playwright/e2e/Features/TestSuitePipelineRedeploy.spec.ts", "playwright/e2e/Pages/AIProfile/ProfileMembers.spec.ts", @@ -5173,6 +5181,7 @@ "playwright/e2e/Features/DataQuality/Profiler.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", @@ -5281,6 +5290,7 @@ "playwright/e2e/Features/DataQuality/DataQuality.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerDateFilter.spec.ts", "playwright/e2e/Features/DataQuality/IncidentManagerSqlQuery.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/EntitySuggestions.spec.ts", "playwright/e2e/Features/FailedTestCaseSampleData.spec.ts", @@ -6794,6 +6804,14 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts" ] }, + { + "sources": [ + "openmetadata-ui/src/main/resources/ui/src/components/Database/Profiler/TableProfiler/QualityTab/QualityTab.component.tsx" + ], + "specs": [ + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts" + ] + }, { "sources": [ "openmetadata-ui/src/main/resources/ui/src/components/Database/Profiler/TestSummary/RunSummaryTiles/RunSummaryTiles.tsx" @@ -12000,6 +12018,7 @@ "playwright/e2e/Features/DataQuality/ProfilerSettingsAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/RunTestCase.spec.ts", "playwright/e2e/Features/DataQuality/TableLevelTests.spec.ts", + "playwright/e2e/Features/DataQuality/TableTestCaseExportFilters.spec.ts", "playwright/e2e/Features/DataQuality/TableTestCasePagination.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseCrudAfterReindex.spec.ts", "playwright/e2e/Features/DataQuality/TestCaseDetailsPage.spec.ts", From 6d08473e7b9955696f548643facdd3946e6eaf32 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 19:41:23 +0530 Subject: [PATCH 06/12] fix(dq): read tableCustomSQLQuery strategy in any case and treat blank as unset --- .../table/base/tableCustomSQLQuery.py | 20 +++++++++--- .../table/sqlalchemy/tableCustomSQLQuery.py | 7 ----- .../observability/data_quality/conftest.py | 31 +++++++++++++++++++ .../test_validations_databases.py | 14 +++++++++ 4 files changed, 60 insertions(+), 12 deletions(-) diff --git a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py index 18ffec936cc1..bf47b7313a48 100644 --- a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py +++ b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py @@ -78,11 +78,7 @@ def _run_validation(self) -> TestCaseResult: default=0, ) - strategy = self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "strategy", - Strategy, - ) + strategy = self._get_strategy() operator = cast(str, operator) # satisfy mypy # noqa: TC006 sql_expression = cast(str, sql_expression) # satisfy mypy # noqa: TC006 @@ -140,6 +136,20 @@ def _run_validation(self) -> TestCaseResult: passed_rows=passed_rows, ) + def _get_strategy(self) -> Strategy | None: + """Read `strategy` as the server accepts it: in any case, around any spaces, blank as unset + + `thresholdUnit` and `dimensionFailurePolicy` are read the same way. + """ + raw_strategy = self.get_test_case_param_value( + self.test_case.parameterValues, # type: ignore + "strategy", + str, + default="", + ) + value = str(raw_strategy).strip().upper() + return Strategy(value) if value else None + def _aborted(self, msg: str) -> TestCaseResult: logger.error(msg) return self.get_test_case_result_object( diff --git a/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py b/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py index 645ca071a69b..07dea14b803e 100644 --- a/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py +++ b/ingestion/src/metadata/data_quality/validations/table/sqlalchemy/tableCustomSQLQuery.py @@ -362,13 +362,6 @@ def compute_row_count(self) -> int | None: return row._asdict().get(Metrics.rowCount.name) return None - def _get_strategy(self): - return self.get_test_case_param_value( - self.test_case.parameterValues, # type: ignore - "strategy", - Strategy, - ) - def fetch_failed_rows_sample(self): cols, rows = self._get_custom_sql_failed_rows() return TableData(columns=cols, rows=rows) diff --git a/ingestion/tests/unit/observability/data_quality/conftest.py b/ingestion/tests/unit/observability/data_quality/conftest.py index 475f37b1488b..4111fcaeee10 100644 --- a/ingestion/tests/unit/observability/data_quality/conftest.py +++ b/ingestion/tests/unit/observability/data_quality/conftest.py @@ -693,6 +693,37 @@ def test_case_table_custom_sql_query_with_threshold_success(): ) # type: ignore +@pytest.fixture +def test_case_table_custom_sql_query_lowercase_strategy(): + """The server accepts an option in any case, so the runner must read `count` as COUNT""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_USER, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="sqlExpression", value="SELECT COUNT(*) FROM users WHERE age > 30"), + TestCaseParameterValue(name="strategy", value=" count "), + TestCaseParameterValue(name="threshold", value="50"), + ], + ) # type: ignore + + +@pytest.fixture +def test_case_table_custom_sql_query_blank_strategy(): + """The server stores a blank option as unset, so the runner must fall back to ROWS""" + return TestCase( + name=TEST_CASE_NAME, + entityLink=ENTITY_LINK_NAME, + testSuite=EntityReference(id=uuid4(), type="TestSuite"), # type: ignore + testDefinition=EntityReference(id=uuid4(), type="TestDefinition"), # type: ignore + parameterValues=[ + TestCaseParameterValue(name="sqlExpression", value="SELECT * FROM users WHERE age > 20"), + TestCaseParameterValue(name="strategy", value=" "), + ], + ) # type: ignore + + @pytest.fixture def test_case_table_custom_sql_unsafe_query_aborted(): """Test case for test column_value_median_to_be_between""" diff --git a/ingestion/tests/unit/observability/data_quality/test_validations_databases.py b/ingestion/tests/unit/observability/data_quality/test_validations_databases.py index f5d57a41b917..2c404f8c50c9 100644 --- a/ingestion/tests/unit/observability/data_quality/test_validations_databases.py +++ b/ingestion/tests/unit/observability/data_quality/test_validations_databases.py @@ -546,6 +546,20 @@ ), None, ), + ( + "test_case_table_custom_sql_query_lowercase_strategy", + "tableCustomSQLQuery", + "TABLE", + (TestCaseResult, "30", None, TestCaseStatus.Success, None, None, None, None), + None, + ), + ( + "test_case_table_custom_sql_query_blank_strategy", + "tableCustomSQLQuery", + "TABLE", + (TestCaseResult, "60", None, TestCaseStatus.Failed, None, None, None, None), + None, + ), ( "test_case_table_custom_sql_unsafe_query_aborted", "tableCustomSQLQuery", From ca1427dc7d3c00518dc24cbef05ff8f2cd35b044 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 19:48:45 +0530 Subject: [PATCH 07/12] fix(dq): read an unset tableCustomSQLQuery strategy as ROWS so failed rows are sampled --- .../validations/table/base/tableCustomSQLQuery.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py index bf47b7313a48..eee59a890049 100644 --- a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py +++ b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py @@ -83,7 +83,6 @@ def _run_validation(self) -> TestCaseResult: operator = cast(str, operator) # satisfy mypy # noqa: TC006 sql_expression = cast(str, sql_expression) # satisfy mypy # noqa: TC006 threshold = cast(float, threshold) # satisfy mypy # noqa: TC006 - strategy = cast(Strategy, strategy) # satisfy mypy # noqa: TC006 row_count = None try: @@ -136,10 +135,11 @@ def _run_validation(self) -> TestCaseResult: passed_rows=passed_rows, ) - def _get_strategy(self) -> Strategy | None: - """Read `strategy` as the server accepts it: in any case, around any spaces, blank as unset + def _get_strategy(self) -> Strategy: + """Read `strategy` as the server accepts it: in any case, around any spaces, blank as ROWS - `thresholdUnit` and `dimensionFailurePolicy` are read the same way. + `thresholdUnit` and `dimensionFailurePolicy` are read the same way. ROWS is the definition's + default, so an unset strategy also gets failed-row samples. """ raw_strategy = self.get_test_case_param_value( self.test_case.parameterValues, # type: ignore @@ -148,7 +148,7 @@ def _get_strategy(self) -> Strategy | None: default="", ) value = str(raw_strategy).strip().upper() - return Strategy(value) if value else None + return Strategy(value) if value else Strategy.ROWS def _aborted(self, msg: str) -> TestCaseResult: logger.error(msg) From 95ddda026ef69ab6ed8a6fe7b9b7bd584a0a3574 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 22:08:15 +0530 Subject: [PATCH 08/12] fix(dq): check test case conformance on every create path and only what an update changes --- ...cked-against-their-definition-at-create.md | 28 ++++-- .../it/tests/DomainIsolationIT.java | 10 +-- .../it/tests/TestCaseResourceIT.java | 84 +++++++++++++++++- .../jdbi3/TestCaseDefinitionConformance.java | 37 ++++++++ .../service/jdbi3/TestCaseRepository.java | 40 ++++++++- .../resources/dqtests/TestCaseResource.java | 2 +- .../TestCaseDefinitionConformanceTest.java | 88 +++++++++++++++++++ 7 files changed, 266 insertions(+), 23 deletions(-) diff --git a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md index 171946243c91..e95c55ad84ea 100644 --- a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md +++ b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md @@ -1,9 +1,9 @@ # A new test case is checked against its definition's options and column types - **Status:** Accepted -- **Revisions:** v1 2026-10-12 (initial) +- **Revisions:** v1 2026-10-12 (initial); v2 2026-10-16 (every create path, and what an update changes) - **Deciders:** Shreyansh Verma -- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `patch_doesNotRecheckDefinitionConformance`) +- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `put_createWithUnofferedOptionValue_4xx`, `put_createOnUnsupportedColumnType_4xx`, `patch_toUnofferedOptionValue_4xx`, `createMany_withOneUnofferedOptionValue_createsNone`, `patch_keepsATestOnARetypedColumnEditable`) - **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line) ## Context @@ -15,7 +15,7 @@ aborted on (`'SUM' is not a valid Strategy`) or that measured nothing were store ## Decision -`prepare()` rejects, when `update` is false, a test case that: +A new test case is rejected when it: - gives a parameter with optionValues a value that is none of them (compared case-insensitively, as `TestCaseThresholdValidator` reads thresholdUnit; a null, empty or whitespace value is unset); @@ -23,8 +23,16 @@ aborted on (`'SUM' is not a valid Strategy`) or that measured nothing were store every type, as the listing filter reads it; a column with no dataType is accepted). The column is found case-insensitively, as `validateColumnTestCase` finds it. -The gate is the one the disabled-definition check uses, so a row stored before these rules stays -editable by PATCH and PUT. +It runs where every create path meets, not in `prepare()`, whose `update` flag is true for a PUT +that creates a row and which the CSV import skips: `createNewEntity` covers POST, PUT and the CSV +import, and `createManyTestCases` checks a whole `createMany` batch before any of it is stored, so +one invalid test still fails the batch with nothing written (`createMany` stores in chunks of 100, +each in its own transaction). + +An update is checked only for what it changes (`TestUpdater`, through +`TestCaseDefinitionConformance.validateUpdate`): a parameter value it changes must be one of that +parameter's options, and a test moved to another column or definition is checked in full. A row +stored before these rules, or one whose column ingestion has since retyped, stays editable. useDynamicAssertion is not checked against supportsDynamicAssertion: a client can still send it on a definition without dynamic support (the Collate UI does, from a toggle left on), and rejecting it @@ -32,9 +40,11 @@ would break that client. Test generation already sends it only where it is suppo ## Consequences -The UI, MCP `CreateTestCaseTool`, ODCS materialization and API clients all get these checks. ODCS -materialization catches the `IllegalArgumentException` and records the rule as skipped. PUT -`createOrUpdate` prepares with `update` true even for a new row, and the non-dry-run CSV import does -not call `prepare()`, so neither is covered yet; both are follow-ups. The ai-platform test generator +The UI, MCP `CreateTestCaseTool`, ODCS materialization, ingestion (dbt, Great Expectations and the +SDKs create by PUT), the CSV import and API clients all get these checks. ODCS materialization +catches the `IllegalArgumentException` and records the rule as skipped. dbt and Great Expectations +create their own definitions with no supportedDataTypes or optionValues, so their test cases pass; +a definition from the test library is held to the types and options its author declared. A CSV +import dry run does not create, so it does not report these rejections. The ai-platform test generator applies the same reading before it sends a request (ADR:ai-platform/2026-10-12-generated-test-parameters-conform-to-their-definition), so a server rejection there means the two have drifted. diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java index ec80fb17bf6e..3538443100c9 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java @@ -65,7 +65,6 @@ import org.openmetadata.schema.type.MetadataOperation; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.exceptions.ForbiddenException; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.sdk.network.HttpMethod; import org.openmetadata.sdk.network.RequestOptions; @@ -580,13 +579,8 @@ void test_incidents_restrictedUserSeesOnlyOwnDomain(TestNamespace ns) throws Exc Table ownTable = createTable(admin, p + "_own", schema, ownDomain, cleanup); Table foreignTable = createTable(admin, p + "_foreign", schema, foreignDomain, cleanup); - String testDefinitionFqn = - admin - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // Supports every column type, so it fits the INT `id` column the incidents are raised on. + String testDefinitionFqn = "columnValuesToBeNotNull"; String ownIncidentFqn = createIncident(admin, ownTable, p + "_own_tc", testDefinitionFqn); String foreignIncidentFqn = createIncident(admin, foreignTable, p + "_foreign_tc", testDefinitionFqn); diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java index d381dc72f1f9..1c5a5e8bcc4f 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java @@ -926,7 +926,89 @@ void post_testOnUnsupportedColumnType_4xx(TestNamespace ns) { } @Test - void patch_doesNotRecheckDefinitionConformance(TestNamespace ns) { + void put_createWithUnofferedOptionValue_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("put_bad_strategy")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + ">"); + request.setTestDefinition("tableCustomSQLQuery"); + request.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("SUM"))); + + Exception error = + assertThrows(Exception.class, () -> SdkClients.adminClient().testCases().upsert(request)); + assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); + } + + @Test + void put_createOnUnsupportedColumnType_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("put_length_on_number")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + + Exception error = + assertThrows(Exception.class, () -> SdkClients.adminClient().testCases().upsert(request)); + assertTrue(error.getMessage().contains("does not support column type"), error.getMessage()); + } + + @Test + void patch_toUnofferedOptionValue_4xx(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("patch_bad_strategy")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + ">"); + request.setTestDefinition("tableCustomSQLQuery"); + request.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("ROWS"))); + TestCase testCase = createEntity(request); + + testCase.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("SUM"))); + + Exception error = + assertThrows(Exception.class, () -> patchEntity(testCase.getId().toString(), testCase)); + assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); + } + + @Test + void createMany_withOneUnofferedOptionValue_createsNone(TestNamespace ns) { + Table table = createTable(ns); + String tableLink = "<#E::table::" + table.getFullyQualifiedName() + ">"; + CreateTestCase valid = new CreateTestCase(); + valid.setName(ns.prefix("many_valid")); + valid.setEntityLink(tableLink); + valid.setTestDefinition("tableRowCountToEqual"); + valid.setParameterValues( + List.of(new TestCaseParameterValue().withName("value").withValue("100"))); + CreateTestCase invalid = new CreateTestCase(); + invalid.setName(ns.prefix("many_bad_strategy")); + invalid.setEntityLink(tableLink); + invalid.setTestDefinition("tableCustomSQLQuery"); + invalid.setParameterValues( + List.of( + new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), + new TestCaseParameterValue().withName("strategy").withValue("SUM"))); + + OpenMetadataClient client = SdkClients.adminClient(); + Exception error = + assertThrows(Exception.class, () -> client.testCases().createMany(List.of(valid, invalid))); + assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); + // The batch wrote nothing: the valid test can still be created on its own. + assertEquals(valid.getName(), createEntity(valid).getName()); + } + + @Test + void patch_keepsATestOnARetypedColumnEditable(TestNamespace ns) { Table table = createTable(ns); CreateTestCase request = new CreateTestCase(); request.setName(ns.prefix("patch_gate")); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java index 27ff9f0f74de..7e8b1e9797c5 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformance.java @@ -7,6 +7,8 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.UUID; +import java.util.function.Supplier; import java.util.stream.Collectors; import org.openmetadata.schema.entity.data.Table; import org.openmetadata.schema.tests.TestCase; @@ -15,6 +17,7 @@ import org.openmetadata.schema.tests.TestDefinition; import org.openmetadata.schema.type.Column; import org.openmetadata.schema.type.ColumnDataType; +import org.openmetadata.schema.type.EntityReference; import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; /** @@ -31,6 +34,40 @@ static void validate( validateColumnType(definition, table, entityLink); } + /** + * Checks only what an update changes, so a row stored before these rules, or on a column + * retyped since, stays editable. A test moved to another column or definition is checked in + * full. + */ + static void validateUpdate( + TestCase original, + TestCase updated, + Supplier definition, + Supplier table) { + List changed = changedValues(original, updated); + if (isRetargeted(original, updated)) { + validate(updated, definition.get(), table.get(), EntityLink.parse(updated.getEntityLink())); + } else if (!changed.isEmpty()) { + validateOptionValues(changed, listOrEmpty(definition.get().getParameterDefinition())); + } + } + + private static boolean isRetargeted(TestCase original, TestCase updated) { + return !Objects.equals(original.getEntityLink(), updated.getEntityLink()) + || !Objects.equals(definitionId(original), definitionId(updated)); + } + + private static UUID definitionId(TestCase test) { + return Optional.ofNullable(test.getTestDefinition()).map(EntityReference::getId).orElse(null); + } + + private static List changedValues(TestCase original, TestCase updated) { + List stored = listOrEmpty(original.getParameterValues()); + return listOrEmpty(updated.getParameterValues()).stream() + .filter(value -> !stored.contains(value)) + .toList(); + } + private static void validateOptionValues( List values, List parameters) { Map> options = optionsByParameter(parameters); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java index 36a2d3118843..355426adc8c2 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java @@ -898,10 +898,6 @@ public void prepare(TestCase test, boolean update) { testDefinition.getTestPlatforms(), testDefinition); validateColumnTestCase(table, entityLink, testDefinition.getEntityType()); - // Same gate as the disabled-definition check: a row stored before these rules stays editable. - if (!update) { - TestCaseDefinitionConformance.validate(test, testDefinition, table, entityLink); - } validateDimensionColumns(test, table); setDataQualityDimension(test, testDefinition); @@ -1187,6 +1183,33 @@ protected List getFieldsStrippedFromStorageJson() { INCIDENTS_FIELD); } + /** + * POST, PUT and the CSV import all create through here, after a prepare whose {@code update} + * flag is true for a PUT that creates a row + * (ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create). + */ + @Override + protected TestCase createNewEntity(TestCase test) { + validateDefinitionConformance(test); + return super.createNewEntity(test); + } + + /** + * createMany stores in chunks, each in its own transaction, so the whole batch is checked before + * any of it is stored. + */ + public List createManyTestCases(UriInfo uriInfo, List testCases) { + testCases.forEach(TestCaseRepository::validateDefinitionConformance); + return createMany(uriInfo, testCases); + } + + private static void validateDefinitionConformance(TestCase test) { + EntityLink entityLink = EntityLink.parse(test.getEntityLink()); + TestDefinition definition = Entity.getEntity(test.getTestDefinition(), "", NON_DELETED); + Table table = Entity.getEntity(entityLink, "columns", ALL); + TestCaseDefinitionConformance.validate(test, definition, table, entityLink); + } + @Override public void storeEntity(TestCase test, boolean update) { store(test, update); @@ -2005,6 +2028,14 @@ private void updateDataQualityDimension() { } } + private void validateChangedConformance() { + TestCaseDefinitionConformance.validateUpdate( + original, + updated, + () -> Entity.getEntity(updated.getTestDefinition(), "", ALL), + () -> Entity.getEntity(EntityLink.parse(updated.getEntityLink()), "columns", ALL)); + } + @Override protected boolean consolidateChanges(TestCase original, TestCase updated, Operation operation) { return false; @@ -2013,6 +2044,7 @@ protected boolean consolidateChanges(TestCase original, TestCase updated, Operat @Transaction @Override public void entitySpecificUpdate(boolean consolidatingChanges) { + validateChangedConformance(); compareAndUpdate( "entity", () -> { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java index 7fedf98cac22..955d8191f733 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java @@ -798,7 +798,7 @@ public Response createMany( new OperationContext(Entity.TEST_CASE, MetadataOperation.EDIT_TESTS)); testCases.add(test); }); - repository.createMany(uriInfo, testCases); + repository.createManyTestCases(uriInfo, testCases); return Response.ok(testCases).build(); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java index 71ce1de95b03..464fa72fe9a4 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java @@ -6,6 +6,7 @@ import java.io.IOException; import java.util.List; +import java.util.UUID; import org.junit.jupiter.api.Test; import org.openmetadata.schema.entity.data.Table; import org.openmetadata.schema.tests.TestCase; @@ -14,6 +15,7 @@ import org.openmetadata.schema.tests.TestDefinition; import org.openmetadata.schema.type.Column; import org.openmetadata.schema.type.ColumnDataType; +import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.TestDefinitionEntityType; import org.openmetadata.service.resources.feeds.MessageParser.EntityLink; @@ -152,6 +154,92 @@ void aTableTestIsNotCheckedAgainstColumnTypes() { () -> TestCaseDefinitionConformance.validate(new TestCase(), LENGTHS, TABLE, table())); } + @Test + void anUpdateThatChangesAnOptionToOneNotOfferedIsRejected() { + TestCase stored = withParameter("strategy", "ROWS").withEntityLink(table().getLinkString()); + TestCase updated = withParameter("strategy", "SUM").withEntityLink(table().getLinkString()); + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> CUSTOM_SQL, () -> TABLE)); + assertTrue(error.getMessage().contains("must be one of [ROWS, COUNT]"), error.getMessage()); + } + + @Test + void anUpdateKeepsAStoredValueOutsideTheOptionsEditable() { + TestCase stored = withParameter("strategy", "SUM").withEntityLink(table().getLinkString()); + TestCase updated = + withParameter("strategy", "SUM") + .withEntityLink(table().getLinkString()) + .withDescription("edited"); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> CUSTOM_SQL, () -> TABLE)); + } + + @Test + void anUpdateKeepsATestOnAColumnRetypedSinceEditable() { + TestCase stored = new TestCase().withEntityLink(column("Amount").getLinkString()); + TestCase updated = + new TestCase().withEntityLink(column("Amount").getLinkString()).withDescription("edited"); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> LENGTHS, () -> TABLE)); + } + + /** Failed-row samples and inspection queries are stored by updates that change no parameter. */ + @Test + void anUpdateThatChangesNoParameterDoesNotLoadTheDefinition() { + TestCase stored = withParameter("strategy", "ROWS").withEntityLink(table().getLinkString()); + TestCase updated = + withParameter("strategy", "ROWS") + .withEntityLink(table().getLinkString()) + .withInspectionQuery("SELECT 1"); + assertDoesNotThrow( + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, + updated, + () -> { + throw new AssertionError("the definition was loaded"); + }, + () -> TABLE)); + } + + @Test + void anUpdateThatMovesTheTestToAnUnsupportedColumnIsRejected() { + TestCase stored = new TestCase().withEntityLink(column("note").getLinkString()); + TestCase updated = new TestCase().withEntityLink(column("Amount").getLinkString()); + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> LENGTHS, () -> TABLE)); + assertTrue(error.getMessage().contains("NUMERIC"), error.getMessage()); + } + + @Test + void anUpdateThatChangesTheDefinitionChecksEveryValueAgainstIt() { + TestCase stored = + withParameter("strategy", "SUM") + .withEntityLink(table().getLinkString()) + .withTestDefinition(new EntityReference().withId(UUID.randomUUID())); + TestCase updated = + withParameter("strategy", "SUM") + .withEntityLink(table().getLinkString()) + .withTestDefinition(new EntityReference().withId(UUID.randomUUID())); + assertThrows( + IllegalArgumentException.class, + () -> + TestCaseDefinitionConformance.validateUpdate( + stored, updated, () -> CUSTOM_SQL, () -> TABLE)); + } + /** * ai-platform reads a bound as a parameter with a validationRule; every dynamic seed must mark * its bounds so. From 917419e12a78bbec5b5cbecc0b83060638d2a1f2 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Fri, 16 Oct 2026 22:43:40 +0530 Subject: [PATCH 09/12] fix(dq): check a new test case before its test suite is created --- ...cked-against-their-definition-at-create.md | 24 ++++++++----- .../it/tests/TestCaseResourceIT.java | 35 +++++++++++++++++++ .../service/jdbi3/TestCaseRepository.java | 16 +++++++-- 3 files changed, 65 insertions(+), 10 deletions(-) diff --git a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md index e95c55ad84ea..81817e405b3e 100644 --- a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md +++ b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md @@ -3,7 +3,7 @@ - **Status:** Accepted - **Revisions:** v1 2026-10-12 (initial); v2 2026-10-16 (every create path, and what an update changes) - **Deciders:** Shreyansh Verma -- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `put_createWithUnofferedOptionValue_4xx`, `put_createOnUnsupportedColumnType_4xx`, `patch_toUnofferedOptionValue_4xx`, `createMany_withOneUnofferedOptionValue_createsNone`, `patch_keepsATestOnARetypedColumnEditable`) +- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `put_createWithUnofferedOptionValue_4xx`, `put_createOnUnsupportedColumnType_4xx`, `patch_toUnofferedOptionValue_4xx`, `createMany_withOneUnofferedOptionValue_createsNone`, `patch_keepsATestOnARetypedColumnEditable`, `post_rejectedForConformance_leavesNoTestSuite`, `put_rejectedForConformance_leavesNoTestSuite`) - **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line) ## Context @@ -23,11 +23,13 @@ A new test case is rejected when it: every type, as the listing filter reads it; a column with no dataType is accepted). The column is found case-insensitively, as `validateColumnTestCase` finds it. -It runs where every create path meets, not in `prepare()`, whose `update` flag is true for a PUT -that creates a row and which the CSV import skips: `createNewEntity` covers POST, PUT and the CSV -import, and `createManyTestCases` checks a whole `createMany` batch before any of it is stored, so -one invalid test still fails the batch with nothing written (`createMany` stores in chunks of 100, -each in its own transaction). +`prepare()` checks a new row before it creates the table's basic test suite, so a rejected create +leaves nothing behind. A row is new when `update` is false (POST) or when no test case has its +FQN yet: a PUT that creates a row prepares with `update` true. `createNewEntity`, which every +create reaches, checks again, because the CSV import never calls `prepare()`. `createManyTestCases` +checks a whole `createMany` batch before any of it is prepared or stored, so one invalid test fails +the batch with nothing written (`createMany` prepares one test at a time, creating suites as it +goes, and stores in chunks of 100, each in its own transaction). An update is checked only for what it changes (`TestUpdater`, through `TestCaseDefinitionConformance.validateUpdate`): a parameter value it changes must be one of that @@ -44,7 +46,13 @@ The UI, MCP `CreateTestCaseTool`, ODCS materialization, ingestion (dbt, Great Ex SDKs create by PUT), the CSV import and API clients all get these checks. ODCS materialization catches the `IllegalArgumentException` and records the rule as skipped. dbt and Great Expectations create their own definitions with no supportedDataTypes or optionValues, so their test cases pass; -a definition from the test library is held to the types and options its author declared. A CSV -import dry run does not create, so it does not report these rejections. The ai-platform test generator +a definition from the test library is held to the types and options its author declared. + +The CSV import creates a row's basic test suite before it validates anything, in a dry run too, +so a row this check rejects can still leave an empty basic suite on a table that had none, as any +other import row that fails does. A dry run does not create, so it does not report these +rejections. Checking import rows before their suite would mean telling new rows from existing +ones in the import, which only `createOrUpdateForImport` does today; that is left to a change of +the import itself. The ai-platform test generator applies the same reading before it sends a request (ADR:ai-platform/2026-10-12-generated-test-parameters-conform-to-their-definition), so a server rejection there means the two have drifted. diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java index 1c5a5e8bcc4f..db442323359e 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java @@ -957,6 +957,41 @@ void put_createOnUnsupportedColumnType_4xx(TestNamespace ns) { assertTrue(error.getMessage().contains("does not support column type"), error.getMessage()); } + @Test + void post_rejectedForConformance_leavesNoTestSuite(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("post_no_suite")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + + assertThrows(Exception.class, () -> createEntity(request)); + assertNull(tableTestSuite(table)); + } + + @Test + void put_rejectedForConformance_leavesNoTestSuite(TestNamespace ns) { + Table table = createTable(ns); + CreateTestCase request = new CreateTestCase(); + request.setName(ns.prefix("put_no_suite")); + request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); + request.setTestDefinition("columnValueLengthsToBeBetween"); + request.setParameterValues( + List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); + + assertThrows(Exception.class, () -> SdkClients.adminClient().testCases().upsert(request)); + assertNull(tableTestSuite(table)); + } + + private static EntityReference tableTestSuite(Table table) { + return SdkClients.adminClient() + .tables() + .get(table.getId().toString(), "testSuite") + .getTestSuite(); + } + @Test void patch_toUnofferedOptionValue_4xx(TestNamespace ns) { Table table = createTable(ns); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java index 355426adc8c2..febcf3660c96 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java @@ -901,6 +901,10 @@ public void prepare(TestCase test, boolean update) { validateDimensionColumns(test, table); setDataQualityDimension(test, testDefinition); + if (isNewTestCase(test, update)) { + TestCaseDefinitionConformance.validate(test, testDefinition, table, entityLink); + } + // Create/resolve the basic test suite only after all validations pass. // This avoids creating side entities when request validation fails early. var testSuite = getOrCreateTestSuite(test, table); @@ -1184,8 +1188,16 @@ protected List getFieldsStrippedFromStorageJson() { } /** - * POST, PUT and the CSV import all create through here, after a prepare whose {@code update} - * flag is true for a PUT that creates a row + * A PUT that creates a row prepares with {@code update} true, so whether the row exists decides. + * The FQN is set here the same way prepareInternal sets it right after prepare. + */ + private boolean isNewTestCase(TestCase test, boolean update) { + setFullyQualifiedName(test); + return !update || findByNameOrNull(test.getFullyQualifiedName(), ALL) == null; + } + + /** + * POST, PUT and the CSV import all create through here; the CSV import never calls prepare * (ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create). */ @Override From cc590ce5bc1a8c4d6e002f42b291c97298c3947f Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Sat, 17 Oct 2026 01:00:32 +0530 Subject: [PATCH 10/12] fix(dq): check new test cases in prepare and correct the claims in docs and comments --- ...cked-against-their-definition-at-create.md | 28 +++++++------- ...et-parameter-is-read-in-its-column-type.md | 34 +++++++++-------- .../mixins/pandas_validator_mixin.py | 4 +- .../validations/mixins/sqa_validator_mixin.py | 2 +- .../table/base/tableCustomSQLQuery.py | 6 +-- .../data_quality/validations/utils.py | 6 +-- .../observability/data_quality/conftest.py | 10 ++--- .../data_quality/test_validations_datalake.py | 2 +- .../data_quality/validations/test_utils.py | 2 +- .../it/tests/DomainIsolationIT.java | 2 +- .../service/jdbi3/TestCaseRepository.java | 37 +------------------ .../resources/dqtests/TestCaseResource.java | 2 +- .../TestCaseDefinitionConformanceTest.java | 2 +- 13 files changed, 51 insertions(+), 86 deletions(-) diff --git a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md index 81817e405b3e..9b1a2b80c411 100644 --- a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md +++ b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md @@ -1,7 +1,7 @@ # A new test case is checked against its definition's options and column types - **Status:** Accepted -- **Revisions:** v1 2026-10-12 (initial); v2 2026-10-16 (every create path, and what an update changes) +- **Revisions:** v1 2026-10-12 (initial); v2 2026-10-16 (checked before the basic test suite is created, and an update checked for what it changes) - **Deciders:** Shreyansh Verma - **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `put_createWithUnofferedOptionValue_4xx`, `put_createOnUnsupportedColumnType_4xx`, `patch_toUnofferedOptionValue_4xx`, `createMany_withOneUnofferedOptionValue_createsNone`, `patch_keepsATestOnARetypedColumnEditable`, `post_rejectedForConformance_leavesNoTestSuite`, `put_rejectedForConformance_leavesNoTestSuite`) - **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line) @@ -10,7 +10,7 @@ `validateTestParameters` checked parameter names, required parameters and min/max rules. Nothing compared a value with the parameter's optionValues, or the column's type with the definition's -supportedDataTypes (used only to filter the definition listing). Test cases that ingestion then +supportedDataTypes, which no test case was checked against. Test cases that ingestion then aborted on (`'SUM' is not a valid Strategy`) or that measured nothing were stored as sent. ## Decision @@ -23,13 +23,8 @@ A new test case is rejected when it: every type, as the listing filter reads it; a column with no dataType is accepted). The column is found case-insensitively, as `validateColumnTestCase` finds it. -`prepare()` checks a new row before it creates the table's basic test suite, so a rejected create -leaves nothing behind. A row is new when `update` is false (POST) or when no test case has its -FQN yet: a PUT that creates a row prepares with `update` true. `createNewEntity`, which every -create reaches, checks again, because the CSV import never calls `prepare()`. `createManyTestCases` -checks a whole `createMany` batch before any of it is prepared or stored, so one invalid test fails -the batch with nothing written (`createMany` prepares one test at a time, creating suites as it -goes, and stores in chunks of 100, each in its own transaction). +`prepare()` checks a test case when `update` is false, before it creates the table's basic test +suite; every create path reaches it that way (#35667). An update is checked only for what it changes (`TestUpdater`, through `TestCaseDefinitionConformance.validateUpdate`): a parameter value it changes must be one of that @@ -48,11 +43,14 @@ catches the `IllegalArgumentException` and records the rule as skipped. dbt and create their own definitions with no supportedDataTypes or optionValues, so their test cases pass; a definition from the test library is held to the types and options its author declared. -The CSV import creates a row's basic test suite before it validates anything, in a dry run too, -so a row this check rejects can still leave an empty basic suite on a table that had none, as any -other import row that fails does. A dry run does not create, so it does not report these -rejections. Checking import rows before their suite would mean telling new rows from existing -ones in the import, which only `createOrUpdateForImport` does today; that is left to a change of -the import itself. The ai-platform test generator +PUT-create and the CSV import reach these checks through #35667, which fixes how those callers +reach `prepare()`. + +`createMany` prepares every test of a batch before it stores any, so one rejected test fails the +batch with no test case stored. A basic test suite that an earlier test's `prepare()` created is +not rolled back, as with any other `prepare()` rejection in a batch: no transaction spans +`createMany`. + +The ai-platform test generator applies the same reading before it sends a request (ADR:ai-platform/2026-10-12-generated-test-parameters-conform-to-their-definition), so a server rejection there means the two have drifted. diff --git a/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md index e04e670dfe39..ecc61db8bdab 100644 --- a/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md +++ b/docs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md @@ -9,26 +9,28 @@ ## Context `columnValuesToBeInSet` and `columnValuesToBeNotInSet` parsed `allowedValues` / `forbiddenValues` -with `literal_eval`. Clients write these parameters differently: the UI writes a JSON array of -strings, a generated test writes JSON (`[false]`), older test cases hold Python literals -(`[True, False]`). `literal_eval` rejects JSON's lowercase `false` / `true` / `null`, which aborted -the test, and on a BOOLEAN column it kept `"true"` / `"false"` as strings, which an engine such as -Databricks refuses to compare with a boolean. +with `literal_eval`. The UI writes a set as a JSON array of strings, and a generated test writes a +JSON array, whose booleans and null are lowercase (`[false]`). `literal_eval` rejects JSON's +lowercase `false` / `true` / `null`, which aborted the test, and on a BOOLEAN column it kept +`"true"` / `"false"` as strings, which an engine such as Databricks refuses to compare with a +boolean. ## Decision -`parse_set_values` reads a set as a Python literal and, when it is not one, as JSON; where both -parse a value they agree, so nothing that parsed before reads differently. A value that is neither -raises ValueError and the test is Aborted, as before. A value that parses but is not a list (a bare -string or number) is now rejected the same way, rather than compared as a single value. On a -column whose engine type is boolean, `"true"` / `"false"` in any case become booleans; nothing -else is mapped. +`parse_set_values` reads a set as a Python literal and, when `literal_eval` rejects it, as JSON. +`literal_eval` runs first, so a value it read before reads the same. A value neither reads still +Aborts the test, as before. A value that parses but is not a list (a bare string or number) is now +rejected with a ValueError, so it Aborts too. On a column typed BOOLEAN, `"true"` / `"false"` in +any case become booleans; nothing else is mapped. -The column type is the engine's (`SQAValidatorMixin.get_set_values_type`). Pandas returns none, so -a datalake set keeps its values as written: pandas infers column types from the values, and a -column of `"True"` / `"False"` strings reads as BOOLEAN. +The column type is the one OpenMetadata records for the column, which the SQL runner maps to +SQLAlchemy (`SQAValidatorMixin.get_set_values_type`). The pandas runner returns none, so a datalake +set keeps its values as written: `GenericDataFrameColumnParser` types a column of `"True"` / +`"False"` strings as BOOLEAN. ## Consequences -A set written by any client compares the same way, and a set test no longer aborts on JSON. A -boolean column read through pandas still compares as strings; that is deliberate, not an omission. +A set test no longer aborts on a JSON set, and on a column typed BOOLEAN `"true"` / `"false"` +compare as booleans. On a datalake the set is not typed, so its values compare as written. That is +deliberate: typing it by `GenericDataFrameColumnParser` would turn a set of `"True"` / `"False"` +strings into booleans, which the column's string values do not equal. diff --git a/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py b/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py index 55647123336a..ec1e951ec938 100644 --- a/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py +++ b/ingestion/src/metadata/data_quality/validations/mixins/pandas_validator_mixin.py @@ -50,8 +50,8 @@ class PandasValidatorMixin: runner: "PandasRunner" def get_set_values_type(self) -> None: - """None: pandas infers column types from the values (strings "True"/"False" read as - BOOLEAN), so a set keeps the values as written; this also avoids loading a chunk.""" + """None: GenericDataFrameColumnParser types a column of "True"/"False" strings as BOOLEAN, + so a set keeps its values as written.""" return def get_column(self: HasValidatorContext, column_name: str | None = None) -> SQALikeColumn: diff --git a/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py b/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py index ad67b9809bd8..5dbf9cbfdd43 100644 --- a/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py +++ b/ingestion/src/metadata/data_quality/validations/mixins/sqa_validator_mixin.py @@ -80,7 +80,7 @@ class SQAValidatorMixin: """Validator mixin for SQA test cases""" def get_set_values_type(self): - """The column type the engine declares, so a set compares in that type""" + """The column's type as OpenMetadata records it, mapped to SQLAlchemy, so a set compares in that type""" return self.get_column().type # type: ignore def get_column(self: HasValidatorContext, column_name: str | None = None) -> Column: diff --git a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py index eee59a890049..1504b2d0d5c8 100644 --- a/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py +++ b/ingestion/src/metadata/data_quality/validations/table/base/tableCustomSQLQuery.py @@ -136,10 +136,10 @@ def _run_validation(self) -> TestCaseResult: ) def _get_strategy(self) -> Strategy: - """Read `strategy` as the server accepts it: in any case, around any spaces, blank as ROWS + """Read `strategy` in any case, ignoring surrounding spaces; a missing or blank value is ROWS - `thresholdUnit` and `dimensionFailurePolicy` are read the same way. ROWS is the definition's - default, so an unset strategy also gets failed-row samples. + ROWS is the definition's default, so a test without a strategy is treated like ROWS, + failed-row samples included. """ raw_strategy = self.get_test_case_param_value( self.test_case.parameterValues, # type: ignore diff --git a/ingestion/src/metadata/data_quality/validations/utils.py b/ingestion/src/metadata/data_quality/validations/utils.py index ac489ae35c10..fa0a1b28ed62 100644 --- a/ingestion/src/metadata/data_quality/validations/utils.py +++ b/ingestion/src/metadata/data_quality/validations/utils.py @@ -39,10 +39,10 @@ def parse_set_values(raw: str, column_type) -> list: def _parse_literal(raw: str) -> Any: - """A Python literal, or JSON when it is not one: JSON writes booleans and null in lowercase + """A Python literal, or JSON when literal_eval rejects it: JSON writes booleans and null in lowercase - Where both parse a value they agree, so a set written either way reads the same. A value that - is neither raises ValueError (json.JSONDecodeError is one). + literal_eval runs first, so a value it read before reads the same. Its ValueError and + SyntaxError fall through to JSON; any other error it raises propagates. """ try: value = literal_eval(raw) diff --git a/ingestion/tests/unit/observability/data_quality/conftest.py b/ingestion/tests/unit/observability/data_quality/conftest.py index 4111fcaeee10..ba93dbbc1672 100644 --- a/ingestion/tests/unit/observability/data_quality/conftest.py +++ b/ingestion/tests/unit/observability/data_quality/conftest.py @@ -695,7 +695,7 @@ def test_case_table_custom_sql_query_with_threshold_success(): @pytest.fixture def test_case_table_custom_sql_query_lowercase_strategy(): - """The server accepts an option in any case, so the runner must read `count` as COUNT""" + """`strategy` in any case, with spaces around it, reads as COUNT""" return TestCase( name=TEST_CASE_NAME, entityLink=ENTITY_LINK_USER, @@ -711,7 +711,7 @@ def test_case_table_custom_sql_query_lowercase_strategy(): @pytest.fixture def test_case_table_custom_sql_query_blank_strategy(): - """The server stores a blank option as unset, so the runner must fall back to ROWS""" + """A blank `strategy` reads as ROWS, the definition's default""" return TestCase( name=TEST_CASE_NAME, entityLink=ENTITY_LINK_NAME, @@ -925,7 +925,7 @@ def test_case_column_value_in_set_boolean(): @pytest.fixture def test_case_column_values_not_in_set_boolean_strings(): - """The forbidden set as the UI and generated tests write it: strings""" + """The forbidden set as the UI writes it, and a generated test can: a JSON array of strings""" return TestCase( name=TEST_CASE_NAME, entityLink=ENTITY_LINK_IS_ACTIVE, @@ -940,7 +940,7 @@ def test_case_column_values_not_in_set_boolean_strings(): @pytest.fixture def test_case_column_values_not_in_set_boolean_json(): - """The forbidden set as a generated test writes it: a JSON array with a lowercase boolean""" + """The forbidden set as a generated test can write it: a JSON array with a lowercase boolean""" return TestCase( name=TEST_CASE_NAME, entityLink=ENTITY_LINK_IS_ACTIVE, @@ -969,7 +969,7 @@ def test_case_column_value_in_set_missing_column(): @pytest.fixture def test_case_column_value_in_set_boolean_strings(): - """The allowed set as the UI and generated tests write it: strings""" + """The allowed set as the UI writes it, and a generated test can: a JSON array of strings""" return TestCase( name=TEST_CASE_NAME, entityLink=ENTITY_LINK_IS_ACTIVE, diff --git a/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py b/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py index ccb64491f197..5e2c10bc56c4 100644 --- a/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py +++ b/ingestion/tests/unit/observability/data_quality/test_validations_datalake.py @@ -1320,7 +1320,7 @@ def test_suite_validation_datalake( ], ) def test_string_booleans_on_a_datalake_column_stay_strings(test_case_type, parameter_values, status, failed_rows): - """Pandas infers a column of "True"/"False" strings as BOOLEAN; its sets must still compare as strings""" + """GenericDataFrameColumnParser types a column of "True"/"False" strings as BOOLEAN; its sets must still compare as strings""" test_case = TestCase( name="string_booleans", entityLink="<#E::table::service.db.users::columns::flag>", diff --git a/ingestion/tests/unit/observability/data_quality/validations/test_utils.py b/ingestion/tests/unit/observability/data_quality/validations/test_utils.py index 4941dae27ba2..1a88c9074302 100644 --- a/ingestion/tests/unit/observability/data_quality/validations/test_utils.py +++ b/ingestion/tests/unit/observability/data_quality/validations/test_utils.py @@ -71,7 +71,7 @@ def test_strings_on_a_string_or_unknown_column_stay_strings(column_type): ], ) def test_a_json_written_set_is_read_like_a_python_one(raw, column_type, expected): - """Generated tests and API clients write JSON, where booleans and null are lowercase""" + """The UI and generated tests write JSON, where booleans and null are lowercase""" assert utils.parse_set_values(raw=raw, column_type=column_type) == expected diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java index 3538443100c9..661c3bc4960a 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/DomainIsolationIT.java @@ -579,7 +579,7 @@ void test_incidents_restrictedUserSeesOnlyOwnDomain(TestNamespace ns) throws Exc Table ownTable = createTable(admin, p + "_own", schema, ownDomain, cleanup); Table foreignTable = createTable(admin, p + "_foreign", schema, foreignDomain, cleanup); - // Supports every column type, so it fits the INT `id` column the incidents are raised on. + // Lists INT among its supported types, so it fits the INT `id` column the incidents are on. String testDefinitionFqn = "columnValuesToBeNotNull"; String ownIncidentFqn = createIncident(admin, ownTable, p + "_own_tc", testDefinitionFqn); String foreignIncidentFqn = diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java index febcf3660c96..1c3e10df2461 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TestCaseRepository.java @@ -901,7 +901,7 @@ public void prepare(TestCase test, boolean update) { validateDimensionColumns(test, table); setDataQualityDimension(test, testDefinition); - if (isNewTestCase(test, update)) { + if (!update) { TestCaseDefinitionConformance.validate(test, testDefinition, table, entityLink); } @@ -1187,41 +1187,6 @@ protected List getFieldsStrippedFromStorageJson() { INCIDENTS_FIELD); } - /** - * A PUT that creates a row prepares with {@code update} true, so whether the row exists decides. - * The FQN is set here the same way prepareInternal sets it right after prepare. - */ - private boolean isNewTestCase(TestCase test, boolean update) { - setFullyQualifiedName(test); - return !update || findByNameOrNull(test.getFullyQualifiedName(), ALL) == null; - } - - /** - * POST, PUT and the CSV import all create through here; the CSV import never calls prepare - * (ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create). - */ - @Override - protected TestCase createNewEntity(TestCase test) { - validateDefinitionConformance(test); - return super.createNewEntity(test); - } - - /** - * createMany stores in chunks, each in its own transaction, so the whole batch is checked before - * any of it is stored. - */ - public List createManyTestCases(UriInfo uriInfo, List testCases) { - testCases.forEach(TestCaseRepository::validateDefinitionConformance); - return createMany(uriInfo, testCases); - } - - private static void validateDefinitionConformance(TestCase test) { - EntityLink entityLink = EntityLink.parse(test.getEntityLink()); - TestDefinition definition = Entity.getEntity(test.getTestDefinition(), "", NON_DELETED); - Table table = Entity.getEntity(entityLink, "columns", ALL); - TestCaseDefinitionConformance.validate(test, definition, table, entityLink); - } - @Override public void storeEntity(TestCase test, boolean update) { store(test, update); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java index 955d8191f733..7fedf98cac22 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestCaseResource.java @@ -798,7 +798,7 @@ public Response createMany( new OperationContext(Entity.TEST_CASE, MetadataOperation.EDIT_TESTS)); testCases.add(test); }); - repository.createManyTestCases(uriInfo, testCases); + repository.createMany(uriInfo, testCases); return Response.ok(testCases).build(); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java index 464fa72fe9a4..4dfe4ac1c5b5 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/TestCaseDefinitionConformanceTest.java @@ -191,7 +191,7 @@ void anUpdateKeepsATestOnAColumnRetypedSinceEditable() { stored, updated, () -> LENGTHS, () -> TABLE)); } - /** Failed-row samples and inspection queries are stored by updates that change no parameter. */ + /** An inspection query is stored by an update that changes no parameter. */ @Test void anUpdateThatChangesNoParameterDoesNotLoadTheDefinition() { TestCase stored = withParameter("strategy", "ROWS").withEntityLink(table().getLinkString()); From c4b530e5485fbf01a74de2d694ba9a1b544e38e7 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Sat, 17 Oct 2026 02:24:11 +0530 Subject: [PATCH 11/12] test(dq): leave the PUT-create conformance ITs to the caller fix --- ...cked-against-their-definition-at-create.md | 21 +++++---- .../it/tests/TestCaseResourceIT.java | 46 ------------------- 2 files changed, 11 insertions(+), 56 deletions(-) diff --git a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md index 9b1a2b80c411..02910de1fe81 100644 --- a/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md +++ b/docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.md @@ -3,7 +3,7 @@ - **Status:** Accepted - **Revisions:** v1 2026-10-12 (initial); v2 2026-10-16 (checked before the basic test suite is created, and an update checked for what it changes) - **Deciders:** Shreyansh Verma -- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `put_createWithUnofferedOptionValue_4xx`, `put_createOnUnsupportedColumnType_4xx`, `patch_toUnofferedOptionValue_4xx`, `createMany_withOneUnofferedOptionValue_createsNone`, `patch_keepsATestOnARetypedColumnEditable`, `post_rejectedForConformance_leavesNoTestSuite`, `put_rejectedForConformance_leavesNoTestSuite`) +- **Guard:** `TestCaseDefinitionConformanceTest`, `TestCaseResourceIT` (`post_testWithUnofferedOptionValue_4xx`, `post_testOnUnsupportedColumnType_4xx`, `patch_toUnofferedOptionValue_4xx`, `createMany_withOneUnofferedOptionValue_createsNone`, `patch_keepsATestOnARetypedColumnEditable`, `post_rejectedForConformance_leavesNoTestSuite`) - **Related:** open-metadata/ai-platform#1650, #35274, #35276 (2.0 line) ## Context @@ -24,7 +24,9 @@ A new test case is rejected when it: found case-insensitively, as `validateColumnTestCase` finds it. `prepare()` checks a test case when `update` is false, before it creates the table's basic test -suite; every create path reaches it that way (#35667). +suite, the gate the disabled-definition check uses. PUT-create and the CSV import miss that gate +for both checks: `createOrUpdate` prepares with `update` true, and the CSV import does not call +`prepare()`. #35667 fixes those callers. An update is checked only for what it changes (`TestUpdater`, through `TestCaseDefinitionConformance.validateUpdate`): a parameter value it changes must be one of that @@ -37,14 +39,13 @@ would break that client. Test generation already sends it only where it is suppo ## Consequences -The UI, MCP `CreateTestCaseTool`, ODCS materialization, ingestion (dbt, Great Expectations and the -SDKs create by PUT), the CSV import and API clients all get these checks. ODCS materialization -catches the `IllegalArgumentException` and records the rule as skipped. dbt and Great Expectations -create their own definitions with no supportedDataTypes or optionValues, so their test cases pass; -a definition from the test library is held to the types and options its author declared. - -PUT-create and the CSV import reach these checks through #35667, which fixes how those callers -reach `prepare()`. +Creates by POST (the UI and API clients), `createMany`, MCP `CreateTestCaseTool` and ODCS +materialization get these checks. Ingestion (dbt, Great Expectations and the Python client's +`create_or_update` create by PUT) and the CSV import get them once #35667 lands. ODCS +materialization catches the `IllegalArgumentException` and records the rule as skipped. dbt and +Great Expectations create their own definitions with no supportedDataTypes or optionValues, so +their test cases pass; a definition from the test library is held to the types and options its +author declared. `createMany` prepares every test of a batch before it stores any, so one rejected test fails the batch with no test case stored. A basic test suite that an earlier test's `prepare()` created is diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java index db442323359e..d84c2660b20b 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResourceIT.java @@ -925,38 +925,6 @@ void post_testOnUnsupportedColumnType_4xx(TestNamespace ns) { assertTrue(error.getMessage().contains("does not support column type"), error.getMessage()); } - @Test - void put_createWithUnofferedOptionValue_4xx(TestNamespace ns) { - Table table = createTable(ns); - CreateTestCase request = new CreateTestCase(); - request.setName(ns.prefix("put_bad_strategy")); - request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + ">"); - request.setTestDefinition("tableCustomSQLQuery"); - request.setParameterValues( - List.of( - new TestCaseParameterValue().withName("sqlExpression").withValue("SELECT 1"), - new TestCaseParameterValue().withName("strategy").withValue("SUM"))); - - Exception error = - assertThrows(Exception.class, () -> SdkClients.adminClient().testCases().upsert(request)); - assertTrue(error.getMessage().contains("must be one of"), error.getMessage()); - } - - @Test - void put_createOnUnsupportedColumnType_4xx(TestNamespace ns) { - Table table = createTable(ns); - CreateTestCase request = new CreateTestCase(); - request.setName(ns.prefix("put_length_on_number")); - request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); - request.setTestDefinition("columnValueLengthsToBeBetween"); - request.setParameterValues( - List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); - - Exception error = - assertThrows(Exception.class, () -> SdkClients.adminClient().testCases().upsert(request)); - assertTrue(error.getMessage().contains("does not support column type"), error.getMessage()); - } - @Test void post_rejectedForConformance_leavesNoTestSuite(TestNamespace ns) { Table table = createTable(ns); @@ -971,20 +939,6 @@ void post_rejectedForConformance_leavesNoTestSuite(TestNamespace ns) { assertNull(tableTestSuite(table)); } - @Test - void put_rejectedForConformance_leavesNoTestSuite(TestNamespace ns) { - Table table = createTable(ns); - CreateTestCase request = new CreateTestCase(); - request.setName(ns.prefix("put_no_suite")); - request.setEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>"); - request.setTestDefinition("columnValueLengthsToBeBetween"); - request.setParameterValues( - List.of(new TestCaseParameterValue().withName("minLength").withValue("1"))); - - assertThrows(Exception.class, () -> SdkClients.adminClient().testCases().upsert(request)); - assertNull(tableTestSuite(table)); - } - private static EntityReference tableTestSuite(Table table) { return SdkClients.adminClient() .tables() From 41aa53c933f213652791461227583d621f2ac110 Mon Sep 17 00:00:00 2001 From: Shreyansh Verma Date: Sat, 17 Oct 2026 15:02:51 +0530 Subject: [PATCH 12/12] test(dq): create IT fixtures with a definition that supports their BIGINT column --- .../openmetadata/it/tests/IncidentPaginationIT.java | 10 +++------- .../openmetadata/it/tests/StaleIncidentStatusIT.java | 12 +++--------- .../it/tests/TestCaseDeleteResilienceIT.java | 12 +++--------- .../TestCaseRelationshipRetentionCleanupIT.java | 12 +++--------- .../TestCaseResolutionStatusLegacyTestSuitesIT.java | 10 +++------- .../it/tests/TestCaseSoftDeleteSearchIT.java | 10 +++------- 6 files changed, 18 insertions(+), 48 deletions(-) diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java index 90c7d2f9ac79..78ed728c1e93 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/IncidentPaginationIT.java @@ -80,13 +80,9 @@ public void setup() throws Exception { Table table = createTestTable(); tableFqn = table.getFullyQualifiedName(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. + String testDefFqn = "columnValuesToBeNotNull"; for (int i = 0; i < TEST_DATA_SIZE; i++) { TestCase testCase = createTestCase(table, i, testDefFqn); diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java index 6dd07f1a985b..844b64fedb69 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/StaleIncidentStatusIT.java @@ -49,7 +49,6 @@ import org.openmetadata.schema.type.ColumnDataType; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.exceptions.OpenMetadataException; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.sdk.network.HttpMethod; import org.openmetadata.sdk.network.RequestOptions; import org.openmetadata.service.Entity; @@ -338,18 +337,13 @@ private int countRowsById(String table, String id) { private TestCase createTestCase(Table table, String name) throws Exception { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. CreateTestCase createTestCase = new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn); + .withTestDefinition("columnValuesToBeNotNull"); return client.testCases().create(createTestCase); } diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java index 41af409a5f21..9b3c92cb01be 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseDeleteResilienceIT.java @@ -53,7 +53,6 @@ import org.openmetadata.schema.type.TestDefinitionEntityType; import org.openmetadata.sdk.client.OpenMetadataClient; import org.openmetadata.sdk.exceptions.OpenMetadataException; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.service.Entity; import org.openmetadata.service.jdbi3.TestCaseResultRepository; import org.openmetadata.service.util.FullyQualifiedName; @@ -294,20 +293,15 @@ private DatabaseService createDatabaseService(TestNamespace ns) { private TestCase createSystemTestCase(Table table, String name) { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. return client .testCases() .create( new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn)); + .withTestDefinition("columnValuesToBeNotNull")); } private TestCase createCustomTestCase(Table table, String name, String testDefinitionFqn) { diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java index 040299817fa9..237473d000d6 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseRelationshipRetentionCleanupIT.java @@ -41,7 +41,6 @@ import org.openmetadata.schema.type.ColumnDataType; import org.openmetadata.schema.type.Relationship; import org.openmetadata.sdk.client.OpenMetadataClient; -import org.openmetadata.sdk.models.ListParams; import org.openmetadata.service.Entity; import org.openmetadata.service.util.FullyQualifiedName; import org.openmetadata.service.util.OrphanTestCaseRelationshipCleanup; @@ -141,20 +140,15 @@ private Table createTable(TestNamespace ns) throws Exception { private TestCase createTestCase(Table table, String name) throws Exception { OpenMetadataClient client = SdkClients.adminClient(); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. return client .testCases() .create( new CreateTestCase() .withName(name) .withEntityLink("<#E::table::" + table.getFullyQualifiedName() + "::columns::id>") - .withTestDefinition(testDefFqn)); + .withTestDefinition("columnValuesToBeNotNull")); } private void seedResult(String testCaseFqn) { diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java index dfc527a207b1..5231ef747f0b 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseResolutionStatusLegacyTestSuitesIT.java @@ -106,13 +106,9 @@ void listingApiSurvivesLegacyTestSuitesFieldOnIncidentDoc() throws Exception { List.of( new Column().withName("id").withDataType(ColumnDataType.BIGINT)))); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. + String testDefFqn = "columnValuesToBeNotNull"; TestCase testCase = client diff --git a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java index 2674f2c2bc8c..90c93f7d0839 100644 --- a/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java +++ b/openmetadata-integration-tests/src/test/java/org/openmetadata/it/tests/TestCaseSoftDeleteSearchIT.java @@ -102,13 +102,9 @@ void softDeletingTestCaseDoesNotPollutePropagatedTimeSeriesDocs() throws Excepti List.of( new Column().withName("id").withDataType(ColumnDataType.BIGINT)))); - String testDefFqn = - client - .testDefinitions() - .list(new ListParams().withLimit(1)) - .getData() - .get(0) - .getFullyQualifiedName(); + // A definition that supports the BIGINT `id` column: the server rejects a column type the + // definition does not list, and the first definition the listing returns need not list it. + String testDefFqn = "columnValuesToBeNotNull"; testCase = client