Add new SQLCheckOperators #23915

denimalpaca · 2022-05-25T15:05:52Z

This pull request adds two new SQL Check Operators to the core sql.py file, one called SQLColumnCheckOperator and one called SQLTableCheckOperator. Corresponding tests are added as well. The current functionality of these operators is minimal, but expected to grow over time. These operators, unlike current ones, save information about the results
of the check queries that is then parseable by third party libraries.

closes: #23874

This commit adds two new SQL Check Operators to the core sql.py file, one called SQLColumnCheckOperator and one called SQLTableCheckOperator. Corresponding tests are added as well. The current functionality of these operators is minimal, but expected to grow over time. These operators, unlike current ones, save information about the results of the check queries that is then parseable by third party libraries.

airflow/operators/sql.py

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

Accidentally copied the last line of the file several times, this commit remedies that change.

denimalpaca · 2022-05-25T16:14:38Z

Question I have for reviewers: would it make sense to add the Snowflake, Bigquery, Redshift, etc... versions of this operator in with this PR, or open a separate issue & PR for the provider updates?

josh-fell · 2022-05-25T17:17:50Z

Question I have for reviewers: would it make sense to add the Snowflake, Bigquery, Redshift, etc... versions of this operator in with this PR, or open a separate issue & PR for the provider updates?

My vote would be for separate ones.

Improved the column check operator to include support for greater than, geq, less than, and leq functions. Updated logic and tests to reflect this change.

Improved the table check operator by refactoring how checks are performed; this makes it much more customizeable at the expense of knowing the specific result of checks, as the results are now all just 0 or 1.

potiuk · 2022-06-06T14:00:18Z

Question I have for reviewers: would it make sense to add the Snowflake, Bigquery, Redshift, etc... versions of this operator in with this PR, or open a separate issue & PR for the provider updates?

Definitely separate. Also to note - those classes will not be available in Airlfow's core until 2.4 release and our providers are 2.1 (and soon 2.2-compatible).

So when you add it to providers, it has to be done in the way that the proivders are importable in 2.2 as well and that those new opertors are treated as optional and provide the right message when someone tries to use the operator in Airflow 2.2/2.3

airflow/operators/sql.py

josh-fell · 2022-06-06T14:24:24Z

airflow/operators/sql.py

