Fixes #33230: [DQ Threshold W1.2] Deviation-from-statistic evaluation + migrate 12 statistical tests - #33444
Fixes #33230: [DQ Threshold W1.2] Deviation-from-statistic evaluation + migrate 12 statistical tests#33444TeddyCr wants to merge 2 commits into
Conversation
…stical tests Adds the second threshold semantic: a tolerance on an aggregate statistic rather than on rows. Range tests widen their bounds by the threshold — ABSOLUTE subtracts/adds the raw value, PERCENTAGE a share of |bound| so a negative bound widens outward (-100 at 5% gives -105, not -95). Exact-value tests tolerate a delta around the expected value on the same two units. The widening happens in `BaseTestValidator.get_bounds()`/`apply_bound_tolerance()`, downstream of `get_min_bound`/`get_max_bound`, so it also holds for validators that resolve their bounds dynamically by overriding those getters — widening inside the getters would silently no-op for every dynamic-assertion test case. Degenerate cases are handled explicitly rather than papered over: a bound or expected value of 0 has no percentage, so the test case stays strict on that side and a warning says why; an unset bound stays ∓inf and the tolerance is a no-op there. A threshold of 0 reproduces today's verdict exactly. The `failureThreshold`/`thresholdUnit` fields this builds on are added here too, since #33227 has not landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
Ingestion
safe to test
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static final String UPDATE_FIELDS = | ||
| "owners,entityLink,testSuite,testSuites,testDefinition,dimensionColumns,topDimensions"; | ||
| private static final String PATCH_FIELDS = | ||
| "owners,entityLink,testSuite,testSuites,testDefinition,computePassedFailedRowCount,useDynamicAssertion,dimensionColumns,topDimensions"; | ||
| "owners,entityLink,testSuite,testSuites,testDefinition,computePassedFailedRowCount,useDynamicAssertion,failureThreshold,thresholdUnit,dimensionColumns,topDimensions"; |
There was a problem hiding this comment.
These fields are included in PATCH_FIELDS but not UPDATE_FIELDS. When an older or partial PUT client omits failureThreshold or thresholdUnit while changing another field, the repository does not hydrate their stored values and then persists the request object with them set to null. This silently removes the configured tolerance, so later data-quality runs revert to strict evaluation.
| private static final String UPDATE_FIELDS = | |
| "owners,entityLink,testSuite,testSuites,testDefinition,dimensionColumns,topDimensions"; | |
| private static final String PATCH_FIELDS = | |
| "owners,entityLink,testSuite,testSuites,testDefinition,computePassedFailedRowCount,useDynamicAssertion,dimensionColumns,topDimensions"; | |
| "owners,entityLink,testSuite,testSuites,testDefinition,computePassedFailedRowCount,useDynamicAssertion,failureThreshold,thresholdUnit,dimensionColumns,topDimensions"; | |
| private static final String UPDATE_FIELDS = | |
| "owners,entityLink,testSuite,testSuites,testDefinition,failureThreshold,thresholdUnit,dimensionColumns,topDimensions"; | |
| private static final String PATCH_FIELDS = | |
| "owners,entityLink,testSuite,testSuites,testDefinition,computePassedFailedRowCount,useDynamicAssertion,failureThreshold,thresholdUnit,dimensionColumns,topDimensions"; |
Knowledge Base Used: Metadata entity lifecycle
| "type": "boolean", | ||
| "default": false | ||
| }, | ||
| "failureThreshold": { | ||
| "description": "Tolerance applied when evaluating this test case. A deviation within the threshold is still reported as a success. Defaults to 0, which evaluates the test case strictly.", | ||
| "type": "number", | ||
| "minimum": 0, | ||
| "default": 0 | ||
| }, | ||
| "thresholdUnit": { | ||
| "description": "Unit `failureThreshold` is expressed in.", |
There was a problem hiding this comment.
The new threshold settings are persisted on a test case, but the TestCase CSV exporter and importer do not write or read either field. Exporting and re-importing a test with a nonzero tolerance therefore omits its failureThreshold and thresholdUnit; the threshold defaults to zero, making the restored test strict and potentially turning previously successful executions into failures.
Knowledge Base Used:
| "type": "boolean", | ||
| "default": false | ||
| }, | ||
| "failureThreshold": { | ||
| "description": "Tolerance applied when evaluating this test case. A deviation within the threshold is still reported as a success. Defaults to 0, which evaluates the test case strictly.", | ||
| "type": "number", | ||
| "minimum": 0, | ||
| "default": 0 | ||
| }, | ||
| "thresholdUnit": { | ||
| "description": "Unit `failureThreshold` is expressed in.", |
There was a problem hiding this comment.
CreateTestCase accepts a failure threshold for every test definition, but only the twelve migrated validators call the new threshold-aware helpers. An API or SDK caller can therefore save a nonzero threshold on another test type without receiving a validation error, while that validator continues evaluating strictly and silently ignores the saved configuration.
Knowledge Base Used:
✅ Generated Sources Auto-UpdatedThe generated TypeScript types ( |
Code Review ✅ ApprovedImplements deviation-from-statistic evaluation with configurable failure thresholds and migrates 12 statistical tests to use the new OptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
The Python checkstyle failed. Please run You can install the pre-commit hooks with |
Jest test CoverageUI tests summary
|
|
✅ Playwright Results — workflow succeededValidated commit ✅ 876 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 54m 15s ⏱️ Max setup 4m 53s · max shard execution 22m 58s · max shard-job elapsed before upload 26m 51s · reporting 7s 🌐 230.41 requests/attempt · 2.15 app boots/UI scenario · 50.51% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
What olivaw checked (machine-observed, §8.2/A.1)
42d153541b0679aa2e8ba4ab206ceb88b2a2873d8cff28cb3cb530eb9613d560f4b5f9979674280fNo build or test command is configured for this workspace, so only the diff's existence was checked. Set
run_config.build_cmd/test_cmdto have olivaw verify its own work.What the agent says it did (unverified)
Greptile Summary
This PR adds configurable absolute or percentage failure tolerances to data-quality test cases and migrates twelve statistical validators to apply them.
Confidence Score: 2/5
The PR does not appear safe to merge because three previously reported threshold contract and persistence defects remain unresolved.
The current code still does not preserve threshold fields through partial PUT updates, does not include them in test-case CSV export/import, and accepts thresholds for validators that do not apply them. The changes since the previous review only relocate generated TypeScript enum declarations, leaving all three existing findings outstanding.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Test-case schema] --> B[Generated Java and Python models] A --> C[Generated TypeScript models] B --> D[TestCase repository] D --> E[Persisted threshold configuration] E --> F[BaseTestValidator] F --> G[Resolve expected value or bounds] G --> H[Apply absolute or percentage tolerance] H --> I[Data-quality verdict and result]Reviews (2): Last reviewed commit: "Update generated TypeScript types and de..." | Re-trigger Greptile
Context used (3)