kacpermuda commented on code in PR #59921:
URL: https://github.com/apache/airflow/pull/59921#discussion_r2652921013
##########
providers/openlineage/src/airflow/providers/openlineage/sqlparser.py:
##########
@@ -497,6 +498,14 @@ def get_openlineage_facets_with_sql(
try:
sqlalchemy_engine = hook.get_sqlalchemy_engine()
+ except ImportError as e:
+ if "sqlalchemy" in str(e).lower() or "No module named 'sqlalchemy" in
str(e):
+ raise AirflowOptionalProviderFeatureException(
Review Comment:
Yes, I don't think we need another except here for ImportError, the generic
one we have below will do just fine
##########
providers/openlineage/src/airflow/providers/openlineage/utils/sql.py:
##########
@@ -157,6 +163,18 @@ def create_information_schema_query(
sqlalchemy_engine: Engine | None = None,
) -> str:
"""Create query for getting table schemas from information schema."""
+ if (
+ Column is None
+ or MetaData is None
+ or Table is None
+ or and_ is None
+ or or_ is None
+ or union_all is None
+ ):
+ raise AirflowOptionalProviderFeatureException(
+ "sqlalchemy is required for SQL schema query generation. "
+ "Install it with: pip install
'apache-airflow-providers-openlineage[sqlalchemy]'"
+ )
Review Comment:
```suggestion
try:
from sqlalchemy import Column, MetaData, Table, and_, or_, union_all
except ImportError:
raise AirflowOptionalProviderFeatureException(
"sqlalchemy is required for SQL schema query generation. "
"Install it with: pip install
'apache-airflow-providers-openlineage[sqlalchemy]'"
)
```
We need this import in only two places, we can probably go with local
imports? Both are fine for me, just nit.
##########
providers/openlineage/src/airflow/providers/openlineage/utils/sql.py:
##########
@@ -217,6 +235,11 @@ def create_filter_clauses(
therefore it is expected the table has them defined.
:param uppercase_names: if True use schema and table names uppercase
"""
+ if and_ is None or or_ is None:
+ raise AirflowOptionalProviderFeatureException(
+ "sqlalchemy is required for SQL filter clause generation. "
+ "Install it with: pip install
'apache-airflow-providers-openlineage[sqlalchemy]'"
+ )
Review Comment:
```suggestion
try:
from sqlalchemy import and_, or_
except ImportError:
raise AirflowOptionalProviderFeatureException(
"sqlalchemy is required for SQL filter clause generation. "
"Install it with: pip install
'apache-airflow-providers-openlineage[sqlalchemy]'"
)
```
Same as above
##########
providers/openlineage/src/airflow/providers/openlineage/utils/utils.py:
##########
@@ -537,7 +538,13 @@ def is_selective_lineage_enabled(obj: DAG | SerializedDAG
| AnyOperator) -> bool
@provide_session
def is_ti_rescheduled_already(ti: TaskInstance, session=NEW_SESSION):
- from sqlalchemy import exists, select
+ try:
Review Comment:
Just above this snippet, in line 536 you can see that this function is
available only for AF2 (hidden behind `if`), so we should be good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
