feat: implement semantic rule validation for DQ rules (#1169)#1203
feat: implement semantic rule validation for DQ rules (#1169)#1203Vsatyam013 wants to merge 6 commits into
Conversation
|
|
|
All commits in PR should be signed ('git commit -S ...'). See https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
Vsatyam013
force-pushed
the
feat/1169-semantic-validation
branch
from
d9d5ef7 to
379a5ba
Compare
Vsatyam013
force-pushed
the
feat/1169-semantic-validation
branch
from
ff01ddc to
f84132b
Compare
ghanse
left a comment
ghanse
left a comment
There was a problem hiding this comment.
Hi @Vsatyam013 , the linked issue focuses more on validating the entire ruleset to detect the following issues:
- Detect if a duplicate rule already exists with the same run config
- Detect if a similar rule exists with different arguments (e.g. a narrower threshold of is_in_range)
Can you make some changes to address these more specifically? I think it should be simpler because the check function inputs are mostly structured. We can document any limitations when users execute checks with Spark expressions.
ghanse
added
under-review
|
@Vsatyam013 do you plan to complete the PR, and address review comments? there was no update since 4 weeks. |
Vsatyam013
force-pushed
the
feat/1169-semantic-validation
branch
from
ea0c092 to
da2897a
Compare
|
Hi @mwojtyczka, sorry for the delay. I've reworked the implementation based on @ghanse's feedback. The SQL keyword validation approach has been removed and replaced with ruleset-level semantic checks, duplicate rule detection and conflicting rule detection (same function and column with different arguments). The validation is now integrated into validate_checks, save_checks, and load_checks with a configurable semantic_validation_mode parameter (warn, fail, or None). CHANGELOG changes have also been reverted.Can you review it? |
| @@ -53,10 +53,9 @@ | |||
| from databricks.labs.dqx.errors import InvalidCheckError, InvalidConfigError, InvalidParameterError | |||
| from databricks.labs.dqx.utils import list_tables, safe_strip_file_from_path, resolve_variables, VariableValue | |||
| from databricks.labs.dqx.io import is_one_time_trigger | |||
|
|
|||
| from .semantic_validator import SemanticValidator, SemanticValidationMode | |||
There was a problem hiding this comment.
use full notation, not relative import
| self, | ||
| config: BaseChecksStorageConfig, | ||
| variables: dict[str, VariableValue] | None = None, | ||
| semantic_validation_mode: str | None = SemanticValidationMode.WARN, # <-- ADD THIS |
There was a problem hiding this comment.
| semantic_validation_mode: str | None = SemanticValidationMode.WARN, # <-- ADD THIS | |
| semantic_validation_mode: str | None = SemanticValidationMode.WARN, |
| ) -> list[dict]: | ||
| """Load DQ rules (checks) from the storage backend described by *config*. | ||
|
|
||
There was a problem hiding this comment.
revert these indentation changes pls
| """ | ||
| handler = self._checks_handler_factory.create(config) | ||
| checks = handler.load(config) | ||
| merged_variables = self._merge_variables(variables) | ||
| return resolve_variables(checks=checks, variables=merged_variables) | ||
| resolved = resolve_variables(checks=checks, variables=merged_variables) # <-- RENAME from return value |
| arguments = SemanticValidator._get_arguments(check) | ||
| criticality = check.get("criticality", "error") | ||
| filter_expr = check.get("filter") | ||
| return (function, tuple(sorted(arguments.items())), criticality, filter_expr) |
There was a problem hiding this comment.
detect_duplicates crashes on any list-valued argument (TypeError: unhashable type: 'list'). This key embeds tuple(sorted(arguments.items())), and detect_duplicates uses the result as a dict key (key in seen / seen[key] = idx). Many common checks carry list arguments — e.g. is_in_list allowed=["A","B","C"], range bounds, for_each_column lists — so the tuple contains a list and dict insertion raises TypeError: unhashable type: 'list'. I reproduced it with a single is_in_list rule. Because apply() runs by default (WARN) inside validate_checks/load_checks/save_checks, this would crash normal rulesets once the indentation error is fixed. Suggest serializing argument values to a hashable form, e.g. json.dumps(arguments, sort_keys=True, default=str), and adding a regression test that includes a list-valued argument.
mwojtyczka
left a comment
mwojtyczka
left a comment
There was a problem hiding this comment.
Thank you for the PR. I feft some comments. The engine.py does not compile due to identation issues. Please also update the PR description.
4 participants
Changes
Implements semantic validation for DQ rule expressions as part of issue #1169.
Adds a
SemanticValidatorclass that checks SQL expressions for forbiddendestructive keywords (DROP, TRUNCATE, DELETE, UPDATE, INSERT, ALTER) before
rules are applied. Validation results are collected into
ChecksValidationStatusrather than raising exceptions, consistent with the existing validation contract.
Linked issues
Resolves #1169
Tests
Documentation and Demos