Repository navigation
Fixes 35274: Check test parameters against their definitions and fix open-window and boolean-set runs - #35629
Conversation
…ean and open-window tests correctly
github-actions
Bot
added
the
safe to test
…-parameters # Conflicts: # bootstrap/sql/migrations/native/2.1.0/mysql/postDataMigrationSQLScript.sql # bootstrap/sql/migrations/native/2.1.0/postgres/postDataMigrationSQLScript.sql
🔄 Playwright impact map auto-refreshedThis PR touched specs or UI source that changed the source→spec routing map. I regenerated What is this file? It is the auto-generated half of Playwright's PR planner. It routes "if source X changes, run specs Y" by walking spec imports and cross-referencing What if I want to regenerate locally instead? Run this before pushing your next change to skip the bot commit: python3 .github/scripts/generate_playwright_impact_map.py
git add .github/playwright/impact-map.generated.json
git commit --amend --no-edit # or a separate commit |
…/open-metadata/OpenMetadata into fix/dq-generated-test-parameters
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
… rows are sampled
TeddyCr
left a comment
TeddyCr
left a comment
There was a problem hiding this comment.
Requesting changes on the create-only gate for TestCaseDefinitionConformance. Details inline.
| if (!update) { | ||
| TestCaseDefinitionConformance.validate(test, testDefinition, table, entityLink); | ||
| } |
There was a problem hiding this comment.
Create-only makes this check easy to bypass:
- PATCH: every PATCH calls
prepareInternal(updated, true)(EntityRepository.java:4838), so it never runs this check. PATCHingstrategytoSUMon a valid test case is stored unchecked. - PUT:
EntityResource.createOrUpdatecallsprepareInternal(entity, true)even when it creates a new row (see the note atEntityRepository.java:3373-3376). So every test case created by PUT skips the check, and that includes ingestion and most SDK clients.
I agree we shouldn't re-check the whole entity on update. A row saved before this rule, or one whose column was later retyped by ingestion, would return 400 on any edit, even a description change. Can we validate only what changed?
- In
TestCaseUpdater.entitySpecificUpdate, whereoriginalandupdatedare both available: check a parameter'soptionValuesonly when that parameter's value changes, and check the column type only whenentityLinkortestDefinitionchanges. - For PUT-create: run the check from
createNewEntity, the path every create goes through, for the same reasoncreatedAt/createdByare set there.
That keeps old and retyped rows editable and enforces the rule on every write path. The ADR's Consequences section and patch_doesNotRecheckDefinitionConformance would need updating to match, plus ITs for PATCH introducing an invalid option and for PUT creating one.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Update: I've rewritten this review so it lists exactly what to change in this PR.
The rule itself, and the create/update split (create checks the whole test case, the updater checks only what changed), are good. What needs to change is where the create check lives. It's currently in three places, and two of them exist only to work around a test-case framework bug. I've filed that bug separately as #35667 and will fix it there.
Background. The repo's pattern is if (!update) in prepare() (there are 29 of these gates in jdbi3/, including the disabled-definition check right above this one), plus the updater for changes. That gate misses two test-case create paths, and the disabled-definition check misses them today too:
- PUT-create:
TestCaseResource.createOrUpdateprepares withupdate = truebefore it knows whether the row exists. - CSV import:
TestCaseCsvnever callsprepareat all.
These are bugs in how those callers reach prepare, not in this rule, so #35667 fixes them at the callers. With that fix, the plain !update gate covers every create path.
Changes requested in this PR
TestCaseRepository.prepare: replaceif (isNewTestCase(test, update))withif (!update), keeping it where it is now (after validation, beforegetOrCreateTestSuite). DeleteisNewTestCase.TestCaseRepository.createNewEntity: delete the override.TestCaseRepository.createManyTestCases: delete it, and changeTestCaseResource.createManyback torepository.createMany(uriInfo, testCases).createManyalready runsprepareInternal(e, false)on every row before any chunk is stored, so with item 1 a single bad row still means nothing is stored (createMany_withOneUnofferedOptionValue_createsNonekeeps passing).validateDefinitionConformance: delete it. Nothing calls it once items 2 and 3 are done.- Keep
validateChangedConformancein the updater as it is. It covers PUT, PATCH and CSV updates, sinceupdateForImportruns the updater. - ITs: keep
put_createWithUnofferedOptionValue_4xx,put_createOnUnsupportedColumnType_4xxandput_rejectedForConformance_leavesNoTestSuite. They depend on #35667 item 1, so this PR merges after #35667. Everything else in the PR's IT list passes with items 1–5 alone. - ADR
2026-10-12-test-cases-are-checked-against-their-definition-at-create: in Decision, replace the paragraph aboutisNewTestCase/createNewEntity/createManyTestCaseswith: "prepare()checks a test case whenupdateis false, before it creates the table's basic test suite; every create path reaches it that way (#35667)." In Consequences, replace the CSV paragraph with a reference to #35667, and remove the dropped tests from Guard if any no longer apply.
After this, every create is checked once, in prepare, before any side entity is created, and every update is checked once, in the updater. No special case for test cases is needed.
| * 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) { |
There was a problem hiding this comment.
Change: delete isNewTestCase and change the call site in prepare to if (!update).
Why: prepare(..., update) is the shared hook, and update tells it whether this is a create. It only looks unreliable here because TestCaseResource.createOrUpdate and the CSV dry run pass true without checking whether the row exists. That's a caller bug, now tracked in #35667. Working around it here costs an extra findByNameOrNull on every PUT and PATCH, on top of the lookup createOrUpdate already does, and makes prepare set the FQN itself.
There was a problem hiding this comment.
Removed; back to if (!update) like the disabled-definition check. PUT-create is left to the caller fix in #35667.
| * (ADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create). | ||
| */ | ||
| @Override | ||
| protected TestCase createNewEntity(TestCase test) { |
There was a problem hiding this comment.
Change: delete this override.
Why: no other repository overrides createNewEntity. With prepare doing the check, POST and PUT-create would run it twice, re-fetching the definition and table each time. The only path it covers that prepare doesn't is the CSV import, which never calls prepare. #35667 fixes that at TestCaseCsv.
There was a problem hiding this comment.
Override removed. I didn't add a check in TestCaseCsv, since your summary moves the CSV caller fix to #35667
| * createMany stores in chunks, each in its own transaction, so the whole batch is checked before | ||
| * any of it is stored. | ||
| */ | ||
| public List<TestCase> createManyTestCases(UriInfo uriInfo, List<TestCase> testCases) { |
There was a problem hiding this comment.
Change: delete createManyTestCases and validateDefinitionConformance, and change TestCaseResource.createMany back to repository.createMany(uriInfo, testCases).
Why: the Javadoc's reason (chunks stored in separate transactions) doesn't hold. EntityRepository.createMany runs prepareInternal(e, false) on every row before createManyEntities stores anything, so with the !update gate a single bad row already rejects the whole batch with nothing stored. This loop just checks every row a second time.
There was a problem hiding this comment.
Dropped it; one bad row still means no test case is stored. The suites earlier rows' prepare() created stay, as with any other prepare rejection in a batch, and the ADR says so.
🚦 Removed from the merge queue —
|
Code Review ✅ Approved 1 closed / 1 findings🟡 Medium risk · Server-side test-definition checks and revised data-quality counting/parsing could reject valid test cases or change reported failures when metadata differs. Checks new data quality test parameters against their definitions' option values and supported column types, and fixes open-window failed-row counts and boolean set comparisons. Resolved this session: an unset or blank ✅ 1 closed✅ Edge Case: Unset/blank strategy runs as ROWS but skips failed-row samples
VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
Describe your changes:
Fixes #35274
Companion PR: open-metadata/ai-platform#1745 (test generation conforms to these definitions; it merges first). Related: open-metadata/ai-platform#1650, #35276 (2.0 line).
Main-line counterpart of #35276 (2.0, merged), with TeddyCr's review on it built in, plus the server-side half that 2.0 did not ship. Data quality tests could pass or fail for reasons unrelated to the data, and AI-generated tests hit every one of them (open-metadata/ai-platform#1650).
Runner (ingestion)
is_unbounded,SQAValidatorMixin._count_rows_outsideshared by both SQL between validators and their failed-row filters).parse_set_values,get_set_param_value): on a column OpenMetadata records as BOOLEAN,"true"/"false"(any case) become booleans, so the comparison is boolean to boolean (Databricks aborted on the string mismatch, Data quality: open between bounds count every row as failed, and boolean sets compare as strings #35274). A datalake set keeps its values as written, becauseGenericDataFrameColumnParsertypes a column of"True"/"False"strings as BOOLEAN (TeddyCr's datalake regression on Fixes #35274: count no failed rows for an open between bound and compare boolean sets as booleans #35276).[false],[null]) is read:literal_evalfirst, then JSON, so every value that parsed before parses the same. Found while reproducing on main: a generatedforbiddenValues: [false]Aborted with "malformed node or string".try, so a test on a missing column is Aborted by the validator's own handler.Server
optionValues(case-insensitive; blank is unset) or its column's type is outside a non-emptysupportedDataTypes. The check runs inprepare()whenupdateis false, so POST,createMany, the MCP tool and ODCS materialization get it now; PUT-create and the CSV import get it with Test case PUT-create and CSV import skip TestCaseRepository.prepare's create-time checks #35667, which fixes how those callers reachprepare().Definitions
tableCustomSQLQuery.strategysays which option fits which query (the 2.0 text, verbatim). The2.1.0migration pushes it, and Fixes 31760: clarify MatchEnum behavior for column values #32123'scolumnValuesToBeInSettexts (matchEnum,allowedValues, the definition description, which never got a migration), to existing installs, since seeding only creates missing definitions.Type of change:
High-level design:
is_unbounded) is used by the checker, the row count and the failed-row filter; set parsing goes through one cooperative hook (get_set_values_type: the SQLAlchemy mixin returns the column type OpenMetadata records, mapped to SQLAlchemy; pandas returns none). Recorded inADR:2026-10-16-a-set-parameter-is-read-in-its-column-type.prepare()checks a test case whenupdateis false, after its other validations and before it creates the table's basic test suite, so a rejected create leaves no suite behind. That is the gate the disabled-definition check uses; PUT-create (createOrUpdateprepares withupdatetrue) and the CSV import (noprepare()call) miss it today, for both checks, and Test case PUT-create and CSV import skip TestCaseRepository.prepare's create-time checks #35667 fixes those callers.createManyprepares every test before it stores any, so one rejected test fails the batch with no test case stored. Updates go throughTestUpdater→validateUpdate, which loads the definition and table only when a parameter value or the target changed (an inspection query is stored by an update that changes neither). It does not checkuseDynamicAssertionagainstsupportsDynamicAssertion, because the Collate UI sends it from a toggle left on. Recorded inADR:2026-10-12-test-cases-are-checked-against-their-definition-at-create(v2).2.1.0post-data scripts, both dialects, each statement guarded so it never writes NULL and idempotent. Main's2.0.6post-data scripts are empty (Fixes #35274: count no failed rows for an open between bound and compare boolean sets as booleans #35276's statements live only on2.0), so a 2.0.5 or older install reaches the new text only through2.1.0. On a 2.0.6 install thestrategystatement is byte-identical to 2.0's and is skipped by the migration log, and thecolumnValuesToBeInSetstatements replace 2.0'smatchEnumwording with main's.Tests:
Use cases covered
["true","false"]/["true"]on a BOOLEAN column compares as booleans (InSet and NotInSet); a"True"/"False"string column on a datalake stays strings; a JSON[false]set runs.strategy=SUM, or a length test on an INT column, is rejected with 400 by POST and bycreateMany(which then stores no test case), and by PUT once Test case PUT-create and CSV import skip TestCaseRepository.prepare's create-time checks #35667 lands; a rejected POST leaves no test suite behind; PATCHing a valid test tostrategy=SUMis rejected; a valid row whose column is later retyped stays editable by PATCH and PUT.supportedDataTypesoroptionValues, so their test cases pass the check. A test library definition is held to the types and options its author declared.Unit tests
TestCaseDefinitionConformanceTest(18, incl. what an update changes, and the seed invariant ai-platform relies on: every dynamic definition marks its two bounds with avalidationRule),SeededTestDefinitions(helper moved out ofTestCaseThresholdValidatorTest); ingestiontest_utils.py(parse_set_values, JSON sets,is_boolean,is_unbounded),test_between_row_violations.py(open window, one open side, unfiltered sample),test_validations_databases.py(_no_boundsx2,_boolean_stringsx2,_boolean_json,_missing_column,_lowercase_strategy,_blank_strategy),test_validations_datalake.py(string booleans),test_row_threshold.py(fixture declares the table, since a set is now parsed in its column's type).TestCaseDefinitionConformanceTest,TestCaseRepositoryTest,TestCaseThresholdValidatorTest): 377/377.tests/unit/observability/data_quality: 813 passed. 6 Databend dialect errors intest_databend_profiler_sql.pyare a missing local driver and fail identically onmain.--covper module): set validators 84 to 93%, between validators 83 to 86%,pandas_validator_mixin90%,sqa_validator_mixin66%,utils59%,base_test_handler55% (a 1,300-line module; every function this PR adds has a test).mvn spotless:applyno diff, Java impact map complete.Backend integration tests
TestCaseResourceIT:post_testWithUnofferedOptionValue_4xx,post_testOnUnsupportedColumnType_4xx,patch_toUnofferedOptionValue_4xx,createMany_withOneUnofferedOptionValue_createsNone,patch_keepsATestOnARetypedColumnEditable(creates a valid length test on VARCHARname, retypes the column to INT, then PATCHes),post_rejectedForConformance_leavesNoTestSuite. The PUT-create ITs belong with Test case PUT-create and CSV import skip TestCaseRepository.prepare's create-time checks #35667, which makes PUT-create reach the check.OrphanedTimeSeriesCleanupITandDomainIsolationITtook the first listed definition for their fixture and put it on an integer column; both now usecolumnValuesToBeNotNull.ThresholdParameterMigrationITexecutes everyUPDATE test_definitionin2.1.0twice on both engines, including the four new statements.Local Java tests (
make java_affected_run, ond6215060c012, 97 min): unit 371/371 passed; integration 4,456 run, 6 failed (MySQL + Elasticsearch: parallel 2,929, isolated 374, rdf 48, cache 1; Postgres + OpenSearch: parallel 1,034, isolated 6, search-it 64).Each failure rerun alone on
d03d713f738(production code unchanged since):OrphanedTimeSeriesCleanupIT: the new create check rejected its fixture (a length test on a BIGINT column). Fixture fixed ind03d713f738; rerun 6/6 passed.SearchAggregateRbacIT(4 tests): a 60s wait for indexing timed out under load; rerun 4/4 passed.DbToEsCountReconciliationIT: not caused by this PR, fails on rerun too.userhas es=25, db=22: the 3 usersSearchAggregateRbacITsoft-deletes, which the ES count includes and the DB count does not.Not run locally: 10
*UIITclasses, which run in the nightly k8s Java IT job.After the review change (every create path, changed-only updates), the tests it can affect, on
ca1427dc7d3+ that change: unit 377/377 (the plan's 12 classes); every IT class that creates or updates test cases (38; 11 more than the plan selects), 2,607 run: MySQL + Elasticsearch 2,250 with 2 failures, Postgres + OpenSearch 352 and search-it 5, all passed.DomainIsolationIT.test_incidents_restrictedUserSeesOnlyOwnDomain: the create check rejected its fixture (a length test on INTid); fixture fixed, class rerun 13/13 passed.WorkflowDefinitionResourceIT.test_NewApprovalRunSupersedesStalePendingTask: a tag approval on a schema, no test case; a 60s wait timed out under load, rerun alone passed.After moving the new-row check before suite creation: unit 377/377;
TestCaseResourceIT360 run, 0 failed (all 9 conformance ITs pass); 32 more of the classes above, 1,770 tests, 0 failed, before that run was cut short to save time.Current design (
if (!update)inprepare(), the updater for changes): unit 377/377;TestCaseResourceIT355 run, 3 failed: the three PUT-create ITs, which depend on #35667 and were then removed from this PR, so the 352 that remain all passed or were skipped;DataContractResourceIT410 run, 0 failed;TestCaseCsvDomainIsolationIT7/7.Ingestion integration tests
64b178a3662: 48/48 passed.data_quality/test_failed_row_samples.py10,postgres/test_data_quality.py17,mysql/test_data_quality.py8,datalake/test_data_quality.py6,test_suite/test_workflow.py6,databend/test_data_quality.py1.Playwright (UI) tests
Pages/Entity.spec.ts: the three test cases that putcolumnValueLengthsToBeBetweenon the NUMERIC first column (now rejected by the server) usecolumnValuesToBeBetweenwith the same values. Every other Playwright test case was checked: its column type is supported and its option values are offered.Manual testing performed
:8585, Postgres), tableorders(100 rows: 18 statuses outside OPEN/CLOSED, 33 inactive, no negative amounts), registered through the metadata workflow.forbiddenValues: [false]Aborted, now Failed with the 33 false rows.OpenMetadataOperations migrateof this commit on fresh MySQL 8.0 and Postgres 17:2.0.6scripts, then this2.1.0: all four descriptions equal the seed, 25 definitions, parameters intact.2.1.0already applied, then this2.1.0: the new statements run on the rerun of the newest version; same result.migrateprints "No Server Migration To be Run" on all four; on the 2.0.6 path thestrategystatement is logged once, under2.0.6.createNewEntity), by API onorders: POST, PUT andcreateManyrejectstrategy=SUMand a length test on NUMERIC (createManystores neither test of the batch); PATCHROWS→SUMis rejected,ROWS→countaccepted; with the column retyped VARCHAR→INT under a length test, PATCH of its description andminLengthand an unchanged PUT all return 200. Before the change, PUT and PATCH accepted both. PUT-create is now left to Test case PUT-create and CSV import skip TestCaseRepository.prepare's create-time checks #35667; the PATCH and retyped-column results hold, the updater check being unchanged since.test_great_expectation_integration.py, 1/1) and data diff IT (test_data_diff.py, 17 passed, 2 skipped byTODOmarks) pass. dbt was not run end to end; its definition and test-case requests, built with its owndbt_utilshelpers and written by PUT as its sink does, were created fornot_null,unique,accepted_valuesand a singular test. A test library definition (VARCHAR/STRING,mode∈ STRICT/LOOSE) accepted a valid case and rejected a NUMERIC column andmode=bogus.UI screen recording:
Not applicable: no UI change (a Playwright spec's test data only).
Checklist:
I have read the CONTRIBUTING document.
My PR title is
Fixes 35274: Check test parameters against their definitions and fix open-window and boolean-set runsMy PR is linked to a GitHub issue via
Fixes #35274above.I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: I updated the migration scripts or explained why it is not needed. (Seed data:
2.1.0migration.)For a UI feature or feature task: not applicable.
I have added tests (unit / integration / Playwright as applicable) and listed them above.
Decision check: adds
docs/decisions/2026-10-12-test-cases-are-checked-against-their-definition-at-create.mdanddocs/decisions/2026-10-16-a-set-parameter-is-read-in-its-column-type.md.I have added a test that covers the exact scenario we are fixing.
The reviewed changes appear safe to merge, with no new actionable issues found.Summary
Checks test parameters against their definitions and fixes open-window counts and boolean sets.
Reviews (8) · Last reviewed commit: "test(dq): create IT fixtures with a defi..." · Reviewed by Greptile