openlineage, common.sql: provide OL SQL parser as internal OpenLineage provider API #31398

JDarDagran · 2023-05-18T19:21:01Z

This PR adds OpenLineage SQL parser as internal provider API.
It also adds support for SQLExecuteQueryOperator. A set of selected SQL providers will be implemented in next PRs.

closes: #29673

ashb

(I haven't looked at the tests in details yet)

airflow/providers/common/sql/hooks/sql.py

airflow/providers/common/sql/operators/sql.py

airflow/providers/openlineage/utils/sqlparser.py

ashb · 2023-05-22T15:10:39Z

airflow/providers/openlineage/utils/sqlparser.py

Is this generic enough to be a worthwhile default, or would making it a required field make more sense?

I'd say both are valuable default values that are common for lots of SQL databases.

airflow/providers/openlineage/utils/sqlparser.py

ashb · 2023-05-22T15:13:36Z

generated/provider_dependencies.json

Does this make it a required dep or optional?

It makes it optional as extra package for pip install

I think we need to revert this PR.

This change add openlineage extra to common.sql but openlineage has never been released. We can not add such extra till we release openlineage

@eladkal So, I think we have chicken and egg problem - the OL provider is not released yet, but it's useless to release without any provider support. Can we not have those changes - which do not change what common.sql does, just add methods to it - without releasing OL provider first?

We often have cases where providers co-depended. In such senario we release the providers in the same wave so users gain the functionality together.

So this PR should have been put on hold till OL provider is ready then merge them in a sequence so both are ready for the same release wave. In providers release cycle we release from main branch we do not cherry pick commits thus anything merged will be released.

Maybe @potiuk have another idea?

airflow/providers/openlineage/extractors/base.py

airflow/providers/common/sql/operators/sql.py

airflow/providers/openlineage/sqlparser.py

airflow/providers/common/sql/hooks/sql.py

tatiana · 2023-06-05T14:25:47Z

airflow/providers/openlineage/utils/sql.py

@@ -0,0 +1,194 @@
+# Licensed to the Apache Software Foundation (ASF) under one


Why do we have the utils folder and didn't add sql to the provider root? If we need a folder, would there be more specific names?

I'd say there are at least 2 kinds of utility files now, which is enough reason for me to have a separate folder for it. Probably some more would show up in near future, e.g. when developing additional support PythonOperator and TaskFlow API.

airflow/providers/openlineage/utils/sql.py

uranusjr · 2023-06-27T09:48:15Z

All the SQL string formatting makes me a bit nervous. Would it be possible to build those SQL with SQLAlchemy instead?

phanikumv · 2023-06-27T09:52:48Z

airflow/providers/common/sql/hooks/sql.py

Suggested change

:return: A :class:`airflow.providers.openlineage.sqlparser.DatabaseInfo` instance.

This can be expressed in the type annotation instead (so you need to add a -> DatabaseInfo if this is deleted)

As it's empty method that may be optionally implemented, is it enough to annotate it with -> DatabaseInfo | None? Does this say enough? I'm no expert in static typing.

JDarDagran · 2023-06-27T13:25:48Z

All the SQL string formatting makes me a bit nervous. Would it be possible to build those SQL with SQLAlchemy instead?

@uranusjr I changed it in 4f685d1

Please review again.

uranusjr · 2023-06-28T07:04:08Z

Some general coding style comments I find confusing:

Why dict(key=value, ...) instead of {}?
There are local imports (sqlparse?) that seemingly can be done globally.
Reasoning is needed for the # type: ignore mypy comments.
Please reformat most of the docstrings to have a short, single sentence summary, a blank line, and longer descriptions in a separate paragraph.

airflow/providers/openlineage/utils/sql.py

airflow/providers/openlineage/extractors/base.py

JDarDagran · 2023-06-28T09:53:32Z

Some general coding style comments I find confusing:

Why dict(key=value, ...) instead of {}?

There are local imports (sqlparse?) that seemingly can be done globally.

Reasoning is needed for the # type: ignore mypy comments.

Please reformat most of the docstrings to have a short, single sentence summary, a blank line, and longer descriptions in a separate paragraph.

@uranusjr

Changed to {}.
Moved import sqlparse to global. Did not find anything else.
I've fixed all typing issues and got rid of # type: ignore.
Reformatted.

airflow/providers/openlineage/utils/sql.py

Implement base methods for SQLExecuteQueryOperator & DbApiHook. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com> Rename methods to expose their purpose for OpenLineage. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

Instead of referencing the SQLParser directly, modify various static methods to class methods instead, so they can use the cls argument to avoid spelling out the class name repeatedly. Also added a few changes to better ultilize type reference and eliminate some verbose type annotations.

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

eladkal · 2023-07-01T07:38:52Z

I think we need to revert this PR.
see my note on https://github.com/apache/airflow/pull/31398/files#r1248622721

This PR adds openlineage extra to common.sql but openlineage provider is not in the release cycle.
We can not add such extra yet - this is an issue for releasing common.sql provider

cc @potiuk @ashb

potiuk · 2023-07-02T10:00:39Z

I think it's perfectly fine. 'openlineage' extra is optional, and I think (looking at the code) all the openlineage imports are under "TYPE_CHECKING" flag. Surely the openlineage package is not yet published, but at most it means that no-one will be able to use "openlineage" extra YET. Not until openlineage provider is released.

But a good thing is that once it is released, it will work as expected witht this common.sql package that is going to be released now - in other words, if in the future openlineage provider is released, there will be no need to release common.sql.

I see fundamentally no problem with releasing common.sql with that code.

JDarDagran requested review from ashb, eladkal, jedcunningham and potiuk as code owners May 18, 2023 19:21

boring-cyborg bot added area:dev-tools area:providers provider:common-sql provider:openlineage AIP-53 labels May 18, 2023

JDarDagran force-pushed the aip-53-sql branch 4 times, most recently from d0e9764 to 774dc8f Compare May 19, 2023 06:39

ashb reviewed May 22, 2023

View reviewed changes

JDarDagran force-pushed the aip-53-sql branch from 774dc8f to 7db5e1f Compare May 25, 2023 13:42

JDarDagran requested a review from ashb May 25, 2023 13:45

JDarDagran force-pushed the aip-53-sql branch 3 times, most recently from 868257d to b4ee4f8 Compare May 30, 2023 10:53

This was referenced May 30, 2023

openlineage, mysql: add OpenLineage support for MySQL #31609

Merged

openlineage, postgres: add OpenLineage support for Postgres #31617

Merged

JDarDagran force-pushed the aip-53-sql branch 3 times, most recently from 1ecb8c9 to e7bbe1f Compare May 31, 2023 12:17

mobuchowski reviewed Jun 1, 2023

View reviewed changes

airflow/providers/openlineage/extractors/base.py Outdated Show resolved Hide resolved

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

airflow/providers/openlineage/sqlparser.py Outdated Show resolved Hide resolved

JDarDagran force-pushed the aip-53-sql branch 2 times, most recently from d8955ff to 6c9d9cf Compare June 3, 2023 12:12

JDarDagran mentioned this pull request Jun 3, 2023

openlineage, snowflake: add OpenLineage support for Snowflake #31696

Merged

tatiana reviewed Jun 5, 2023

View reviewed changes

airflow/providers/common/sql/hooks/sql.py Outdated Show resolved Hide resolved

tatiana reviewed Jun 5, 2023

View reviewed changes

airflow/providers/openlineage/utils/sql.py Outdated Show resolved Hide resolved

pankajastro requested review from tatiana and uranusjr June 27, 2023 05:57

JDarDagran force-pushed the aip-53-sql branch from 8cd5bb1 to 65c8015 Compare June 27, 2023 08:42

phanikumv reviewed Jun 27, 2023

View reviewed changes

JDarDagran force-pushed the aip-53-sql branch from 65c8015 to 4f685d1 Compare June 27, 2023 13:24

uranusjr reviewed Jun 28, 2023

View reviewed changes

airflow/providers/openlineage/utils/sql.py Outdated Show resolved Hide resolved

uranusjr reviewed Jun 28, 2023

View reviewed changes

airflow/providers/openlineage/extractors/base.py Outdated Show resolved Hide resolved

JDarDagran force-pushed the aip-53-sql branch from 4f685d1 to 291405f Compare June 28, 2023 09:50

JDarDagran requested a review from uranusjr June 28, 2023 12:18

uranusjr reviewed Jun 29, 2023

View reviewed changes

airflow/providers/openlineage/utils/sql.py Outdated Show resolved Hide resolved

JDarDagran and others added 5 commits June 29, 2023 10:09

Rewrite information schema query construction to SQLALchemy ORM.

7ce4e23

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

Clean up typing and iterator usage

bc70358

Add static typing to hint returned type.

46ac906

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

JDarDagran force-pushed the aip-53-sql branch from 2062514 to 46ac906 Compare June 29, 2023 08:09

uranusjr approved these changes Jun 29, 2023

View reviewed changes

Fix mypy issues.

3d5f9c1

Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

mobuchowski approved these changes Jun 29, 2023

View reviewed changes

mobuchowski merged commit f2e2125 into apache:main Jun 29, 2023

JDarDagran deleted the aip-53-sql branch June 29, 2023 12:24

eladkal mentioned this pull request Jul 1, 2023

Prepare docs for July 2023 wave of Providers #32298

Merged

1 task

eladkal mentioned this pull request Jul 6, 2023

Status of testing Providers that were prepared on July 06, 2023 #32389

Closed

69 tasks

Sabbir02 mentioned this pull request Jan 21, 2025

openlineage, postgres: add OpenLineage support for Postgres #31617 #45856

Closed

2 tasks

		@@ -0,0 +1,194 @@
		# Licensed to the Apache Software Foundation (ASF) under one

openlineage, common.sql: provide OL SQL parser as internal OpenLineage provider API #31398

openlineage, common.sql: provide OL SQL parser as internal OpenLineage provider API #31398

Uh oh!

Conversation

JDarDagran commented May 18, 2023

Uh oh!

ashb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDarDagran May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

uranusjr commented Jun 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDarDagran Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDarDagran commented Jun 27, 2023

Uh oh!

uranusjr commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JDarDagran commented Jun 28, 2023

Uh oh!

Uh oh!

eladkal commented Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Jul 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

JDarDagran May 23, 2023 •

edited

Loading

JDarDagran Jun 29, 2023 •

edited

Loading

uranusjr commented Jun 28, 2023 •

edited

Loading

eladkal commented Jul 1, 2023 •

edited

Loading

potiuk commented Jul 2, 2023 •

edited

Loading