+
+    column_checks = {
+        # pass value should be number of acceptable nulls
+        "null_check": "SUM(CASE WHEN 'column' IS NULL THEN 1 ELSE 0 END) AS column_null_check",


Suggested change

"null_check": "SUM(CASE WHEN 'column' IS NULL THEN 1 ELSE 0 END) AS column_null_check",

"null_check": "SUM(CASE WHEN column IS NULL THEN 1 ELSE 0 END) AS column_null_check",

Should be checking for the column value rather than the string of the the column name I think?

Ummm... good question. I'm not 100% sure. Testing on Snowflake, the lack of ' version had a parse error iirc. So it might be a dialect issue that can be resolved in the provider versions.

airflow/operators/sql.py

josh-fell · 2022-06-06T14:34:28Z

airflow/operators/sql.py

+
+        self.table = table
+        self.column_mapping = column_mapping
+        self.sql = f"SELECT * FROM {self.table};"


Just curious, why is this needed here?

For OpenLineage. The Extractor needs some static query to render, unless we want to have the entire templated query rendered in __init__(). I can double-check this, though, it might have changed with the move to the Listener.

Just to reiterate - those "common" SQL classes will not be released unti Airflow 2.4 and it means that the first operator tha will be able to use them as "mandatory" will be 12 months after 2.4 is released (that's our policy for providers), so I just want to make you aware @denimalpaca that those classes won't be to useful for quite some time.

Maybe a better Idea will be to add a separate "base sql" provider which will not be in the "airflow/operators" but it will be a separate package that could be installed additionally to airflow. We have not done so before, but maybe there is a good reason to introduce such a "shared sql" provider that wil encapsulate all SQL-like base functionality that open lineage might base on? I think trying to implement it in Airflow core is a bad idea, if the goal is fast adoption by multiple providers.

Just saying.

Maybe a better Idea will be to add a separate "base sql" provider

I think this is an interesting idea and I support it. We could start with these operators and slowly move existing core SQL operators over too.

@potiuk Do you think this provider would be included with Airflow installs like HTTP, FTP, SQLite, and IMAP, or completely separate?

airflow/operators/sql.py

josh-fell · 2022-06-06T14:54:31Z

tests/operators/test_sql.py

@@ -385,6 +387,82 @@ def test_fail_min_sql_max_value(self, mock_get_db_hook):
            operator.execute()


+class TestColumnCheckOperator(unittest.TestCase):


IMO this can be a pytest test rather than continuing to use unittest.

Is there a reason to use one over the other? I was just copying the test pattern from the other ones.

We prefer pytest test type unless there are good reasons not to (for new test)

josh-fell · 2022-06-06T14:56:05Z

tests/operators/test_sql.py

+    def _construct_operator(self, column_mapping):
+        return SQLColumnCheckOperator(task_id="test_task", table="test_table", column_mapping=column_mapping)
+
+    @mock.patch.object(SQLColumnCheckOperator, "get_db_hook")


Since every test case uses the same mocked object, you could think about using a pytest fixture for mocking get_db_hook. I don't have a strong opinion though.

If you use pytest fixtures, the monkeypatch fixture makes this easy to do in setup_method.

tests/operators/test_sql.py

New docstrings were using example dictionaries in the param list, and the way they were written before caused doc build errors. The fix was to update the formatting and add missing words to the word list.

Moves record logging below the AirflowException for no records, and makes the Exception message more clear.

Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>

SQLTableCheckOperator was rendering an invalid SQL query, this fix changes the way the query template is built from the passed in strings.

… result string

After some research, it seems the null_check query does *not* need quotes around the column name.

denimalpaca · 2022-06-15T14:17:18Z

Closing this PR in favor of #24476

Adds operators, tests, and new and updated docs for a Core SQL Provider. This provider is made in favor of adding these operators to the existing SQL operators in core Airflow. The new provider will allow for quicker development cycles. closes: #23874, #24422 related: #23915

boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label May 25, 2022

uranusjr reviewed May 25, 2022

View reviewed changes

airflow/operators/sql.py Outdated Show resolved Hide resolved

denimalpaca and others added 2 commits May 25, 2022 12:10

Use list comprehension in _get_failed_tests()

a58a3c5

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

Remove repeated lines of code

c6950de

Accidentally copied the last line of the file several times, this commit remedies that change.

denimalpaca added 3 commits May 25, 2022 16:59

Fix doc strings.

1818691

Update SQLColumnCheckOperator

fc8380e

Improved the column check operator to include support for greater than, geq, less than, and leq functions. Updated logic and tests to reflect this change.

Update SQLTableCheckOperator

0f56b08

Improved the table check operator by refactoring how checks are performed; this makes it much more customizeable at the expense of knowing the specific result of checks, as the results are now all just 0 or 1.

josh-fell reviewed Jun 6, 2022

View reviewed changes

Fix doc errors in docstrings

14afffb

New docstrings were using example dictionaries in the param list, and the way they were written before caused doc build errors. The fix was to update the formatting and add missing words to the word list.

denimalpaca requested a review from mik-laj as a code owner June 6, 2022 16:29

denimalpaca and others added 4 commits June 6, 2022 12:38

Reorder logging

eba96f3

Moves record logging below the AirflowException for no records, and makes the Exception message more clear.

Apply suggestions from code review

2388dd8

Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>

Add more information to docstring

cf89bf5

Fix docstring indent error

c83c79e

denimalpaca requested review from uranusjr and josh-fell June 8, 2022 14:17

denimalpaca added 5 commits June 8, 2022 16:58

Remove reminder comments; Add column alias for unique_check query.

7136397

Fix SQL rendering bug in SQLTableCheckOperator

cbc76c1

SQLTableCheckOperator was rendering an invalid SQL query, this fix changes the way the query template is built from the passed in strings.

Add note about OpenLineage compatability; add tab in _get_failed_test…

fa083ea

… result string

Remove quotes around column in null_check

7439986

After some research, it seems the null_check query does *not* need quotes around the column name.

Merge branch 'apache:main' into add_sql_column_table_check_operators

99158b9

denimalpaca mentioned this pull request Jun 15, 2022

Add Core SQL Provider #24476

Merged

denimalpaca closed this Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new SQLCheckOperators #23915

Add new SQLCheckOperators #23915

denimalpaca commented May 25, 2022

denimalpaca commented May 25, 2022

josh-fell commented May 25, 2022

potiuk commented Jun 6, 2022 •

edited

Loading

josh-fell Jun 6, 2022

denimalpaca Jun 6, 2022

josh-fell Jun 6, 2022

denimalpaca Jun 6, 2022

potiuk Jun 12, 2022 •

edited

Loading

josh-fell Jun 13, 2022 •

edited

Loading

josh-fell Jun 6, 2022

denimalpaca Jun 6, 2022

potiuk Jun 12, 2022

josh-fell Jun 6, 2022

uranusjr Jun 14, 2022

denimalpaca commented Jun 15, 2022

	"null_check": "SUM(CASE WHEN 'column' IS NULL THEN 1 ELSE 0 END) AS column_null_check",
	"null_check": "SUM(CASE WHEN column IS NULL THEN 1 ELSE 0 END) AS column_null_check",

		@@ -385,6 +387,82 @@ def test_fail_min_sql_max_value(self, mock_get_db_hook):
		operator.execute()


		class TestColumnCheckOperator(unittest.TestCase):

Add new SQLCheckOperators #23915

Add new SQLCheckOperators #23915

Conversation

denimalpaca commented May 25, 2022

denimalpaca commented May 25, 2022

josh-fell commented May 25, 2022

potiuk commented Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Jun 12, 2022 • edited Loading

Choose a reason for hiding this comment

josh-fell Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denimalpaca commented Jun 15, 2022

potiuk commented Jun 6, 2022 •

edited

Loading

potiuk Jun 12, 2022 •

edited

Loading

josh-fell Jun 13, 2022 •

edited

Loading