Repository navigation
fix: pass custom_checks through to DQEngine.save_checks - #1501
GewoonMaarten wants to merge 5 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 |
…tadata when saving checks
446c26c to
3988a92
Compare
| def _resolve_name_and_fingerprint( | ||
| original_check: dict, normalized_check: dict, custom_checks: dict[str, Callable] | None = None | ||
| ) -> tuple[str | None, str]: |
There was a problem hiding this comment.
Let's add a line in the Args section of the docstring for custom_checks
| run_config_name: str = "default", | ||
| rule_set_fingerprint: str | None = None, | ||
| created_at: datetime | None = None, | ||
| custom_checks: dict[str, Callable] | None = None, |
There was a problem hiding this comment.
Need to also add this to the Args section in the docstring.
| custom_check_functions: Optional dictionary with custom check functions (e.g., *globals()* of | ||
| the calling module). |
There was a problem hiding this comment.
I think this should be added to the ChecksStorageHandler.save method instead?
| def save( | ||
| self, | ||
| checks: list[dict], | ||
| config: LakebaseChecksStorageConfig, | ||
| custom_check_functions: dict[str, Callable] | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
You will need to pass custom_check_functions through a few methods:
_save_checks_to_lakebase -> _normalize_checks
| config: BaseChecksStorageConfig, | ||
| variables: dict[str, VariableValue] | None = None, | ||
| semantic_validation_mode: str | None = ChecksSemanticValidationMode.WARN, | ||
| custom_check_functions: dict[str, Callable] | None = None, |
There was a problem hiding this comment.
Similar to the other methods, this argument needs a line in the method docstring.
| checks: list[dict], | ||
| config: InstallationChecksStorageConfig, | ||
| custom_check_functions: dict[str, Callable] | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
Need to pass customer_check_functions to handler.save here.
ghanse
added
under-review
mwojtyczka
left a comment
mwojtyczka
left a comment
There was a problem hiding this comment.
Automated code-review findings (Isaac Review pass, against 752fe62a). Ranked by severity; line numbers refer to that commit. #1–#3 were verified directly against the PR blob.
The PR threads custom_check_functions into the save path so checks referencing custom functions can be fingerprinted/validated at save time. That wiring is correct for the direct TableChecksStorageHandler path (checks_storage.py:433 passes it to both compute_rule_set_fingerprint_by_metadata(...) and custom_checks=), but the two most common indirect paths are still broken.
1. InstallationChecksStorageHandler.save drops custom_check_functions when delegating (High)
src/databricks/labs/dqx/checks_storage.py:1209
handler, config = self._get_storage_handler_and_config(config)
return handler.save(checks, config) # custom_check_functions never forwardedsave accepts custom_check_functions (line 1198) but delegates with only (checks, config). When the resolved backend is a table or lakebase, deserialize_checks then runs with custom_checks=None and fail_on_missing=True → InvalidCheckError for any check referencing a custom function. Since the installation handler is the primary way installed DQX saves checks, the fix does not reach the main path.
2. LakebaseChecksStorageHandler.save accepts the param but never threads it (High)
src/databricks/labs/dqx/checks_storage.py:654
save (line 985) → _save_checks_to_lakebase(checks, config, engine) (line 1014) → _normalize_checks_for_lakebase, which calls:
rule_set_fingerprint = compute_rule_set_fingerprint_by_metadata(checks) # no custom functionsvs. the Table path at line 433 which passes custom_check_functions. Saving custom checks to Lakebase still raises InvalidCheckError, and the newly added parameter is silently ignored.
3. custom_check_functions Args block is on the wrong abstract method (Med)
src/databricks/labs/dqx/checks_storage.py:335
The Args entry documenting custom_check_functions sits in the abstract load docstring, whose signature is load(self, config: T) (no such parameter). The abstract save (line 342), which does take it, has no Args section. API docs will render a nonexistent param for load and none for save.
4. save_checks public docstring omits the new parameter (Low)
src/databricks/labs/dqx/engine.py:1741
AGENTS.md requires Google-style docstrings documenting all args on public functions. save_checks documents checks/config/variables/semantic_validation_mode but not custom_check_functions, so users of this public API have no guidance on when/how to pass it (e.g. globals() of the calling module).
5. Concrete save() overrides don't document the parameter (Low)
src/databricks/labs/dqx/checks_storage.py:1076 (and the WorkspaceFile/File/Volume/Table/Lakebase/Installation overrides)
None of the handler save() docstrings describe custom_check_functions. For the file-based handlers (WorkspaceFile/File/Volume) it is also accepted-but-unused with no note, so a caller can't tell it is a no-op there versus load-bearing for the Table path.
The file-based handlers (WorkspaceFile/File/Volume) legitimately don't need the parameter — they only serialize YAML/JSON — so accepting-and-ignoring there is acceptable, just undocumented. No test coverage was added for the new parameter in the diff; a regression test that saves a custom-function check via the Installation and Lakebase paths would have caught #1 and #2.
This branch has not been deployed
3 participants
Changes
Linked issues
Resolves #1467
Tests
Documentation and Demos