Refactored methods for loading and saving checks#487
Conversation
Added negative int test
|
✅ 249/249 passed, 1 skipped, 50m18s total Running from acceptance #1521 |
refactor
There was a problem hiding this comment.
Pull Request Overview
This refactoring moves all load and save checks methods from DQEngine to separate modules to improve modularity and maintainability. The main changes implement a storage abstraction pattern with different handlers for files, workspace files, tables, and installation configurations.
- Moved validation, serialization, and storage logic into separate modules (
checks_validator.py,checks_serializer.py,checks_storage.py, etc.) - Replaced multiple specific methods with unified
load_checks()andsave_checks()methods that accept storage configuration objects - Refactored utility functions to better handle column name validation and expression parsing
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/databricks/labs/dqx/engine.py | Simplified engine class by removing load/save methods and using new storage handler factory |
| src/databricks/labs/dqx/checks_storage.py | New module implementing storage handlers for different check storage types |
| src/databricks/labs/dqx/checks_serializer.py | New module handling serialization between checks formats (dict <-> DQRule <-> DataFrame) |
| src/databricks/labs/dqx/checks_validator.py | New module containing validation logic moved from engine |
| src/databricks/labs/dqx/checks_resolver.py | New module for resolving check functions |
| src/databricks/labs/dqx/config_loader.py | New module for loading run configurations |
| src/databricks/labs/dqx/utils.py | Enhanced column name utilities with better validation and expression handling |
| src/databricks/labs/dqx/check_funcs.py | Updated function calls and validation for dataset comparison checks |
| tests/ | Updated all tests to use new API methods and storage configuration objects |
| docs/ | Updated documentation to reflect new API |
Comments suppressed due to low confidence (1)
src/databricks/labs/dqx/check_funcs.py:1590
- [nitpick] The function name
_validate_ref_paramsis ambiguous. Consider renaming to_validate_reference_parametersto be more descriptive.
def _validate_ref_params(
dinbab1984
left a comment
There was a problem hiding this comment.
Please find below the manual test evidence details the functionality of DQX loading/saving mechanisms and highlights mismatches with the existing documentation, particularly regarding YAML extension handling and Delta Table saving behavior for installation configurations.
DQX Loading/Saving Mechanisms:
- Local File (FileChecksStorageConfig):
- Saving: Works for
yaml,yml, andjson. - Loading: Works for
ymlandjson; fails foryamlwith an "Unknown extension: yaml" error.
- Saving: Works for
- Workspace File (WorkspaceFileChecksStorageConfig):
- Saving: Works for
yaml,yml, andjson. - Loading: Works for
ymlandjson; fails foryamlwith the same extension error.
- Saving: Works for
- Installation Configuration (InstallationChecksStorageConfig):
- Saving (File): Works for
yaml,yml, andjson. - Loading: Works for
ymlandjson; fails foryamlwith the unknown extension error.
- Saving (File): Works for
- Delta Table (TableChecksStorageConfig):
- Saving: Works for both append and overwrite modes.
- Loading: Works as expected for both modes.
Mismatches With Documentation:
- Extension Handling: YAML vs. YML:
- Documentation Expectation: Both
yamlandymlare typically considered equivalent and supported for YAML files. - Observed Behavior: Loading functions fail for
yamlbut succeed forymlandjson, which is inconsistent with standard YAML handling and likely contradicts the documentation.
- Documentation Expectation: Both
- Table Save Mode (Installation):
- Documentation Statement: "Always overwrite checks in the table for the given run config."
- Observed Behavior: The code comment indicates that the save operation appends the table, contrary to the documentation. This functional mismatch could lead to an accumulation of old rule sets instead of their replacement.
Summary Table
| Storage Method | Save Works | Load Works | YAML Extension Handling | Documentation Match |
|---|---|---|---|---|
| Local File | Yes | Yes (yml, json), No (yaml) |
yaml fails to load |
No – should support yaml |
| Workspace File | Yes | Yes (yml, json), No (yaml) |
yaml fails to load |
No – should support yaml |
| Installation (Table/File) | Yes | Yes (yml, json), No (yaml) |
yaml fails to load |
No – should support yaml |
| Delta Table | Yes (all) | Yes (all) | Not format dependent | Yes (assuming overwrite) |
Key Insights
- Loading checks with the file extension
.yamlfails everywhere, despite documentation and convention indicating it should be supported. Only.yml(and.json) work for loads. - Installation via table may append rather than overwrite, conflicting with the docs' stated behavior for the given run config.
- All save mechanisms otherwise work for the tested formats.
- Delta table loads/saves behave as expected, supporting both append and overwrite.
In summary, while most DQX loading/saving mechanisms work as expected, there are clear mismatches in YAML file extension handling (yaml fails, yml works) and possible confusion around overwrite versus append modes when saving to installation tables.
Issues fixed |
dinbab1984
left a comment
There was a problem hiding this comment.
LGTM , Below is a consolidated test summary table comparing both the new and old (deprecated) APIs for saving and loading Data Quality (DQ) checks in Databricks. For each storage method, the table shows example code snippets, supported formats, working status, and whether the old API issues a deprecation warning—all derived from recent code and observations.
DQ Checks Save/Load: New vs Deprecated APIs Comparison
| Storage Method | Save (New API) | Load (New API) | Save (Deprecated API) | Load (Deprecated API) | Formats Supported | Works? | Deprecated Warning | Notes |
|---|---|---|---|---|---|---|---|---|
| Local File | dq_engine.save_checks(checks, config=FileChecksStorageConfig(location=location)) |
dq_engine.load_checks(config=FileChecksStorageConfig(location=location)) |
dq_engine.save_checks_in_local_file(checks=checks, path=location) |
dq_engine.load_checks_from_local_file(filepath=location) |
yaml, yml, json | Yes | Old API: Yes | New API does not show deprecated warnings. Old API works, but issues a deprecation warning. |
| Workspace File | dq_engine.save_checks(checks, config=WorkspaceFileChecksStorageConfig(location=location)) |
dq_engine.load_checks(config=WorkspaceFileChecksStorageConfig(location=location)) |
dq_engine.save_checks_in_workspace_file(checks=checks, workspace_path=location) |
dq_engine.load_checks_from_workspace_file(workspace_path=location) |
yaml, yml, json | Yes | Old API: Yes | Both new and old APIs work. Old API shows a deprecation warning. |
| Installation - Table | dq_engine.save_checks(checks, config=InstallationChecksStorageConfig(assume_user=True, run_config_name="default")) |
dq_engine.load_checks(config=InstallationChecksStorageConfig(assume_user=True, run_config_name="default")) |
dq_engine.save_checks_in_installation(checks=checks, method="table", assume_user=True, run_config_name="default") |
dq_engine.load_checks_from_installation(method="table", assume_user=True, run_config_name="default") |
Not format-specific | Yes | Old API: Yes | Requires config.yml to define checks_table. New API preferred; old works but issues deprecation warning. |
| Installation - File | dq_engine.save_checks(checks, config=InstallationChecksStorageConfig(assume_user=True, run_config_name="default")) |
dq_engine.load_checks(config=InstallationChecksStorageConfig(assume_user=True, run_config_name="default")) |
dq_engine.save_checks_in_installation(checks=checks, method="file", assume_user=True, run_config_name="default") |
dq_engine.load_checks_from_installation(method="file", assume_user=True, run_config_name="default") |
yaml, yml, json | Yes | Old API: Yes | Requires config.yml to set checks_file. Both APIs work; old API issues deprecation warning. |
| Delta Table | dq_engine.save_checks(checks, config=TableChecksStorageConfig(location=table_name, mode=mode)) |
dq_engine.load_checks(config=TableChecksStorageConfig(location=table_name, run_config_name="default")) |
dq_engine.save_checks_in_table(checks=checks, table_name=table_name, run_config_name="default", mode=mode) |
dq_engine.load_checks_from_table(table_name=table_name, run_config_name="default") |
Not format-specific | Yes | Old API: Yes | Supports append and overwrite. Both APIs work. Old API issues deprecation warning. |
Key Points
- All methods above work for both saving and loading DQ checks with either API.
- New APIs (
.save_checks(),.load_checks()) do not issue deprecation warnings and are the recommended approach. - Old APIs (e.g.,
.save_checks_in_local_file(),.load_checks_from_installation(), etc.) still work but always issue a deprecation warning. - YAML, YML, and JSON are supported for file-based methods; table storage is format-agnostic.
- Certain setups (installation-file, installation-table) require configuration changes in config.yml for correct operation.
- Migration to the new API is highly advised, as deprecated APIs are likely to be removed in future releases.
Changes
DQEngineto a separate module. The previous approach of continuously expanding DQEngine was not scalable or maintainable. This change will make it easier to add more ways of saving and loading checks.DQEnginehad grown beyond its intended scope, so responsibilities were redistributed. The change should provide a good basis for extensions without overloading the engine.DQRulehas been centralized inchecks_serializermodule. It is now possible to convert checks defined in dict toDQRuleand vice versa. Detailed documentation is available here.compare_datasetscheck to make sure only simple expressions are allowedvalidate-checkscli command to enable/disable validating custom check functions (validate-custom-check-functions)BREAKING CHANGES!
If you are loading or saving checks from a storage (file, workspace file, table, installation), you are affected. We are deprecating the the below methods. We are keeping the methods in the
DQEnginebut you should update your code as these methods will be removed in future versions.Loading checks to storage has been unified under
load_checksmethod. The following methods have been removed from theDQEngine:load_checks_from_local_file,load_checks_from_workspace_file,load_checks_from_installation,load_checks_from_table.Saving checks in storage has been unified under
load_checksmethod. The following methods have been removed from theDQEngine:save_checks_in_local_file,save_checks_in_workspace_file,save_checks_in_installation,save_checks_in_table.The
save_checksandload_checkstakeconfigas a parameter, which determines the storage types used. The following storage configs are currently supported:FileChecksStorageConfig: file in the local filesystem (YAML or JSON)WorkspaceFileChecksStorageConfig: file in the workspace (YAML or JSON)TableChecksStorageConfig: a tableInstallationChecksStorageConfig: storage defined in the installation context, using either thechecks_tableorchecks_filefield from the run configuration.The
load_run_configmethod has been moved toconfig_loader.RunConfigLoader, as it is not intended for direct use and falls outside theDQEnginecore responsibilities.Tests