Airflow Data Quality Provider part 1#69575
Conversation
|
This is part 1 , the UI plugin is part of this big PR #69413, i will ship that separate. but to show how the UI looks like here is some screenshots #69413 (comment) (this is minimal what had done with help of UI not an UI expert anyone can modify or help with this 😄 ) |
cbdf1fd to
9dac869
Compare
ef557f3 to
106b8de
Compare
377ca0a to
1047525
Compare
|
sorry @Lee-W i meant to add and it got removed, readded back. |
| config["conn_id"] = conn_id | ||
| if table: | ||
| config["table"] = table | ||
| asset.extra[DQ_EXTRA_KEY] = config |
There was a problem hiding this comment.
Please don't use asset.extra. This is going to be removed #55200.
|
@Lee-W regarding extra what other options you would suggest? do we have any other ways to store that now? |
4165252 to
c86e09f
Compare
c86e09f to
c0d1bfb
Compare
SameerMesiah97
left a comment
There was a problem hiding this comment.
Since this is a very large PR, I could only review a few pieces of it but there is some general feedback I can share:
-
In your last dev list response, you mentioned that the provider currently builds on
DbApiHook, but that DataFusion is the next execution engine you plan to add. That made me wonder about the long-term architecture. Since DataFusion isn't a DB-API implementation, I would expect the provider to evolve towards a more general execution layer that both SQL and DataFusion plug into. I am just wondering what this provider would like when this happens? -
I think the docs could be made more objective and instructional. In several places the wording feels a little clunky or promotional, whereas I'd expect Airflow documentation to focus on clearly explaining how to use the feature. Simplifying some of the phrasing would also improve readability. This may come across as a bit pedantic but we have to keep in mind that docs will be the first point of interaction for this provider. LLMs can be very useful for this with good prompts if you think it will be too time-consuming.
-
I spotted some tests which seemed redundant, effectively testing native python behaviour. I would review the other tests to see if they can be trimmed.
Overall, I think this is a very interesting addition at the provider level. Not your typical external service integration.
| .. exampleinclude:: /../src/airflow/providers/common/dataquality/example_dags/example_dq_llm_generated_ruleset.py | ||
| :language: python | ||
| :start-after: [START howto_decorator_dq_check_llm_runtime_ruleset] | ||
| :end-before: [END howto_decorator_dq_check_llm_runtime_ruleset] |
There was a problem hiding this comment.
I understand that this may come across as a bit pedantic but I think the documentation here veers too far into marketing language when I believe they should be purely instructional. Statements like 'Writing a RuleSet by hand for every table doesn't scale' is of course very defensible but it is a bit too opinionated for provider documentation.
I think it would not hurt to use an LLM here with specific instructions to keep it factual. I also found some of the phrasing a little awkward from a native English perspective. Again, a well-prompted LLM should be very useful for this sort of thing.
This feedback applies to the rest of the docs too. I glanced at them and they had similar issues.
| :param asset: An asset decorated with :func:`~airflow.providers.common.dataquality.assets.asset_quality`. | ||
| Supplies defaults for ``ruleset``, ``table``, and ``conn_id`` (explicit arguments | ||
| win) and is automatically added to the task's outlets so its asset events carry | ||
| the check summary. |
There was a problem hiding this comment.
Now, this is a more general concern about the API of this operator: it is a bit hard to understand with the inter-relationships between the different parameters. For example, ruleset, and `table`` are all optional, but become required unless asset supplies them. I think we should construct the API in a way that removes all these interdependencies so that our users would find the operator more intuitive.
Maybe we should simplify the public API by making asset the sole source of configuration when it is provided, rather than allowing table, ruleset, and conn_id to override it. At the moment the operator supports multiple overlapping configuration mechanisms, which introduces precedence rules and conditional required parameters.
| warned = [r.rule_name for r in results if r.status == WARN] | ||
| if self.fail_on == "error" and failed: | ||
| raise DQCheckFailedError(f"Data quality rules failed: {failed} (score={summary['score']})") | ||
| if self.fail_on == "warn" and (failed or warned): |
There was a problem hiding this comment.
I am just curious why you are using raw strings such as "error" and "warn" here instead of an Enum? I see you added an Enum class in this PR. Perhaps, you could add a new class like this:
class FailOn(str, Enum):
ERROR = "error"
WARN = "warn"
NEVER = "never"
Also, this more of a nit but I think we could handle each case more explicity like the below:
if self.fail_on is FailOn.ERROR:
...
elif self.fail_on is FailOn.WARN:
...
elif self.fail_on is FailOn.NEVER:
self.log.warning(...)
|
|
||
|
|
||
| def test_dq_check_failed_error_is_runtime_error(): | ||
| assert isinstance(DQCheckFailedError("failed check"), RuntimeError) |
There was a problem hiding this comment.
Are these tests needed? It seems like you are just testing inheritance.
Adds a new
apache-airflow-providers-common-dataqualityprovider forDbApiHook-based data quality checks.Airflow already has SQL check operators, and many users rely on them for data quality today. This provider adds a
DQRule/RuleSetlayer for checks that need stable rule identity, persisted history, and a connection to Airflow assets. That makes quality results easier to analyze over time, lets downstream asset consumers gate on recent quality, and gives LLM-assisted workflows one schema to generate when proposing checks from table context. Execution still goes through existingcommon.sql/DbApiHookconnections.This PR is the backend/provider slice only. The UI plugin and read-only API are intentionally left for a follow-up PR.
Ships:
DQRuleandRuleSetmodels for named data quality rules.common.sql/DbApiHook.custom_sqlsupport for database-specific or more complex checks.DQCheckOperatorand the@task.dq_checkTaskFlow decorator.[dq] results_pathfor task, run, and rule-level history.asset_quality()andrequire_quality(), that attach provider-owned quality metadata to assets without changing Airflow core.https://github.com/gopidesupavan/airflow/blob/f32940bd261b94238256eaced9150dd51329ce3e/providers/dq/src/airflow/providers/dq/skills/dq-rule-authoring/SKILL.md
This first version is intentionally focused on the backend contract: deterministic rule definitions, SQL execution through existing Airflow SQL providers, persisted results, and asset-linked quality summaries.
Design decisions:
[dq] results_path.Asset.extra["airflow.dq"]; runtime summaries are attached to asset events underextra["airflow.dq.result"].DbApiHook/ SQL execution because Airflow already has broad database coverage throughcommon.sql. File and object-store data checks are left for a later iteration.Later iterations:
DQProfileOperator.Was generative AI tooling used to co-author this PR?
codex
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.